chore(#3793): resolve connection roles from RoleConstants - #3903
Draft
Thuen wants to merge 1 commit into
Draft
Conversation
ConnectionEntityEnricher loaded all 148 roles, joined to provider and provider type, on every enrichment call. Roles are seeded from RoleConstants and never written at runtime, so the table is only a projection of the constants and the round-trip buys nothing. The constants hold foreign keys rather than navigations, so the lookup rebuilds the role -> provider -> provider type graph the query used to produce with its includes. That graph reaches the API: RoleDto carries Provider and ProviderDto carries Type. It is rebuilt into its own structure rather than assigned onto the constants' entities, because those instances are the seeds StaticDataIngest hands to EF, and a populated reference navigation on a seed makes DbSet.Add cascade an insert of the referenced provider. StaticDataIngest never deletes, so a role dropped from the constants in an earlier release can still exist in the database and be referenced by an assignment made while it was current. Unknown ids therefore fall back to a query for just those rows, instead of failing the whole read on one stale row, and log a warning naming them because reaching that path means the constants and the table have drifted. The normal path returns the shared lookup with no query and no allocation. ConnectionQuery takes an ILogger to pass on to the enricher, following TranslationService, which is the existing logging precedent in this project. Its two manual construction sites are updated accordingly. Removes the TODO this issue was filed from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Thuen
force-pushed
the
chore/3793-roleconstants-enricher
branch
from
August 17, 2026 11:00
d7c2a74 to
a9ee77a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ConnectionEntityEnricher.FetchRolesAsyncloaded all 148 roles, joined to provider and provider type, on every enrichment call — which is essentially every enduser connection query. Roles are seeded fromRoleConstantsand nothing writes the table at runtime, so it is only a projection of the constants and the round-trip buys nothing. The method already carried a TODO pointing at this issue; it is now gone along with the method.One thing the issue got wrong
The issue states that
RoleConstantsincludes "theirProviderandProvider.Typerelationships". It does not — the constants only hold foreign keys:A straight swap to
TryGetByIdwould therefore have shippedRole.Provider == null, and that graph reaches the API:RoleDtocarriesProvider, andProviderDtocarriesType.ConnectionsControllerTest+GetRolesalready assertsr.Role.Provider?.Code == "sys-altinn2", so it would have caught it — but only after the fact. The lookup therefore rebuilds the same graph the query produced with its includes, fromRoleConstants,ProviderConstantsandProviderTypeConstants.Role.EntityTypeis deliberately left unset, because the previous query did not include it either.Why the graph is rebuilt rather than assigned onto the constants
Setting
Provideron the constants' ownEntityinstances would be the obvious shortcut, but those instances are exactly whatStaticDataIngesthands to EF as seeds.DbSet.Addtracks the entity and every reachable untracked related entity asAdded, so a populated navigation on a seed makes adding a new role cascade an insert of its provider.That failure would have been invisible in CI: on a fresh database the provider constants are themselves tracked from their own ingest pass, so nothing happens. On an existing database the tracked instances are the ones loaded from the table, the constants stay untracked, and the first newly added role would hit a PK violation on
providerat startup. Keeping the seeds free of navigations avoids the whole question.Fallback for unknown ids
ApplyEnrichmentlooks roles up with the dictionary indexer. Previously the dictionary came from the database, so any role an assignment referenced was present by construction. Resolving from constants opens a gap:StaticDataIngestnever deletes, so a role dropped fromRoleConstantsin an earlier release can still exist in the table and be referenced by an assignment made while it was current.ResolveRolesAsynccollects theRoleIdandViaRoleIdvalues the constants do not cover, logs a warning naming them, and queries for just those rows with the same includes. When nothing is missing — the normal path — it returns the shared lookup with no query, no allocation and no logging. An id absent from both constants and database still throws, which is correct for a genuinely dangling FK.Reaching the fallback means the constants and the table have drifted, which is worth an alert, hence
LogWarning.ConnectionQuerynow takes anILoggerto pass on to the enricher, followingTranslationService— constructor-injectedILogger<T>with structured PascalCase placeholders — which is the existing logging precedent in this project. Its two manual construction sites are updated: the FFB tool injects the logger, andConnectionQueryTestspassesNullLogger<ConnectionQuery>.Instanceas the otherTranslationServicetests already do.Related Issue(s)
Verification
Run against a real Postgres:
ConnectionsControllerTest+GetRoles: 10/10 — includes theProvider?.Code == "sys-altinn2"assertion, which is the proof the provider graph survives the refactor.ConnectionsControllerTest+GetRolesRoleFallback: 1/1 — new. Writes a role that exists only in the database, assigns it, and asserts the endpoint returns it with name, code,Provider.CodeandProvider.Typepopulated. Uses its ownApiFixturerather than the shared read-only collection because it writes. Verified non-vacuous: disabling the fallback makes it fail withKeyNotFoundExceptionon that role id, surfacing as a 500.Altinn.AccessMgmt.PersistenceEF.Tests: 47/47 — includes 2 new tests asserting every role resolves its provider and every provider resolves its type, so a constant pointing at a non-constant cannot silently produce a nullProvider. Verified non-vacuous by pointing one role at a random Guid.127.0.0.1:10000, the Azurite blob emulator used byPolicyRepository, which was not running locally. Baseline on a clean tree was 32 failures from the same cause.Two verification gaps worth naming:
LogWarningitself is not asserted. It sits unconditionally on the fallback path, and the test above proves that path is taken, so the call does execute — butApiFixturesetsLogging:LogLevel:* = Error, so Warning is filtered out of integration-test output and cannot be observed without changing the fixture for all tests.Altinn.AccessManagement.Testscannot be built locally, because its test-data file paths exceed Windows MAX_PATH under this worktree. The one-line change toConnectionQueryTestswas checked by confirming the build produces 142MSB3030file-copy errors and zeroCSerrors, so the C# compiles; runningConnectionQueryTestsis left to CI.Documentation
🤖 Generated with Claude Code