Replace products and services with brrg api - #62
pernillehofgaard wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesOpen Endpoint Removal and AnnualFinancialReport Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Dan.Plugin.Brreg/AnnualFinancialReport.cs`:
- Around line 209-223: The broad catch block in the financial report retrieval
logic is masking all upstream failures (timeouts, server errors, authentication
issues) as a permanent "no report available" error, which breaks retry and
alerting behavior. Instead of catching all exceptions, distinguish between
actual "report not available" scenarios (such as specific response codes or
deserialize failures) and upstream infrastructure errors. For genuine "no
report" cases, throw the EvidenceSourcePermanentClientException, but for
transient failures from Requests.MakeRequest, either re-throw the original
exception or wrap it in an appropriate transient exception type that allows
retries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a49487f7-0202-4baf-8a03-5e2bf719763b
📒 Files selected for processing (3)
src/Dan.Plugin.Brreg/AnnualFinancialReport.cssrc/Dan.Plugin.Brreg/Metadata.cssrc/Dan.Plugin.Brreg/Regnskapsregisteret.cs
💤 Files with no reviewable changes (1)
- src/Dan.Plugin.Brreg/Regnskapsregisteret.cs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Dan.Plugin.Brreg/AnnualFinancialReport.cs (1)
40-46: 🩺 Stability & Availability | 🔴 CriticalRegister the
"SafeHttpClient"named client in the DI container.The constructor calls
httpClientFactory.CreateClient("SafeHttpClient")(line 45), but this named client is not registered inProgram.cs. The DI configuration only registers"myMaskinportenClient". Without the registration, either an exception will be thrown at runtime or a default client will be returned, silently losing any intended resiliency/timeout policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Dan.Plugin.Brreg/AnnualFinancialReport.cs` around lines 40 - 46, The AnnualFinancialReport constructor references a named HTTP client called "SafeHttpClient" when calling httpClientFactory.CreateClient("SafeHttpClient"), but this named client is not registered in the DI container in Program.cs. Register the "SafeHttpClient" named client in Program.cs using the same pattern as other HTTP clients (such as "myMaskinportenClient"), ensuring you configure it with any required resiliency policies, timeout settings, or other configuration needed for safe HTTP communication.
🧹 Nitpick comments (2)
src/Dan.Plugin.Brreg/AnnualFinancialReport.cs (2)
302-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused helper duplicates
RunPdfAsynclogic.
GetAnnualFinancialReportPdfreconstructs the exact same URL, Accept/Basic-auth headers, GET, and success check asRunPdfAsync(Lines 101-122), but does not appear to be called. Either wire it into the handler (see the contract comment on Lines 91-128) and remove the inline duplication, or delete it. Keeping both copies will let the two PDF paths drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Dan.Plugin.Brreg/AnnualFinancialReport.cs` around lines 302 - 331, The GetAnnualFinancialReportPdf method duplicates the exact same URL construction, Accept header setup, Basic authentication header logic, GET request, and success status code checking that already exists in the RunPdfAsync method. To fix this, either delete the GetAnnualFinancialReportPdf method entirely if it is unused, or refactor both methods to extract the common HTTP request and header setup logic into a shared helper method that both RunPdfAsync and GetAnnualFinancialReportPdf can call to eliminate the duplication and prevent future drift between the two PDF handling paths.
341-342: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid the serialize→deserialize round-trip.
responseis already a deserializeddynamicfromRequests.MakeRequest; re-serializing then deserializing toList<string>is an extra full JSON round-trip on every call. SinceMakeRequestreturns the parsed JSON, you can cast/convert directly (e.g.((JArray)response).ToObject<List<string>>()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Dan.Plugin.Brreg/AnnualFinancialReport.cs` around lines 341 - 342, The availableYears assignment is performing an unnecessary JSON serialize-deserialize round-trip on the already-deserialized response object from Requests.MakeRequest. Replace the JsonConvert.SerializeObject and JsonConvert.DeserializeObject pattern with a direct cast and conversion using the ToObject method (for example, cast response to JArray and call ToObject<List<string>>()) to eliminate the inefficient round-trip and improve performance on every call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Dan.Plugin.Brreg/AnnualFinancialReport.cs`:
- Around line 97-101: The year parameter extracted via TryGetParameter is not
validated before being interpolated into the URL path for the
RegnskapsregisteretUri endpoint. Add validation after the TryGetParameter call
to check that the method returned true (indicating year was successfully
retrieved) and that year matches an expected format such as a four-digit numeric
year. Only proceed to build the url variable if validation passes, otherwise
handle the validation failure by returning an error response or appropriate
exception. This prevents malformed URLs, null/empty path segments, and potential
path traversal issues.
- Around line 91-128: The RunPdfAsync method is returning raw PDF bytes directly
instead of conforming to the evidence framework contract. Replace the current
response logic that directly returns PDF bytes with a call to
EvidenceSourceResponse.CreateResponse(), passing a lambda that calls the
existing GetAnnualFinancialReportPdf() helper method (which properly converts
the PDF to base64 and wraps it in EvidenceValue objects). This will align the
RunPdfAsync endpoint with the contract defined in GetDefinitionPdf() and make
the implementation consistent with all other handlers in the class that use
EvidenceSourceResponse.CreateResponse().
---
Outside diff comments:
In `@src/Dan.Plugin.Brreg/AnnualFinancialReport.cs`:
- Around line 40-46: The AnnualFinancialReport constructor references a named
HTTP client called "SafeHttpClient" when calling
httpClientFactory.CreateClient("SafeHttpClient"), but this named client is not
registered in the DI container in Program.cs. Register the "SafeHttpClient"
named client in Program.cs using the same pattern as other HTTP clients (such as
"myMaskinportenClient"), ensuring you configure it with any required resiliency
policies, timeout settings, or other configuration needed for safe HTTP
communication.
---
Nitpick comments:
In `@src/Dan.Plugin.Brreg/AnnualFinancialReport.cs`:
- Around line 302-331: The GetAnnualFinancialReportPdf method duplicates the
exact same URL construction, Accept header setup, Basic authentication header
logic, GET request, and success status code checking that already exists in the
RunPdfAsync method. To fix this, either delete the GetAnnualFinancialReportPdf
method entirely if it is unused, or refactor both methods to extract the common
HTTP request and header setup logic into a shared helper method that both
RunPdfAsync and GetAnnualFinancialReportPdf can call to eliminate the
duplication and prevent future drift between the two PDF handling paths.
- Around line 341-342: The availableYears assignment is performing an
unnecessary JSON serialize-deserialize round-trip on the already-deserialized
response object from Requests.MakeRequest. Replace the
JsonConvert.SerializeObject and JsonConvert.DeserializeObject pattern with a
direct cast and conversion using the ToObject method (for example, cast response
to JArray and call ToObject<List<string>>()) to eliminate the inefficient
round-trip and improve performance on every call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 769d3e2c-e0cc-4891-996c-846f598a1db2
📒 Files selected for processing (2)
src/Dan.Plugin.Brreg/AnnualFinancialReport.cssrc/Dan.Plugin.Brreg/Metadata.cs
1 participant
Description
Replace products and services lookup with brønnøysund api when fetching annual financial resports and årsregnskap
Documentation
Summary by CodeRabbit
Release Notes
New Features
Refactor
Breaking Changes
Documentation