Repository navigation
feat: add ccrwrapper endpoints - #196
Conversation
|
WalkthroughAdds a CCR HTTP client and exception, BRREG pagination and hierarchy models, new core registry interfaces and implementations with caching and hierarchy logic, new Azure Functions for CCR lookups, deprecates legacy registry APIs, refactors caching to distributed cache, removes a PPE proxy function, and bumps many NuGet packages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dan.PluginTest/Plugin.cs (1)
16-22: Remove unused variable and externalize hard-coded values in PluginGenericTestLine 141: The
unitvariable is assigned but never used. Additionally, the call contains hard-coded account ID ("923609016") and environment ("local") that should be externalized to configuration or derived from runtime context.var unit = await ccrClientService.IsPublic("923609016", "local");Either complete the feature (use the result) or remove the call. If needed, move the CCR validation logic into a proper service method with injected configuration for account ID and environment resolution.
Dan.Common/Services/EntityRegistryService.cs (1)
113-123: Guard against null OverordnetEnhet before lookup.The do/while dereferences OverordnetEnhet via null-forgiving and can issue a bogus upstream call (e.g., “.../underenheter/”) if OverordnetEnhet is null. Switch to a guarded while and bail if lookup fails.
Apply:
- var parentUnit = unit; - do - { - // Only subunits are at the leaf node, any nested parents are MainUnits - // Example: https://data.brreg.no/enhetsregisteret/api/underenheter/879587662 - parentUnit = await InternalGet(parentUnit.OverordnetEnhet!, UnitType.MainUnit); - } - while (parentUnit?.OverordnetEnhet != null); - unit = parentUnit; + var parentUnit = unit; + while (!string.IsNullOrEmpty(parentUnit.OverordnetEnhet)) + { + var nextOrg = parentUnit.OverordnetEnhet!; + var next = await InternalGet(nextOrg, UnitType.MainUnit); + if (next is null) break; // upstream returned nothing; stop nesting + parentUnit = next; + } + unit = parentUnit;
🧹 Nitpick comments (21)
Dan.Core/Services/Interfaces/IEntityRegistryApiClientService.cs (1)
9-20: Document the temporary namespace duplication.Having
IEntityRegistryApiClientServicein bothDan.Common.InterfacesandDan.Core.Services.Interfacesmay cause confusion. Consider adding an XML comment noting this is the preferred interface and the Common version is deprecated.Example addition:
/// <summary> /// API Service for fetching entity registries /// </summary> +/// <remarks> +/// This interface supersedes Dan.Common.Interfaces.IEntityRegistryApiClientService which is deprecated. +/// </remarks> public interface IEntityRegistryApiClientServiceDan.Core/Services/ConsentService.cs (2)
204-211: Sanitize logging to avoid leaking URLs/authorization artifactsAvoid logging full URLs/request messages that may include sensitive values. Log only operation and status.
- _logger.LogInformation("Getting jwt: " + url); + _logger.LogInformation("Getting JWT from Altinn consent status endpoint"); ... - _logger.LogInformation($"JWT-get {result.RequestMessage} result: {result.StatusCode} : {result.ReasonPhrase}"); + _logger.LogInformation("JWT-get result: {StatusCode} : {Reason}", result.StatusCode, result.ReasonPhrase);
459-461: Use culture-safe, allocation-free comparisonUse OrdinalIgnoreCase instead of ToLower().Contains to avoid culture issues and allocs.
- if (!Settings.AltinnServiceOwnerApiUri.ToLower().Contains("tt02")) + if (!Settings.AltinnServiceOwnerApiUri.Contains("tt02", StringComparison.OrdinalIgnoreCase))Dan.Core/Services/AuthorizationRequestValidatorService.cs (1)
20-21: Entity registry interface swap: OK; confirm DI and env flag handlingThe type switch aligns with the CCR refactor. Ensure DI is updated and that AllowTestCcrLookup isn’t toggled on a singleton instance.
Reuse the DI verification script from the ConsentService comment to confirm registrations and service lifetime.
Consider moving the environment toggle into DI configuration to avoid runtime mutation of shared state.
Also applies to: 38-40, 49-50
Dan.Core/Services/RequirementValidationService.cs (2)
20-21: Entity registry interface swap and env flag: looks fineType change is consistent with the migration. Same note on DI and mutable AllowTestCcrLookup as in other services.
Confirm DI registration and service lifetime using the earlier script.
Prefer configuring env behavior via options at registration time.
Also applies to: 35-38, 47-48
65-83: Avoid Task.Run around already-async workWrapping an async method in Task.Run adds unnecessary scheduling and overhead. Collect the Task directly.
- var taskList = new List<Task>(); + var taskList = new List<Task>(); foreach (var er in _authRequest.EvidenceRequests) { ... foreach (var req in evidenceCodeRequirements[er.EvidenceCodeName]) { if (req.AppliesToServiceContext.Count > 0 && !req.AppliesToServiceContext.Contains(_requestContextService.ServiceContext.Name)) continue; - - taskList.Add(Task.Run(() => ValidateSingleRequirement(req, er.EvidenceCodeName))); + taskList.Add(ValidateSingleRequirement(req, er.EvidenceCodeName)); } } await Task.WhenAll(taskList.ToArray());Dan.Core/FuncOpenData.cs (1)
82-107: Decouple from EntityRegistryService constants and align with new CCR wrapperFuncOpenData still special-cases CCR datasets and references concrete EntityRegistryService.CcrProxy* constants. With CCR lookups moving to dedicated endpoints, consider:
- Removing this CCR branch or delegating to the new CCR wrapper.
- Moving the dataset-name constants to a neutral place (config/constants) to avoid coupling to a concrete class.
Would you like a follow-up diff to retire this path and route callers to FuncCcrWrapper?
Also applies to: 105-108
Dan.Common/Models/BrregPage.cs (1)
1-1: Json.NET attributes resolve via global using; optional DTO improvements availableNewtonsoft.Json is already globally available via
Dan.Common/GlobalUsings.cs, so no file-level using is required.Consider sealing these DTO classes or using record types for immutability and clarity. Optionally rename
SubunitstoBrregSubunitsfor clearer call sites.Dan.Core.UnitTest/Services/EntityRegistryServiceTest.cs (1)
62-67: Prefer async/await over.Resultin testsAvoid blocking on Tasks in MSTest. Convert these tests to
async Taskandawaitthe calls for clarity and to prevent deadlocks.Example for one test (apply similarly to the others):
- [TestMethod] - public void TestGetMapping() + [TestMethod] + public async Task TestGetMapping() { - var s = new EntityRegistryService(entityRegistryApiClientServiceMock.Object, logger.Object); - var r = s.Get("91").Result; + var s = new EntityRegistryService(entityRegistryApiClientServiceMock.Object, logger.Object); + var r = await s.Get("91"); Assert.AreEqual("91", r?.OrganizationNumber); }Also applies to: 70-75, 78-83, 86-91, 94-99, 102-107, 110-115, 118-123, 126-131, 134-139
Dan.Core/Services/Interfaces/IEntityRegistryService.cs (2)
16-23: Fix XML summaries: methods return units, not “organization number”Adjust summaries to reflect that these methods retrieve entity registry “units” (simple or full), not just the number.
Also applies to: 26-33
23-33: Consider adding CancellationToken to public APIThese methods perform network I/O downstream. Expose
CancellationTokenparameters (optional with default) to propagate cancellations from HTTP requests and background jobs. This is easier to do now before the interface is widely consumed.Also applies to: 35-41, 47-69
Dan.Core/Services/CachingEntityRegistryApiClientService.cs (1)
121-126: Log exception details on deserialization failuresCapture the exception to aid diagnostics.
- catch (Exception) - { - logger.LogError("Failed deserializing from BR on {registryApiUri}", registryApiUri); - throw; - } + catch (Exception ex) + { + logger.LogError(ex, "Failed deserializing from BR on {RegistryApiUri}", registryApiUri); + throw; + }Dan.Core/Services/EntityRegistryService.cs (2)
270-279: Build list-lookup URI without a stray slashUsing
".../underenheter/{0}"with"?overordnetEnhet=..."yields.../underenheter/?overordnetEnhet=.... Prefer a dedicated list endpoint pattern to avoid reliance on server tolerance.- private static Uri GetLookupUrlForSubunitsOfAUnit(string organizationNumber) - { - var urlPattern = IsSyntheticOrganizationNumber(organizationNumber) ? - PpeSubUnitLookupEndpoint : - SubUnitLookupEndpoint; - - var query = $"?overordnetEnhet={organizationNumber}"; - - return new Uri(string.Format(urlPattern, query)); - } + private static Uri GetLookupUrlForSubunitsOfAUnit(string organizationNumber) + { + var baseUrl = IsSyntheticOrganizationNumber(organizationNumber) + ? "https://data.ppe.brreg.no/enhetsregisteret/api/underenheter" + : "https://data.brreg.no/enhetsregisteret/api/underenheter"; + return new Uri($"{baseUrl}?overordnetEnhet={organizationNumber}"); + }
195-198: Defensive check in IsSyntheticOrganizationNumberAvoid
StartsWithon null/empty and use char-check for speed.- private static bool IsSyntheticOrganizationNumber(string organizationNumber) - { - return organizationNumber.StartsWith('2') || organizationNumber.StartsWith('3'); - } + private static bool IsSyntheticOrganizationNumber(string organizationNumber) + { + if (string.IsNullOrEmpty(organizationNumber)) return false; + var c = organizationNumber[0]; + return c == '2' || c == '3'; + }Dan.Core/FuncCcrWrapper.cs (1)
12-29: Validate orgNumber and return 400 on invalid inputEarly reject clearly invalid org numbers (e.g., non-digits, wrong length) to save downstream calls and clarify client errors.
Example (apply to each handler):
public async Task<HttpResponseData> RunCcrOrgnumberLookup( [HttpTrigger(AuthorizationLevel.Anonymous, "get", Route = "ccr/{orgNumber}")] HttpRequestData req, ILogger log, string orgNumber) { + if (string.IsNullOrWhiteSpace(orgNumber) || orgNumber.Length != 9 || !orgNumber.All(char.IsDigit)) + { + var bad = req.CreateResponse(HttpStatusCode.BadRequest); + await bad.WriteStringAsync("orgNumber must be 9 digits."); + return bad; + } var unit = await entityRegistryService.GetFull(orgNumber, attemptSubUnitLookupIfNotFound: true);Also applies to: 31-42, 44-61, 63-74, 76-93
Dan.Common/Services/EntityRegistryService.cs (1)
201-204: Minor: align synthetic check with char-based variant used elsewhere.Slightly faster and consistent with CcrClientService.
- private bool IsSyntheticOrganizationNumber(string organizationNumber) - { - return organizationNumber.StartsWith("2") || organizationNumber.StartsWith("3"); - } + private static bool IsSyntheticOrganizationNumber(string organizationNumber) => + !string.IsNullOrEmpty(organizationNumber) && (organizationNumber[0] is '2' or '3');Dan.Common/Services/CcrClientService.cs (5)
145-156: Add cancellation support in HTTP calls.Improve resiliency by allowing call sites to cancel requests.
- private async Task<T?> GetCcrResponse<T>(string url) + private async Task<T?> GetCcrResponse<T>(string url, CancellationToken ct = default) { HttpResponseMessage response; try { - response = await httpClient.GetAsync(url); + response = await httpClient.GetAsync(url, ct); }(Calls don’t need changes due to default parameter; optionally thread a token through later.)
Also applies to: 150-156
161-166: Handle 200 OK with empty body.Avoid deserializing empty content; treat as not found/none.
- var responseString = await response.Content.ReadAsStringAsync(); - var unit = JsonConvert.DeserializeObject<T>(responseString); + var responseString = await response.Content.ReadAsStringAsync(); + if (string.IsNullOrWhiteSpace(responseString)) + return default; + var unit = JsonConvert.DeserializeObject<T>(responseString); return unit;
154-156: Log URL for failed requests.Adds context to logs without leaking payload.
- logger.LogError(e, "Failed to get CCR response: {message}", e.Message); + logger.LogError(e, "Failed to get CCR response. GET {url}. Error: {message}", url, e.Message);
58-63: Nit: fix “Crr” vs “Ccr” constant naming.Cosmetic consistency only.
- private const string CrrBaseDev = "https://dev-api.data.altinn.no/v1/public/ccr"; - private const string CrrBaseTest = "https://test-api.data.altinn.no/v1/public/ccr"; - private const string CrrBaseProd = "https://api.data.altinn.no/v1/public/ccr"; + private const string CcrBaseDev = "https://dev-api.data.altinn.no/v1/public/ccr"; + private const string CcrBaseTest = "https://test-api.data.altinn.no/v1/public/ccr"; + private const string CcrBaseProd = "https://api.data.altinn.no/v1/public/ccr"; @@ - "dev" => CrrBaseDev, - "test" => CrrBaseTest, - "prod" => CrrBaseProd, + "dev" => CcrBaseDev, + "test" => CcrBaseTest, + "prod" => CcrBaseProd,Also applies to: 191-207
69-143: DRY: synthetic guard repeated in 5 methods.Extract guard to a small helper to reduce duplication and drift.
Example:
private bool BlockedSynthetic(string org) => IsSyntheticOrganizationNumber(org) && !AllowTestCcrLookup;Then early-return where applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
Dan.Common.UnitTest/Dan.Common.UnitTest.csproj(2 hunks)Dan.Common/Dan.Common.csproj(1 hunks)Dan.Common/Exceptions/CcrException.cs(1 hunks)Dan.Common/Extensions/HostBuilderExtensions.cs(1 hunks)Dan.Common/Interfaces/IEntityRegistryApiClientService.cs(1 hunks)Dan.Common/Interfaces/IEntityRegistryService.cs(1 hunks)Dan.Common/Models/BrregPage.cs(1 hunks)Dan.Common/Models/EntityRegistryUnit.cs(1 hunks)Dan.Common/Models/EntityRegistryUnitHierarchy.cs(1 hunks)Dan.Common/Services/CcrClientService.cs(1 hunks)Dan.Common/Services/DefaultEntityRegistryApiClientService.cs(1 hunks)Dan.Common/Services/EntityRegistryService.cs(4 hunks)Dan.Core.UnitTest/AuthorizationRequestValidatorServiceTest.cs(0 hunks)Dan.Core.UnitTest/ConsentServiceTest.cs(0 hunks)Dan.Core.UnitTest/Dan.Core.UnitTest.csproj(1 hunks)Dan.Core.UnitTest/RequirementValidationServiceTest.cs(0 hunks)Dan.Core.UnitTest/Services/EntityRegistryServiceTest.cs(2 hunks)Dan.Core/Config/Settings.cs(1 hunks)Dan.Core/Dan.Core.csproj(1 hunks)Dan.Core/FuncCcrWrapper.cs(1 hunks)Dan.Core/FuncConsentReceipt.cs(2 hunks)Dan.Core/FuncOpenData.cs(1 hunks)Dan.Core/FuncPpeProxy.cs(0 hunks)Dan.Core/Middleware/ExceptionHandlerMiddleware.cs(1 hunks)Dan.Core/Program.cs(2 hunks)Dan.Core/Services/AltinnCorrespondenceService.cs(1 hunks)Dan.Core/Services/AuthorizationRequestValidatorService.cs(2 hunks)Dan.Core/Services/CachingEntityRegistryApiClientService.cs(4 hunks)Dan.Core/Services/ConsentService.cs(2 hunks)Dan.Core/Services/EntityRegistryService.cs(1 hunks)Dan.Core/Services/Interfaces/IEntityRegistryApiClientService.cs(1 hunks)Dan.Core/Services/Interfaces/IEntityRegistryService.cs(1 hunks)Dan.Core/Services/RequirementValidationService.cs(2 hunks)Dan.PluginTest/Config/PluginConstants.cs(1 hunks)Dan.PluginTest/Dan.PluginTest.csproj(1 hunks)Dan.PluginTest/Metadata.cs(1 hunks)Dan.PluginTest/Plugin.cs(2 hunks)
💤 Files with no reviewable changes (4)
- Dan.Core.UnitTest/ConsentServiceTest.cs
- Dan.Core.UnitTest/AuthorizationRequestValidatorServiceTest.cs
- Dan.Core.UnitTest/RequirementValidationServiceTest.cs
- Dan.Core/FuncPpeProxy.cs
🧰 Additional context used
🧬 Code graph analysis (17)
Dan.Common/Services/DefaultEntityRegistryApiClientService.cs (1)
Dan.Common/Services/EntityRegistryService.cs (1)
Obsolete(10-299)
Dan.Common/Extensions/HostBuilderExtensions.cs (1)
Dan.Common/Services/CcrClientService.cs (1)
CcrClientService(56-208)
Dan.Common/Exceptions/CcrException.cs (1)
Dan.Core/Helpers/EvidenceSourceHelper.cs (1)
Exception(226-229)
Dan.Core/Program.cs (3)
Dan.Core/Services/EntityRegistryService.cs (1)
EntityRegistryService(7-280)Dan.Core/Services/CachingEntityRegistryApiClientService.cs (1)
CachingEntityRegistryApiClientService(11-154)Dan.Common/Models/EntityRegistryUnit.cs (1)
EntityRegistryUnit(8-100)
Dan.Common/Interfaces/IEntityRegistryService.cs (2)
Dan.Common/Services/DefaultEntityRegistryApiClientService.cs (1)
Obsolete(8-36)Dan.Common/Services/EntityRegistryService.cs (1)
Obsolete(10-299)
Dan.Core/Services/Interfaces/IEntityRegistryApiClientService.cs (1)
Dan.Common/Interfaces/IEntityRegistryApiClientService.cs (1)
Task(12-12)
Dan.Core/Services/Interfaces/IEntityRegistryService.cs (5)
Dan.Common/Services/EntityRegistryService.cs (3)
SimpleEntityRegistryUnit(232-251)IsPublicAgency(179-185)IsPublicAgency(190-190)Dan.Common/Models/SimpleEntityRegistryUnit.cs (1)
SimpleEntityRegistryUnit(4-13)Dan.Common/Models/EntityRegistryUnit.cs (1)
EntityRegistryUnit(8-100)Dan.Common/Models/EntityRegistryUnitHierarchy.cs (1)
EntityRegistryUnitHierarchy(3-10)Dan.Common/Interfaces/IEntityRegistryService.cs (2)
IsPublicAgency(97-97)IsPublicAgency(104-104)
Dan.Common/Models/EntityRegistryUnitHierarchy.cs (2)
Dan.Common/Models/EntityRegistryUnit.cs (1)
EntityRegistryUnit(8-100)Dan.Common/Models/BrregPage.cs (1)
Subunits(32-39)
Dan.PluginTest/Plugin.cs (3)
Dan.Core/FuncCcrWrapper.cs (5)
Function(12-29)Function(31-42)Function(44-61)Function(63-74)Function(76-92)Dan.PluginTest/Config/PluginConstants.cs (1)
PluginConstants(3-17)Dan.Common/Util/EvidenceSourceResponse.cs (1)
EvidenceSourceResponse(9-112)
Dan.PluginTest/Metadata.cs (2)
Dan.PluginTest/Config/PluginConstants.cs (1)
PluginConstants(3-17)Dan.Common/Models/EvidenceValue.cs (1)
SchemaFromObject(79-82)
Dan.Common/Models/BrregPage.cs (1)
Dan.Common/Models/EntityRegistryUnit.cs (2)
Links(147-154)EntityRegistryUnit(8-100)
Dan.Core/Services/CachingEntityRegistryApiClientService.cs (5)
Dan.Common/Models/EntityRegistryUnit.cs (4)
Organisasjonsform(162-172)Links(147-154)Link(156-160)EntityRegistryUnit(8-100)Dan.Common/Services/EntityRegistryService.cs (3)
Uri(253-266)Uri(268-281)Uri(283-298)Dan.Common/Services/DefaultEntityRegistryApiClientService.cs (1)
Task(24-35)Dan.Core/Config/Settings.cs (1)
Settings(11-400)Dan.Common/Models/BrregPage.cs (2)
BrregPage(8-27)Subunits(32-39)
Dan.Common/Interfaces/IEntityRegistryApiClientService.cs (2)
Dan.Common/Services/DefaultEntityRegistryApiClientService.cs (1)
Obsolete(8-36)Dan.Common/Services/EntityRegistryService.cs (1)
Obsolete(10-299)
Dan.Core/FuncCcrWrapper.cs (3)
Dan.Common/Interfaces/IEntityRegistryService.cs (9)
Task(27-27)Task(34-34)Task(44-44)Task(51-51)Task(72-72)Task(90-90)Task(111-111)IsPublicAgency(97-97)IsPublicAgency(104-104)Dan.Common/Services/EntityRegistryService.cs (2)
IsPublicAgency(179-185)IsPublicAgency(190-190)Dan.Core/Services/EntityRegistryService.cs (1)
IsPublicAgency(187-193)
Dan.Common/Services/CcrClientService.cs (4)
Dan.Common/Models/EntityRegistryUnit.cs (1)
EntityRegistryUnit(8-100)Dan.Common/Models/EntityRegistryUnitHierarchy.cs (1)
EntityRegistryUnitHierarchy(3-10)Dan.Common/Constants.cs (1)
Constants(7-33)Dan.Common/Exceptions/CcrException.cs (4)
CcrException(6-34)CcrException(12-14)CcrException(21-23)CcrException(31-33)
Dan.Core/Services/EntityRegistryService.cs (5)
Dan.Common/Models/EntityRegistryUnit.cs (2)
EntityRegistryUnit(8-100)Organisasjonsform(162-172)Dan.Core/Services/Interfaces/IEntityRegistryService.cs (6)
Task(23-23)Task(33-33)Task(40-40)Task(47-47)Task(57-61)Task(68-68)Dan.Common/Services/EntityRegistryService.cs (7)
SimpleEntityRegistryUnit(232-251)IsSyntheticOrganizationNumber(201-204)IsPublicAgency(179-185)IsPublicAgency(190-190)Uri(253-266)Uri(268-281)Uri(283-298)Dan.Common/Models/SimpleEntityRegistryUnit.cs (1)
SimpleEntityRegistryUnit(4-13)Dan.Common/Services/CcrClientService.cs (1)
IsSyntheticOrganizationNumber(186-189)
Dan.Core.UnitTest/Services/EntityRegistryServiceTest.cs (3)
Dan.Core/Services/EntityRegistryService.cs (5)
EntityRegistryService(7-280)Uri(252-259)Uri(261-268)Uri(270-279)IsPublicAgency(187-193)Dan.Common/Interfaces/IEntityRegistryApiClientService.cs (1)
Task(12-12)Dan.Common/Models/EntityRegistryUnit.cs (4)
EntityRegistryUnit(8-100)Organisasjonsform(162-172)SektorKodeDto(138-145)NaeringsKodeDto(126-136)
🪛 GitHub Check: build-and-test
Dan.Common/Models/EntityRegistryUnitHierarchy.cs
[warning] 7-7:
Missing XML comment for publicly visible type or member 'EntityRegistryUnitHierarchy.Unit'
[warning] 5-5:
Missing XML comment for publicly visible type or member 'EntityRegistryUnitHierarchy.OrgNumber'
[warning] 3-3:
Missing XML comment for publicly visible type or member 'EntityRegistryUnitHierarchy'
[warning] 9-9:
Non-nullable property 'Subunits' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
[warning] 7-7:
Non-nullable property 'Unit' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
[warning] 5-5:
Non-nullable property 'OrgNumber' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (23)
Dan.Common.UnitTest/Dan.Common.UnitTest.csproj (2)
12-20: Test framework updates are straightforward; approve dependency versions.The incremental updates to test-related packages (FluentAssertions, MSTest, coverlet) are all minor or patch versions and represent standard maintenance.
27-29: Verify the new Services folder structure is properly wired.The addition of
<Folder Include="Services\" />aligns with the CCR-related service reorganization mentioned in the PR summary. Ensure that test files in this folder are discoverable and the folder structure is properly reflected in your local development environment.Dan.Common/Dan.Common.csproj (2)
37-38:⚠️ Verify code compatibility with Azure Functions v2.0+ MAJOR version updates.The updates to
Microsoft.Azure.Functions.Worker(1.23.0 → 2.1.0) andMicrosoft.Azure.Functions.Worker.ApplicationInsights(1.3.0 → 2.0.0) are MAJOR version bumps that introduce breaking changes. Ensure that:
- Function initialization, configuration, and dependency injection code is updated for v2.x patterns
- ApplicationInsights integration is compatible with v2.0.0
- Host builder and worker setup follows new conventions
Please verify that all code changes necessary to support these breaking changes are present in other files in this PR. Specifically:
- Check if
HostBuilderExtensionsor function initialization code has been updated- Confirm logging/ApplicationInsights wiring is compatible with v2.0.0
- Run the test suite to ensure no runtime errors occur with the new versions
39-41: Verify Microsoft.Extensions v9.0.9 compatibility across codebase.The updates to
Microsoft.Extensions.Caching.Memory,Microsoft.Extensions.Caching.StackExchangeRedis, andMicrosoft.Extensions.Http.Pollyfrom 8.0.x to 9.0.9 represent MAJOR version increments. Typical breaking changes in v9.x include API surface adjustments, configuration changes, and deprecation removals.Confirm that:
- Caching configuration and usage patterns are compatible with v9.0.9
- Polly policy configuration/registration is compatible with v9.0.9
- The codebase uses only public APIs that remain in v9.x
Dan.Core.UnitTest/Dan.Core.UnitTest.csproj (1)
12-24: Dependency updates verified as compatible.The upgrade of Microsoft.Extensions.Caching.Memory from 8.0.1 to 9.0.9 is safe for the net8.0 target. There are no package-specific API breaking changes between 8.0.1 and the 9.0.x series, and the NuGet package for the 9.x releases targets .NET 8.0, ensuring full compatibility. The changes include dependency updates and security improvements (CVE-2024-43483 fix). All other dependency bumps (MSTest packages, FluentAssertions, coverlet) are minor/patch versions. The FluentAssertions rationale (v8 subscription model) is well documented.
Dan.Core/Middleware/ExceptionHandlerMiddleware.cs (1)
109-110: No action required — error handling status codes are correctly maintained.All instances of
WriteAsJsonAsyncthroughout the codebase follow this pattern: the status code is set on the response object (viaCreateResponse(statusCode)or theStatusCodeproperty) before callingWriteAsJsonAsync(errorModel)without a parameter. This is the consistent, correct API usage for Azure Functions Worker 2.1.0. The status codes remain properly configured regardless of the parameter removal, so error responses will continue to return 401, 404, 500, 503, etc., as intended.Dan.Core/Config/Settings.cs (1)
363-367: LGTM!The new
CcrUrlproperty follows the established pattern for configuration settings and is properly documented.Dan.Common/Interfaces/IEntityRegistryService.cs (1)
6-6: LGTM!The deprecation is clearly communicated with a migration path to
ICcrClientService.Dan.Common/Exceptions/CcrException.cs (1)
1-34: LGTM!The exception class follows standard exception patterns with proper constructor chaining and clear documentation.
Dan.Common/Services/DefaultEntityRegistryApiClientService.cs (1)
8-8: LGTM!The deprecation aligns with the broader migration to
ICcrClientServiceand provides clear guidance.Dan.Common/Extensions/HostBuilderExtensions.cs (1)
118-118: LGTM!The DI registration for
ICcrClientServicefollows the established pattern and uses an appropriate transient lifetime.Dan.PluginTest/Dan.PluginTest.csproj (1)
11-12: LGTM!The package updates are minor/patch versions that align with the broader dependency updates in Dan.Core.
Dan.Common/Models/EntityRegistryUnit.cs (1)
53-55: LGTM!The new
Underenheterproperty follows the established pattern and properly maps the nested sub-units data from CCR responses.Dan.Core/Dan.Core.csproj (1)
36-68: Review comment is not supported by evidence.The codebase uses Polly through backward-compatible patterns (PolicyRegistry, Policy.CacheAsync), which remain stable in v8.6.3. The web search confirms Microsoft.Extensions.Caching 9.0.9 introduces no breaking changes to the IDistributedCache interface used throughout the code. Version updates are compatible with existing usage patterns.
Dan.PluginTest/Config/PluginConstants.cs (1)
16-16: LGTM!The new constant follows the existing naming pattern and is properly integrated with the metadata and plugin test flow.
Dan.Common/Interfaces/IEntityRegistryApiClientService.cs (1)
6-6: LGTM! Deprecation properly signals the migration path.The Obsolete attribute correctly guides consumers to use
ICcrClientServiceinstead.Dan.PluginTest/Metadata.cs (1)
96-111: LGTM!The new evidence code entry is properly structured and consistent with the existing
DatasetOneentry pattern.Dan.Core/FuncConsentReceipt.cs (1)
39-51: Verify removal of UseCoreProxy configuration.Similar to
AltinnCorrespondenceService, the initialization_entityRegistryService.UseCoreProxy = false;was removed. Ensure this is intentional and the default behavior is correct for this service.Dan.Core/Program.cs (2)
138-140: LGTM! Good use of explicit namespacing during migration.The comment clearly documents the temporary nature of the explicit namespacing, which helps prevent confusion during the transition period.
172-177: LGTM! Cache policy properly configured.The new
EntityRegistryListCachePolicyis correctly configured with appropriate serialization and 12-hour expiration, matching the pattern used for single entity caching.Dan.Core/FuncOpenData.cs (1)
23-24: Interface type change: OK; confirm DIType update is consistent. Ensure DI resolves Services.Interfaces.IEntityRegistryService in this project.
Use the DI verification script from earlier comments to confirm registration.
Also applies to: 27-32
Dan.Core/Services/CachingEntityRegistryApiClientService.cs (1)
18-18: Remove review comment; constant is actively used in cache policy registrationThe
EntityRegistryListCachePolicyconstant is used inDan.Core/Program.cs:173where it's passed toPolicy.CacheAsync()as part of the cache policy registration setup. This is the intended usage location, confirming the constant is not dead code.Likely an incorrect or invalid review comment.
Dan.Common/Services/EntityRegistryService.cs (1)
10-13: No configuration issues found — C# 12 features are properly supported.The repository targets
net8.0across all projects, which defaults to C# 12 language version. NoLangVersionrestrictions exist globally or per-project. The primary constructor syntax (line 11-12) and collection expressions (lines 44, 45, 49) are valid C# 12 features that are correctly supported in this configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Dan.Common/Services/EntityRegistryService.cs (1)
51-51: Remove unused SubunitListCache declaration.This cache is declared but never read or written in this file. The actual cache implementation exists in
Dan.Core/Services/EntityRegistryService.cs. Remove this unused declaration to eliminate dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Dan.Common/GlobalUsings.cs(1 hunks)Dan.Common/Models/EntityRegistryUnitHierarchy.cs(1 hunks)Dan.Common/Services/EntityRegistryService.cs(3 hunks)Dan.Core/FuncConsentReceipt.cs(2 hunks)Dan.Core/Services/AltinnCorrespondenceService.cs(1 hunks)Dan.Core/Services/AuthorizationRequestValidatorService.cs(2 hunks)Dan.Core/Services/CachingEntityRegistryApiClientService.cs(4 hunks)Dan.Core/Services/ConsentService.cs(2 hunks)Dan.Core/Services/EntityRegistryService.cs(1 hunks)Dan.Core/Services/RequirementValidationService.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Dan.Core/Services/AuthorizationRequestValidatorService.cs
- Dan.Common/Models/EntityRegistryUnitHierarchy.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T09:05:48.800Z
Learnt from: SondreJDigdir
PR: data-altinn-no/core#196
File: Dan.Core/Services/AltinnCorrespondenceService.cs:57-65
Timestamp: 2025-10-29T09:05:48.800Z
Learning: The IEntityRegistryService interface has been split into two separate implementations: Dan.Core.Services.Interfaces.IEntityRegistryService (which only uses direct endpoints and does not have a UseCoreProxy property) and Dan.Common.Interfaces.IEntityRegistryService (which supports proxy routing with UseCoreProxy). When reviewing Dan.Core services, the absence of UseCoreProxy configuration is intentional and correct.
Applied to files:
Dan.Core/FuncConsentReceipt.csDan.Core/Services/CachingEntityRegistryApiClientService.csDan.Core/Services/AltinnCorrespondenceService.csDan.Core/Services/EntityRegistryService.csDan.Core/Services/ConsentService.csDan.Core/Services/RequirementValidationService.csDan.Common/Services/EntityRegistryService.cs
🧬 Code graph analysis (3)
Dan.Core/Services/CachingEntityRegistryApiClientService.cs (4)
Dan.Common/Models/EntityRegistryUnit.cs (4)
Organisasjonsform(162-172)Links(147-154)Link(156-160)EntityRegistryUnit(8-100)Dan.Common/Services/DefaultEntityRegistryApiClientService.cs (1)
Task(24-35)Dan.Core/Config/Settings.cs (1)
Settings(11-400)Dan.Common/Models/BrregPage.cs (2)
BrregPage(8-27)Subunits(32-39)
Dan.Core/Services/EntityRegistryService.cs (5)
Dan.Core/Config/Settings.cs (1)
Settings(11-400)Dan.Common/Models/EntityRegistryUnit.cs (2)
EntityRegistryUnit(8-100)Organisasjonsform(162-172)Dan.Common/Services/EntityRegistryService.cs (2)
SimpleEntityRegistryUnit(231-250)IsSyntheticOrganizationNumber(200-203)Dan.Common/Models/SimpleEntityRegistryUnit.cs (1)
SimpleEntityRegistryUnit(4-13)Dan.Common/Services/CcrClientService.cs (1)
IsSyntheticOrganizationNumber(186-189)
Dan.Common/Services/EntityRegistryService.cs (4)
Dan.Common/Services/DefaultEntityRegistryApiClientService.cs (1)
Obsolete(8-36)Dan.Core/Services/EntityRegistryService.cs (14)
EntityRegistryService(8-294)Task(43-44)Task(50-51)Task(56-104)Task(109-123)Task(133-170)Task(175-175)Task(180-184)Task(202-221)Task(223-226)Task(228-231)Uri(266-273)Uri(275-282)Uri(284-293)Dan.Core/Services/CachingEntityRegistryApiClientService.cs (4)
Task(23-56)Task(58-110)Task(117-136)Task(138-158)Dan.Common/Interfaces/IEntityRegistryService.cs (2)
Task(27-27)Task(34-34)
🪛 GitHub Actions: Build and test a PR
Dan.Core/Services/EntityRegistryService.cs
[error] 199-199: TestSyntheticLookupsNotAllowedByDefault failed due to NullReferenceException in IsSyntheticOrganizationNumber. Investigate possible null organizationNumber or uninitialized state leading to a null reference.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (11)
Dan.Common/GlobalUsings.cs (1)
9-9: LGTM! Global using for System.Net.Http added.This addition supports the new CCR client service infrastructure that uses HttpClientFactory.
Dan.Core/Services/RequirementValidationService.cs (1)
20-20: Interface migration to Interfaces.IEntityRegistryService is correct.The change from
IEntityRegistryServicetoInterfaces.IEntityRegistryServicealigns with the architectural split where Dan.Core services use direct endpoints only. The removal ofUseCoreProxyandAllowTestCcrLookupinitialization is intentional and correct for this interface.Based on learnings
Also applies to: 34-40
Dan.Core/Services/CachingEntityRegistryApiClientService.cs (3)
11-21: Well-structured refactor to distributed caching with keyed locking.The addition of
IDistributedCacheandAsyncKeyedLockerensures thread-safe caching and prevents duplicate concurrent requests for the same entity. The dependency injection via primary constructor is clean.
51-53: Good fix: differentiated TTL for null vs non-null results.Caching null results for only 5 minutes (vs 12 hours for valid results) prevents long-term masking of newly created units. This addresses the past review concern.
60-68: Safe regex extraction with match guard.The code now checks
match.Successbefore accessingGroups[1], preventing potential exceptions. Returning an empty list when the pattern doesn't match is a reasonable fallback.
[approve_code_challenges]Dan.Common/Services/EntityRegistryService.cs (1)
10-12: Obsolete marking and primary constructor migration look good.The deprecation message correctly directs users to
Dan.Common.Services.CcrClientServiceas the replacement.Dan.Core/Services/EntityRegistryService.cs (2)
8-16: Constructor and default AllowTestCcrLookup configuration look good.The primary constructor pattern is clean, and defaulting
AllowTestCcrLookupto!Settings.IsProductionEnvironmentis reasonable for a non-production safety check.
247-261: Defensive initialization of IndustrialCodes is well-implemented.The null-coalescing assignment (
??= []) ensures the list is initialized before adding Naeringskode2 or Naeringskode3, preventing null reference exceptions. The comment acknowledges the defensive approach.Dan.Core/FuncConsentReceipt.cs (1)
25-25: Interface migration is consistent with architectural changes.The update to
Services.Interfaces.IEntityRegistryServiceand removal ofUseCoreProxyinitialization aligns with the Dan.Core interface split. Dan.Core services use direct endpoints only.Based on learnings
Also applies to: 39-45
Dan.Core/Services/ConsentService.cs (1)
36-36: Interface migration completed correctly.The update to
Interfaces.IEntityRegistryServiceremoves the need forUseCoreProxyandAllowTestCcrLookupinitialization. Per learnings, the Dan.Core interface only uses direct endpoints.Based on learnings
Note: A past review flagged potential singleton state bleed if
AllowTestCcrLookupis mutated across multiple locations. Verify the DI lifetime and consider immutable configuration via constructor options.Also applies to: 50-64
Dan.Core/Services/AltinnCorrespondenceService.cs (1)
55-55: Interface update aligns with architectural split.The migration to
Interfaces.IEntityRegistryServiceand removal ofUseCoreProxy/AllowTestCcrLookupinitialization is intentional. The Dan.Core interface implementation uses direct endpoints only, making the proxy configuration unnecessary.Based on learnings
Also applies to: 57-62
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Dan.Core/Services/EntityRegistryService.cs (1)
92-101: Preserve last known parent if an upstream parent fetch failsIf InternalGet returns null once, you overwrite parentUnit and end up returning null even though a valid subunit was found. Keep the last successful parent.
- var parentUnit = unit; - do - { - // Only subunits are at the leaf node, any nested parents are MainUnits - // Example: https://data.brreg.no/enhetsregisteret/api/underenheter/879587662 - parentUnit = await InternalGet(parentUnit.OverordnetEnhet!, UnitType.MainUnit); - } - while (parentUnit?.OverordnetEnhet != null); - unit = parentUnit; + var parentUnit = unit; + var parentNumber = parentUnit.OverordnetEnhet; + while (!string.IsNullOrEmpty(parentNumber)) + { + var next = await InternalGet(parentNumber, UnitType.MainUnit); + if (next is null) break; // upstream inconsistency; keep last known + parentUnit = next; + parentNumber = next.OverordnetEnhet; + } + unit = parentUnit;
🧹 Nitpick comments (7)
Dan.Core/Services/EntityRegistryService.cs (3)
29-33: Static dictionaries can grow unboundedEntries never get removed; expired ones persist and keys accumulate. Prefer IMemoryCache with TTL/size limits, or actively evict expired entries on read.
- Option A: replace with IMemoryCache, set absolute expiration 10m and a size limit.
- Option B: on cache hit with expired entry, remove with TryRemove to bound growth.
- Option C: wrap access behind a small cache helper to dedupe in-flight fetches (Lazy<Task>).
139-170: Add cycle guard and cancellation to recursive hierarchyDefensive: guard against accidental cycles and accept a CancellationToken to abort deep trees; current default depth 10 helps but CT is preferable.
286-299: Make query URL construction robust and encodedCurrent string.Format + “? …” works after the Replace, but it’s brittle and unencoded. Use UriBuilder and EscapeDataString; don’t rely on formatting placeholders.
- if(urlPattern.EndsWith("/{0}")) - { - urlPattern = urlPattern.Replace("/{0}", "{0}"); - } - var query = $"?overordnetEnhet={organizationNumber}"; - - return new Uri(string.Format(urlPattern, query)); + var baseUrl = urlPattern.Replace("/{0}", string.Empty).Replace("{0}", string.Empty); + var builder = new UriBuilder(baseUrl) + { + Query = $"overordnetEnhet={Uri.EscapeDataString(organizationNumber)}" + }; + return builder.Uri;Dan.Core.UnitTest/Services/EntityRegistryServiceTest.cs (4)
26-58: Tighten URL matching in mocksContains(...) can misroute (e.g., “/enheter/91” also matches “/enheter/910”). Prefer EndsWith with Ordinal.
- if (url.AbsolutePath.Contains("/enheter/91")) + if (url.AbsolutePath.EndsWith("/enheter/91", StringComparison.Ordinal)) { return Task.FromResult(new EntityRegistryUnit { Organisasjonsnummer = "91", Organisasjonsform = orgForm })!; } - if (url.AbsolutePath.Contains("/underenheter/92")) + if (url.AbsolutePath.EndsWith("/underenheter/92", StringComparison.Ordinal)) { return Task.FromResult(new EntityRegistryUnit { Organisasjonsnummer = "92", OverordnetEnhet = "91", Organisasjonsform = orgForm })!; } - if (url.AbsolutePath.Contains("/enheter/93")) + if (url.AbsolutePath.EndsWith("/enheter/93", StringComparison.Ordinal)) { return Task.FromResult(new EntityRegistryUnit { Organisasjonsnummer = "93", OverordnetEnhet = "91", Organisasjonsform = orgForm })!; } - if (url.AbsolutePath.Contains("/underenheter/94")) + if (url.AbsolutePath.EndsWith("/underenheter/94", StringComparison.Ordinal)) { return Task.FromResult(new EntityRegistryUnit { Organisasjonsnummer = "94", OverordnetEnhet = "93", Organisasjonsform = orgForm })!; } - if (url.AbsolutePath.Contains("/underenheter/31")) + if (url.AbsolutePath.EndsWith("/underenheter/31", StringComparison.Ordinal)) { return Task.FromResult(new EntityRegistryUnit { Organisasjonsnummer = "31", Organisasjonsform = orgForm })!; }
61-131: Use async/await in tests to avoid .Result deadlocksModernize tests to async Task and await service calls. Repeat this pattern across test methods.
- public void TestGetMapping() + public async Task TestGetMapping() { var s = new EntityRegistryService(entityRegistryApiClientServiceMock.Object, logger.Object); - var r = s.Get("91").Result; + var r = await s.Get("91"); Assert.AreEqual("91", r?.OrganizationNumber); }
14-21: Test instance reuseYou already construct entityRegistryService in the fixture; consider reusing it in all tests for consistency unless isolation is required.
133-164: Add coverage for subunit listing and hierarchyConsider tests for GetSubunits and GetSubunitHierarchy (with maxDepth/cycle guard) to validate the new endpoint/query path end-to-end. I can draft test cases if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dan.Core.UnitTest/Services/EntityRegistryServiceTest.cs(1 hunks)Dan.Core/Services/EntityRegistryService.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T09:05:48.800Z
Learnt from: SondreJDigdir
PR: data-altinn-no/core#196
File: Dan.Core/Services/AltinnCorrespondenceService.cs:57-65
Timestamp: 2025-10-29T09:05:48.800Z
Learning: The IEntityRegistryService interface has been split into two separate implementations: Dan.Core.Services.Interfaces.IEntityRegistryService (which only uses direct endpoints and does not have a UseCoreProxy property) and Dan.Common.Interfaces.IEntityRegistryService (which supports proxy routing with UseCoreProxy). When reviewing Dan.Core services, the absence of UseCoreProxy configuration is intentional and correct.
Applied to files:
Dan.Core.UnitTest/Services/EntityRegistryServiceTest.csDan.Core/Services/EntityRegistryService.cs
🧬 Code graph analysis (2)
Dan.Core.UnitTest/Services/EntityRegistryServiceTest.cs (2)
Dan.Core/Services/EntityRegistryService.cs (15)
EntityRegistryService(8-300)Uri(268-275)Uri(277-284)Uri(286-299)Task(43-44)Task(50-51)Task(56-104)Task(109-123)Task(133-170)Task(175-175)Task(180-184)Task(204-223)Task(225-228)Task(230-233)IsPublicAgency(189-195)Dan.Common/Models/EntityRegistryUnit.cs (4)
Organisasjonsform(162-172)EntityRegistryUnit(8-100)SektorKodeDto(138-145)NaeringsKodeDto(126-136)
Dan.Core/Services/EntityRegistryService.cs (4)
Dan.Core/Config/Settings.cs (1)
Settings(11-400)Dan.Common/Models/EntityRegistryUnit.cs (2)
EntityRegistryUnit(8-100)Organisasjonsform(162-172)Dan.Common/Services/EntityRegistryService.cs (4)
SimpleEntityRegistryUnit(231-250)IsSyntheticOrganizationNumber(200-203)Uri(252-265)Uri(267-280)Dan.Common/Models/SimpleEntityRegistryUnit.cs (1)
SimpleEntityRegistryUnit(4-13)
🔇 Additional comments (3)
Dan.Core/Services/EntityRegistryService.cs (3)
8-11: DI and interface split looks correctUsing Dan.Core.Services.Interfaces.IEntityRegistryService without UseCoreProxy is expected for Core. Good alignment with the split. Based on learnings
197-202: Null-safe and efficient synthetic checkUsing StartsWith(char) and treating null/empty as synthetic avoids NREs and is efficient. LGTM.
249-263: No action required—C# 12 collection expressions are supportedThe repository targets .NET 8 (net8.0) across all projects, which defaults to C# 12 language features. Collection expressions (
[]and??= []) will compile correctly on all agents without modification.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Dan.Core/Services/EntityRegistryService.cs (4)
12-15: Doc now matches behavior“Defaults to true outside Production” is clear and correct. Thanks for fixing.
196-201: Null‑safe synthetic org check — resolvedThe added IsNullOrEmpty guard fixes the previous NRE. LGTM.
248-263: Defensive IndustrialCodes mapping — goodNull‑coalescing initialization avoids NREs when only Naeringskode2/3 exist.
91-101: Parent climb uses null‑forgiving — acceptable per business rulesGiven the control flow guarantees a subunit at entry, dereferencing OverordnetEnhet is safe here.
🧹 Nitpick comments (5)
Dan.Core/Services/EntityRegistryService.cs (5)
55-63: Trim and short‑circuit empty org numbers before any lookupsAvoid building “/enheter/” URLs and polluted cache keys in non‑prod. Normalize once up front.
public async Task<EntityRegistryUnit?> GetFull(string organizationNumber, bool attemptSubUnitLookupIfNotFound = true, bool nestToAndReturnMainUnit = false, bool subUnitOnly = false) { - - if (IsSyntheticOrganizationNumber(organizationNumber) && !AllowTestCcrLookup) + if (string.IsNullOrWhiteSpace(organizationNumber)) + { + return null; + } + organizationNumber = organizationNumber.Trim(); + if (IsSyntheticOrganizationNumber(organizationNumber) && !AllowTestCcrLookup) { return null; }
108-122: Guard and normalize input in GetSubunits as wellSame concern as GetFull; also prevents cache entries like “SubunitList_ ”.
public async Task<List<EntityRegistryUnit>> GetSubunits(string organizationNumber) { + if (string.IsNullOrWhiteSpace(organizationNumber)) + { + return new List<EntityRegistryUnit>(capacity: 0); + } + organizationNumber = organizationNumber.Trim(); var cacheKey = "SubunitList_" + organizationNumber; if (SubunitListCache.TryGetValue(cacheKey, out var cacheEntry) && cacheEntry.expiresAt > DateTime.UtcNow) { return cacheEntry.list; }
111-114: Evict expired cache entries to avoid unbounded growthDictionaries keep expired keys forever. Remove them when encountered.
- if (SubunitListCache.TryGetValue(cacheKey, out var cacheEntry) && cacheEntry.expiresAt > DateTime.UtcNow) - { - return cacheEntry.list; - } + if (SubunitListCache.TryGetValue(cacheKey, out var cacheEntry)) + { + if (cacheEntry.expiresAt > DateTime.UtcNow) return cacheEntry.list; + SubunitListCache.TryRemove(cacheKey, out _); + }- if (EntityRegistryUnitsCache.TryGetValue(cacheKey, out var cacheEntry) && cacheEntry.expiresAt > DateTime.UtcNow) - { - return cacheEntry.unit; - } + if (EntityRegistryUnitsCache.TryGetValue(cacheKey, out var cacheEntry)) + { + if (cacheEntry.expiresAt > DateTime.UtcNow) return cacheEntry.unit; + EntityRegistryUnitsCache.TryRemove(cacheKey, out _); + }Also applies to: 206-209
147-159: Avoid fetching subunits when depth limit reachedMove the network call inside the depth check.
- var subunits = await GetSubunits(orgNumber); var subunitHierarchies = new List<EntityRegistryUnitHierarchy>(); if (currentDepth < maxDepth) { - foreach (var subunit in subunits) + var subunits = await GetSubunits(orgNumber); + foreach (var subunit in subunits) { var subunitHierarchy = await GetSubunitHierarchy(subunit.Organisasjonsnummer, currentDepth, maxDepth, subunit); if (subunitHierarchy != null) { subunitHierarchies.Add(subunitHierarchy); } } }
285-298: Query URL fix looks good; consider UriBuilder for robustnessReplacing “/{0}” and formatting with “?overordnetEnhet=…” works. Using UriBuilder avoids string surgery and handles encoding reliably.
- if(urlPattern.EndsWith("/{0}")) - { - urlPattern = urlPattern.Replace("/{0}", "{0}"); - } - var query = $"?overordnetEnhet={organizationNumber}"; - - return new Uri(string.Format(urlPattern, query)); + var baseUrl = urlPattern.Replace("/{0}", string.Empty).Replace("{0}", string.Empty); + var ub = new UriBuilder(baseUrl) { Query = $"overordnetEnhet={organizationNumber}" }; + return ub.Uri;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dan.Core/Services/EntityRegistryService.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T09:05:48.800Z
Learnt from: SondreJDigdir
PR: data-altinn-no/core#196
File: Dan.Core/Services/AltinnCorrespondenceService.cs:57-65
Timestamp: 2025-10-29T09:05:48.800Z
Learning: The IEntityRegistryService interface has been split into two separate implementations: Dan.Core.Services.Interfaces.IEntityRegistryService (which only uses direct endpoints and does not have a UseCoreProxy property) and Dan.Common.Interfaces.IEntityRegistryService (which supports proxy routing with UseCoreProxy). When reviewing Dan.Core services, the absence of UseCoreProxy configuration is intentional and correct.
Applied to files:
Dan.Core/Services/EntityRegistryService.cs
🧬 Code graph analysis (1)
Dan.Core/Services/EntityRegistryService.cs (7)
Dan.Core/Config/Settings.cs (1)
Settings(11-400)Dan.Common/Models/EntityRegistryUnit.cs (2)
EntityRegistryUnit(8-100)Organisasjonsform(162-172)Dan.Common/Models/SimpleEntityRegistryUnit.cs (1)
SimpleEntityRegistryUnit(4-13)Dan.Common/Services/CcrClientService.cs (1)
IsSyntheticOrganizationNumber(186-189)Dan.Common/Models/EntityRegistryUnitHierarchy.cs (1)
EntityRegistryUnitHierarchy(6-22)Dan.Common/Models/BrregPage.cs (1)
Subunits(32-39)Dan.Common/Interfaces/IEntityRegistryService.cs (2)
IsPublicAgency(97-97)IsPublicAgency(104-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
Dan.Core/Services/EntityRegistryService.cs (1)
22-27: No action required — C# 12 collection expressions are properly supportedThe codebase targets
net8.0, which implicitly enables C# 12 and its collection expressions. No explicit LangVersion configuration is needed; the<TargetFramework>net8.0</TargetFramework>setting in Dan.Core.csproj automatically provides C# 12 support. The collection expressions used at lines 22-27 and 255-261 will compile successfully.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dan.Core/Services/EntityRegistryService.cs (1)
136-173: Safe recursive hierarchy traversal with depth limit.The
maxDepthparameter (defaulting to 10) prevents runaway recursion. The depth tracking at lines 142-143 and the guard at line 153 ensure safe termination. Consider adding logging when hitting max depth for operational visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dan.Core/Services/EntityRegistryService.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T09:05:48.800Z
Learnt from: SondreJDigdir
PR: data-altinn-no/core#196
File: Dan.Core/Services/AltinnCorrespondenceService.cs:57-65
Timestamp: 2025-10-29T09:05:48.800Z
Learning: The IEntityRegistryService interface has been split into two separate implementations: Dan.Core.Services.Interfaces.IEntityRegistryService (which only uses direct endpoints and does not have a UseCoreProxy property) and Dan.Common.Interfaces.IEntityRegistryService (which supports proxy routing with UseCoreProxy). When reviewing Dan.Core services, the absence of UseCoreProxy configuration is intentional and correct.
Applied to files:
Dan.Core/Services/EntityRegistryService.cs
🧬 Code graph analysis (1)
Dan.Core/Services/EntityRegistryService.cs (3)
Dan.Core/Config/Settings.cs (1)
Settings(11-400)Dan.Core/Services/CachingEntityRegistryApiClientService.cs (2)
Task(23-56)Task(58-110)Dan.Common/Models/EntityRegistryUnitHierarchy.cs (1)
EntityRegistryUnitHierarchy(6-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
Dan.Core/Services/EntityRegistryService.cs (5)
8-31: Clean constructor and configuration.The primary constructor syntax is concise and the
AllowTestCcrLookupdefault correctly restricts synthetic lookups to non-production environments. Caching strategy withConcurrentDictionaryand TTL is appropriate for this registry access pattern.
58-66: Robust input validation.The whitespace guard and trim at lines 58-62 prevent malformed URLs from empty or whitespace-only inputs, and the synthetic organization number check correctly enforces production restrictions.
200-205: Null-safe synthetic organization number check.The
string.IsNullOrEmptyguard prevents theNullReferenceExceptionflagged in previous reviews whenStartsWithis called on a null reference. The method now safely handles all inputs.
253-266: Null-safe industrial codes initialization.The
??= []pattern at lines 259 and 264 correctly addresses the previous NRE concern whenNaeringskode2orNaeringskode3are present butNaeringskode1is null. Each code is safely added to the list.
289-302: Query parameter construction now correct.The logic at lines 295-298 properly strips the path placeholder before appending the query string, producing well-formed URLs like
/underenheter?overordnetEnhet=...instead of the previously malformed/underenheter/?overordnetEnhet=....
2 participants
Description
Need to be able to call ccr without identifier and with query parameters. This splits out the ccr lookup from the opendata endpoint
Documentation
Summary by CodeRabbit
New Features
Deprecations
Chores