Repository navigation
Close the API gaps: write observers, discovery endpoints, projectors - #7
Merged
Merged
Conversation
GetAsync cached the mapped model, so every caller of For<T>().GetAsync()
received the same mutable object. Assigning to any property of it wrote
into the entry every later reader of that group was served from —
including GET /api/settings/{group}, which then returned the mutated
value as though it were stored. SetAsync(selector, value) was mutating
the cached instance for the same reason.
Cache the stored rows and map a fresh model on each read. Skipping the
round trip to the store is what the cache was ever worth; MapToModel is
reflection over a handful of properties.
Two consequences worth knowing, both documented in docs/caching.md: a
[Sensitive] value is decrypted per read rather than once per cache
entry, and a custom ISettingCache is handed Setting[] — so what lands
in Redis is the stored ciphertext rather than a decrypted model.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Serving a settings group as anything other than its stored values — a
display name in the request's language, an image id resolved to a URL —
meant standing up a parallel endpoint and leaving the generated one
returning a value the UI never shows. Settings does not need to know
what those other components are; it needs a hook.
ISettingProjector<T>, registered with .UseProjector<T, TProjector>(),
is applied by MapSettingsEndpoints on GET. Three constraints make it
safe to have, and each has a test:
- read path only, so POST never consults it and a projector cannot
influence what is stored;
- never applied to the ETag, which keeps describing the stored
revision, so If-Match goes on protecting the stored values;
- no projector registered is the previous response, byte for byte.
Also tag the generated routes "Settings", as the sibling packages do.
app.MapGroup("").WithTags("Settings").MapSettingsEndpoints() — an empty
prefix whose only purpose was to carry a tag — becomes
app.MapSettingsEndpoints().
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A consuming AppDbContext implemented ISettingDbContext and declared two DbSet properties the library mostly never read, then restated in OnModelCreating an engine the context already knows. EfCoreSettingStore and EfCoreAuditStore now take a plain DbContext and reach their entities through Set<Setting>() / Set<SettingAuditEntry>(), and UseEntityFrameworkCore<TContext> constrains on nothing but DbContext. It also registers TContext as the scoped DbContext, which is what lets .UseAuditStore<EfCoreAuditStore>() go on resolving. ISettingDbContext is obsolete and does nothing; it goes in the next release. ApplySettingsConfiguration(DbContext) reads the engine off the context, so OnModelCreating is one call and switching provider needs no matching edit. The explicit overload stays for overriding that — a model built against one engine and migrated onto another. An unrecognised provider throws naming the overload to use, rather than guessing a column type the engine cannot hold and surfacing it later as truncated settings. The whole settings registration a context needs is now that one call, which is what the test and sample contexts demonstrate. The PostgreSQL and SQL Server integration contexts use the inferring overload, so both provider names are mapped against a live server rather than only in a unit test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Plain single-space declarations, matching how the settings classes are written in samples/SampleApp.
The brief the preceding four commits work from: a review of dotnetboost-sample listing what composing Settings, Media and MultiLingual in application code turned up. Part 1 of 4, covering the items this repo owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The audit trail could not be frozen into 1.0. ChangedBy was the literal "system" on every entry and never anything else, over a column required in all four schemas. ClearAsync deleted every row in a group and recorded nothing, so the most destructive operation in the library was the only one with no trail. Entries were written one round trip at a time after the write had committed, each with its own SaveChangesAsync, outside the write's transaction — and the loop had no try/catch, so an audit store failure threw on a write that had succeeded, thirty lines from change handlers whose exceptions are deliberately swallowed. GetHistoryAsync was unbounded over a table that only grows. And it existed only for EF Core: the Dapper bootstrap created a SettingAudits table nothing ever wrote to, so a Dapper user got a table, concluded auditing worked, and got silence. Remove it, and leave the port behind: ISettingWriteObserver is called once per completed write with the group, the per-property diff, the actor, a correlation id and a timestamp. One notification per write, so the several changes of one save are identifiable as one edit. ClearAsync reports too, which is why SettingWriteKind exists. Exceptions are logged and swallowed, matching ISettingChangedHandler<T> — the two are now documented as a pair, with the division between typed-per-group and untyped-every-group spelled out. [Sensitive] properties arrive redacted with both values null. The library decides what is sensitive and an observer only learns that it changed; an observer is precisely what forwards writes elsewhere, so it should not be where a secret escapes. Redaction follows the attribute, not whether a value was encrypted at rest. Actor is nullable and null means not captured, never a placeholder — that is what made ChangedBy worthless. ISettingActorAccessor supplies it; the API package ships one reading the signed-in user. LoggingSettingWriteObserver ships so the port is not empty: one line per write naming the changed properties, never their values. Nothing is registered against DbContext any more either. That existed only so EfCoreAuditStore could resolve, and a library should not claim a framework type it does not own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The call rebuilt the whole model from the cache and put it through the group write, which compares every property against fresh store rows and writes whatever differs. So a property another instance had changed while this one's cache was warm read back stale, counted as a difference, and was written back — a call that meant to touch property A reverted someone's edit to property B, over a window as wide as CacheDuration rather than a round trip. Build and write a single row instead. Properties the caller did not name are not part of the write, so there is nothing to revert. The per-property row construction is now shared with the group write rather than duplicated, so the encryption and change-detection rules cannot drift apart between the two paths. Validators still see the whole group — they take a model, not a property — built from a fresh read, so untouched properties carry their stored values rather than a blank instance's defaults. Skipping that would have made the selector overload a way around every registered validator. The XML doc understated the problem twice: it said "reads the current group" when the read was cached, and framed the damage as landing on the property being written. Corrected. The regression test gives the two managers separate caches, which is what two replicas have and the only arrangement where the bug is reachable — one shared cache would let the other writer's eviction clear the stale copy before it could do harm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A write evicted the local IMemoryCache entry and nothing else, so every other instance went on serving the old values until its own entry expired — up to CacheDuration, ten minutes by default. "Change it, reload, done, no redeploy" is this library's central claim and it silently stopped being true at two replicas, in a repo that ships an Aspire AppHost. ISettingChangeSignal mirrors ILanguageChangeSignal in MultiLingual: a version token in a store every instance can reach, published on each write and compared by readers. Per group here rather than global, since a write to one group should not make every other group reload. Publishing is an ISettingWriteObserver rather than a second call site inside the manager, so ClearAsync propagates for free. The staleness bound is ChangeCheckInterval, 5 seconds by default, and the docs say so rather than implying propagation is instant — what this buys is a bound of seconds instead of minutes. An unreachable signalling store is logged and treated as unchanged: a signal is an optimisation over the cache duration, so a Redis outage should cost a slower reload, not an error. The default signals nothing, so a single-instance application needs no configuration and behaves exactly as before — the no-op always reports a null token, which always matches, so every cache hit is served as it was. Probes — the token a cached copy was loaded under, and when it was last checked — are keyed by the cache instance. That is both correct (a probe describes one cache's entry) and what lets a test model two instances honestly, with separate caches and a shared signal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing enumerated the [SettingGroup] classes and nothing described a
group's properties, so a "form generated from the class" was really
generated by guessing at the shape of a JSON response. The consuming
sample shows the cost: its dashboard hardcodes the route, the C# class
name and the numeric bounds — bounds that exist nowhere in C# and are
therefore enforced nowhere on the server.
GET /api/settings lists the groups with their route, persistence key and
whether they carry [Authorize]. GET /api/settings/{route}/schema
describes one group's properties: name as the GET body spells it, type,
default, and whether it is [Sensitive].
Neither returns a stored value, and several tests exist to keep it that
way. The default is withheld for a [Sensitive] property: a compile-time
default on a secret is a value like any other, and these endpoints are
meant to be safe to expose more widely than values are. The schema sits
inside its group so it inherits the group's [Authorize] — a group's
endpoints should not disagree about who may look at it.
Constraints are the valuable half for a generated form, and
ISettingValidator cannot supply them: it answers whether a value is
valid, never what would be. ISettingSchemaContributor fills that, and
the existing UseFluentValidation(assembly) call now registers an
implementation reading FluentValidation's own descriptor, so the schema
publishes the bounds your validators already enforce. A rule it cannot
read — a Must(...), a comparison against another property — is skipped
rather than guessed at: a wrong published bound is worse than a missing
one, because a client would believe it.
The rule for which properties the engine stores now lives in one place,
SettingSchema, shared with SettingManager. A schema that listed a
property a write ignores would be worse than no schema.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A projector that throws fails the request, deliberately unlike ISettingChangedHandler<T> and ISettingWriteObserver, whose exceptions are logged and swallowed: a failed notification must not break a write that has already committed, whereas a failed projection means the response would be wrong, and a wrong response must not be hidden. Behaviour unchanged — it was never written down. Covered by a test rather than only asserted in a comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Missed when the SQL Server twin was updated: the schema test still required the audit table the bootstrap no longer builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "EF Core store takes a plain DbContext" bullet was written before the audit trail was removed and never updated. It named EfCoreAuditStore and Set<SettingAuditEntry>(), both listed as deleted in the Removed section of the same release, and ended by stating that UseEntityFrameworkCore<TContext> registers TContext as the scoped DbContext so .UseAuditStore<EfCoreAuditStore>() keeps resolving — which the bullet directly above it says was removed. Drop those two claims. The bullet above is unchanged, and the audit types now appear in the Unreleased section only where its Removed entry announces their deletion. The [1.0.0-preview.1] section still describes the audit trail, which is correct history for a shipped release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The brief the preceding commits worked from. Every item it assigned to this repo has landed, so it is history rather than a plan; it stays readable at 8edd75a. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
First-match-wins was wrong, and the comment justifying it cited a precedent that does not transfer. First-match is right for ISettingValidator because running two validators over one property risks rejecting or reporting a value twice. Constraints are not a decision — they are additive facts about a property, and two contributors describing different aspects of one group is the normal case. It already blocked a consumer: publishing `translated` for properties carrying an attribute from another package would have replaced the FluentValidation contributor, so the group gained `translated` and lost min, max, pattern and required — with the survivor decided by registration order, which nothing documented or controlled. Merge two levels deep, property then constraint name. On a collision the first registered wins and the clash is logged at Warning naming both contributors, the group, the property and the constraint; taking either silently would leave the published schema dependent on registration order, which is the defect being fixed. Each contributor stays individually wrapped. With several registered that matters more, not less: one failing must not cost the others their constraints, and there is now a test for exactly that, since it is what silently regresses if the try/catch is ever hoisted out of the loop. The single-contributor case — every current caller, including UseFluentValidation — hands back the contributor's own dictionary with nothing merged and nothing allocated, and is not behaviourally changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ISettingSchemaContributor was the only port on SettingBuilder with no way to register it from the chain. Nobody hit the gap because DotNetBoost.Settings.FluentValidation registers its own internally — but a consumer publishing a constraint of its own, which is exactly what the port exists for, had to drop out of AddSettings() and call AddSingleton<ISettingSchemaContributor, T>() by hand. TryAddEnumerable with a type descriptor: several different contributor types all register, which is the point now that the schema merges them, while a repeat of one type is deduped. A contributor only describes, so a duplicate would describe the group twice and the merge would read the second pass as a collision with itself. Deliberately unlike UseWriteObserver, where two registrations are two observers because an observer does something. The factory overload uses plain Add: two factories are not comparable by implementation type, so deduping them would silently drop one. Registration order decides which value survives a collision, so the XML doc says so rather than leaving it to be discovered. Both registration choices are covered by tests that fail without them — plain Add breaks the dedup case, service-type TryAdd breaks the distinct-types and composes-with-FluentValidation cases. TestApp gained a SettingBuilder hook, so these run through the chain an application actually writes, and warning capture, so the duplicate test can assert a clash was *not* logged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
UseProjector used AddScoped, so a second registration for one group was accepted and the last silently won — the other was never constructed, and the response was quietly missing half its shape. That is never what anyone means. Projectors do not compose: ProjectAsync takes the group, so a second has nowhere to receive the first's output. A group needing both translations and media wants one projector doing the join, not two registrations. Build() now rejects it, beside the checks for duplicate group names and routes, naming the group and both implementations. Descriptors registered by factory or instance carry no ImplementationType and are counted individually rather than folded together — treating two unknowns as one would silently pass the case the guard most wants to catch. The same implementation type twice is collapsed rather than reported: an idempotent mistake, not an ambiguity, matching the mercy UseSchemaContributor already extends. ReplaceProjector<TSettings, TProjector> is the deliberate-override path the guard makes necessary, and the error message names it so the error says what to do instead. This is a guard, not a guarantee, and the XML doc says so: registrations added after Build() still win by last-wins, which keeps the test-override seam open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
.Produces(200, type) named the stored group, which is wrong for every projected group — and the library could not do better on its own: a projector is resolved from the request scope, long after the route was mapped. The docs admitted it in prose and told the reader to rewrite the document themselves, which dotnetboost-sample has been doing with an IOpenApiOperationTransformer. ISettingProjector<T, TProjection> names the type instead. It sits on the projector rather than as a third argument to UseProjector so the compiler checks it; a third generic argument would be a promise the author could get wrong, and catching that would need a runtime check. A default interface member adapts the typed member to the object-returning one the generated endpoint already calls through reflection, so implementing the typed interface is all an author does. An existing ISettingProjector<T> keeps compiling and keeps declaring the group. The declared type travels to the endpoint as a keyed singleton under the group type: one per group by construction, with no ordering rule to define. Every registration path writes the key, with the group as the value when the projector is untyped, so a later ReplaceProjector cannot leave a stale projection type behind. A projector declaring two projections for one group throws at the registration call, naming both, rather than surfacing as a LINQ failure. This documents the response; it does not change it. The body is byte for byte what the untyped interface produced — asserted, so the default interface member is proven to dispatch — and POST still accepts the group, because advertising the projection as a request body would promise a write the API refuses. Tests assert IProducesResponseTypeMetadata off Endpoint.Metadata rather than generating an OpenAPI document: AddOpenApi is .NET 9+ and this suite also runs on net8.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1904708 claimed that every registration path writes the projection declaration, "so a later ReplaceProjector cannot leave a stale projection type behind" — and nothing held it. Deleting the DeclareProjection call from ReplaceProjector left all 174 unit and 49 API tests green, which makes it exactly the line a refactor drops in silence. Two cases, one per direction: a typed projector replaced by an untyped one declares the group, and an untyped one replaced by a typed one declares the projection. Both fail without that call. Asserted on the endpoint's declared 200, which is what a consumer's OpenAPI document is built from. A test on the keyed descriptor would pass even if RegisterGet stopped reading it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Media exposes modelBuilder.ApplyMedia() and MultiLingual exposes modelBuilder.ApplyMultiLingual(this). Settings was the only one of the three naming the mechanism rather than the library, and in a consuming AppDbContext the three calls sit on adjacent lines, so the odd shape read as if it meant something. It did not. preview.2 is the last cheap moment to fix it. Additive, using the pattern ISettingDbContext already set: both overloads are renamed, and the old names stay as [Obsolete] one-line forwarders that go in the next release. An existing context keeps compiling with a deprecation warning — confirmed to be a warning rather than a break by building without this repo's warnings-as-errors. ResolveProvider's exception told the caller to use ApplySettingsConfiguration(DatabaseProvider), and the test asserted that literal, so both moved together; otherwise the test would have gone green on a string nothing throws. The forwarders are covered: a context on the old name builds the same model as one on the new name, for every provider and for the context-inferring overload. CS0618 is suppressed in that one region and nowhere else. CHANGELOG records the rename as a new entry. The existing entries describing what preview.2 shipped are left alone — editing them would make the history lie. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c2c1c84 left the Unreleased section introducing ApplySettingsConfiguration(DbContext) under Added and renaming it under Changed, held together by a "this supersedes the naming used in the Added entry" clause. Read as release notes, the release adds an API and renames it in the same breath. That followed from treating those entries as shipped history. They are not: [Unreleased] runs from line 6 to 220, and the only released sections are 1.0.0-preview.1 and 0.1.0. Nothing in Added had shipped under any name, so there was nothing to preserve and no rename to record — the entry can simply say ApplySettings(DbContext). What stays in Changed is the part that is a real change. ApplySettingsConfiguration(DatabaseProvider) is the only member 1.0.0-preview.1 published, confirmed against that package's XML docs, so it is the one an [Obsolete] forwarder exists for and the one the entry now talks about. Also updates the EF-store entry, which still described a consuming context's registration as an ApplySettingsConfiguration call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An [Obsolete] forwarder exists so code already calling the old name keeps compiling. No such code can exist for ApplySettingsConfiguration (ModelBuilder, DbContext): that overload has never been in a published package. It is new in this unreleased work, and the only place it ever existed under the old name was a locally packed preview.2 whose one consumer, dotnetboost-sample, is already on the new name. So the alias was a deprecation warning aimed at code that cannot exist, plus a commitment to a removal note for a method with no users. The (ModelBuilder, DatabaseProvider) forwarder is the opposite case and stays: it shipped on nuget.org in 1.0.0-preview.1, and a real consumer's OnModelCreating calls it. Its tests stay with it, as does the CS0618 suppression they need. CHANGELOG already described exactly one forwarder as of aed285f, so it is correct as written and left untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unit-test line coverage was 79.03%, under the 80 the CI coverage gate names in COVERAGE_THRESHOLD. The whole shortfall sat in one package: DotNetBoost.Settings.FluentValidation, at 67.5%. FluentSettingValidator<T> had no coverage at all. It is the adapter that makes a FluentValidation rule reach SetAsync and the REST API — Core knows only the untyped ISettingValidator — and the error shape it produces is what the API serialises into a 400 body, so it is contract surface rather than an implementation detail. Covered: CanValidate matching only its own type, a valid model reporting no errors, errors keyed by property name, two rules failing on one property grouping into one key with two messages, and the type-mismatch path returning a failure that names both types instead of throwing from inside FluentValidation. The schema contributor's tests exercised Length(min,max), InclusiveBetween, Matches, NotEmpty, GreaterThanOrEqualTo and LessThan, leaving four arms of Collect's switch unread. The new cases pin the distinctions a client would otherwise get wrong: an exact length is both bounds at once rather than falling through to the general length rule, a one-sided length rule leaves the other bound absent rather than publishing an invented floor, and LessThanOrEqualTo/GreaterThan stay separate from their inclusive counterparts — rendering exclusiveMax as max would let a client accept the boundary value the server refuses. AddFluentValidationSettings, the standalone counterpart to UseFluentValidation, is covered along with its null guards. Bridge() returns the concrete FluentSettingValidator<T> rather than ISettingValidator because CA1859 is an error here; the members it calls are the interface's either way. The package is now at 100% line coverage and the total is 81.95%. Branch coverage is 78.28% and still under 80, which matters only if the gate reads branches — worth confirming on the next run, since minimumCoverage may not be an input the ReportGenerator action recognises at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The README was updated for the quick start, the write observer and the discovery endpoints when those landed, but four statements were left behind by later commits in the same branch. The worst contradicted the quick start twelve lines above it: "EF Core needs two extra DbSets on your context", when 4b4cc73 removed both the marker interface and the DbSet requirement, and the audit entity one of those two DbSets pointed at no longer exists. storage-providers.md has said "no interface and no DbSet properties" since that commit; the README landing page still told a reader the opposite. Projectors had no README presence at all — not a table row, not a mention — though ISettingProjector<T> is one of this branch's larger additions and rest-api.md has a section on it. A reader looking for "can I change what GET returns" found nothing and had to open the REST API guide on spec. The caching paragraph said a write "clears the cache on that server" and sent multi-server readers to the caching guide, which is true but now understates the answer: SynchronizeThroughDistributedCache() is a single line and the guide's recommended path. Naming it in the one place a reader is already thinking about several instances seems worth two lines. The table row named Redis specifically, which is now the alternative rather than the recommendation. And MapSettingsEndpoints() maps the group list and schema by default (includeDiscovery: true), not just the group's GET and POST as its inline comment claimed. CHANGELOG: [Unreleased] held nothing but this branch's work — it is _Nothing yet._ on main — so it becomes [1.0.0-preview.2] with today's date, and a fresh empty [Unreleased] takes its place in the form this file already uses. The new section opens the way preview.1's does, naming what a reader upgrading needs to know first: exactly one break (the audit trail), and two deprecations that compile with a warning. Those are counted from the entries below rather than from memory — ISettingDbContext and ApplySettingsConfiguration are warnings, not breaks, which c2c1c84 and e8ef8d8 established deliberately. Every new public name in the branch appears somewhere in the section: checked ISettingChangeSignal, SynchronizeThroughDistributedCache, UseChangeSignal, ISettingProjector, UseProjector, ReplaceProjector, ISettingSchemaContributor, UseSchemaContributor, ISettingWriteObserver, UseWriteObserver, ISettingActorAccessor, ChangeCheckInterval, ApplySettings and includeDiscovery. Every docs link and anchor in the README resolves against the tree, including the two new ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing the audit trail left the EF model with a single entity —
ApplySettings configures exactly one ToTable("Settings") — but three
sentences still described two tables.
docs/storage-providers.md opened its EF Core section with "add the two
settings tables to your DbContext" and then, forty lines later, said the
migration "creates the `Settings` table". The README's quick start had
the same mismatch in a code comment it gained in this branch: 4b4cc73
rewrote that comment to drop the interface and the DbSet properties and
kept the word "two". A third, nearby, said the bootstrap creates "the
settings tables".
The Dapper and MongoDB sections were already right, and the one
remaining mention of two DbSet properties stays: it is the preview.1
upgrade note, and a reader upgrading really does have two to delete.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The job passed minimumCoverage to ReportGenerator-GitHub-Action, which has no such input. GitHub logged Unexpected input(s) 'minimumCoverage', valid inputs are ['reports', 'targetdir', 'reporttypes', ...] and ignored it, so for the whole life of the workflow this job generated an HTML report, uploaded it and exited 0 no matter what the coverage was. COVERAGE_THRESHOLD: 80 was decorative. The ruleset on main lists "Coverage Gate" as a required check, which made it worse than having no check at all: coverage could fall to single digits and every PR would still show a green tick next to the word "Gate". The report step keeps its job and gains a Cobertura output, which the new step reads. A merged report is the point: the unit tests run on net8.0 and net10.0, so the artifact holds two coverage files describing the same source. Summing them double-counts and either one alone misses what only the other compiles — net10.0 by itself reads 81.95% line, while the merge of both is 81.55% over 927 coverable lines. Line coverage only, at the existing 80. Branch coverage is 75.73% and gating on it today would fail the build over code this change is not about; it is printed and written to the step summary so the number stays in view, and raising it is a deliberate follow-up. The three ways this step could wrongly pass are all closed, since wrongly passing is the bug being fixed: a missing merged report, a missing line-rate attribute and an unparseable one each fail the job with an ::error:: rather than defaulting to zero or to "fine". Verified by running the step's own script against the real merged report — 80 passes at 81.55%, 95 fails, a deleted report fails, and a line-rate of "oops" fails. The artifact upload moves to if: always() so a build that fails on coverage still carries the line-by-line report, which is exactly when someone wants it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MapToModel throws SettingDecryptionException on purpose, and says why at
the throw site: falling through "would leave the property on its default
value, so a rotated-away key would surface as an app running on default
credentials rather than as an error."
Both write paths then caught it and did exactly that. Three bare
catches — one in the group overload, two in the single-property one —
read `catch { previous = new T(); }`, so a stranded value produced a
before-image of compile-time defaults. Every [Sensitive] property
compared as changed, which meant a write to one unrelated property
rewrote all of them under the current key, told ISettingWriteObserver
they had changed, and discarded the values nobody could read. The
IgnoreDecryptionFailures() opt-out was bypassed, since the catch did not
consult it. And unlike the observer catch at the end of the same file,
these logged nothing at all: the only symptom was a secret quietly
becoming "DEFAULT-DO-NOT-USE".
The read path was always right — GetAsync calls MapToModel uncaught —
so this was the write path disagreeing with the read path about the same
value.
Narrowed rather than removed: an exception filter lets
SettingDecryptionException through while other mapping faults still fall
back to defaults, so a write cannot be blocked by a conversion quirk,
but they now log (event 1009) instead of vanishing. The two calls in the
single-property overload collapse into one try — they map the same rows
and only need to be separate instances, which the comment now says.
Three tests, in the class whose summary already stated this rule for
reads. The two asserting the throw fail against the previous code with
"No exception was thrown", and both verify no row was written; the third
pins the opt-out, and passes either way by design.
CodeQL flagged these as "Generic catch clause" on PR #7, which is how
they were found — filed as style, and a bug underneath.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sadeqabuhattem
added a commit
that referenced
this pull request
Oct 10, 2026
Code scanning posts findings as pull request review comments, and the ruleset on main requires every conversation resolved, so each PR arrived with the merge button already blocked by whatever the quality suite had to say. #7 carried eleven such threads: three were real and fixed, and the other eight were deliberate code or false positives dismissed by hand — three obsolete-method calls inside a CS0618 pragma that exists to exercise the obsolete forwarder, two casts that are load-bearing for Task.FromResult's inference, the observer catch documented as logged-and-swallowed, a Where suggestion and a string concat in a test helper. Seven of the eight were in tests/. Selected by omitting `queries` rather than naming it: that is CodeQL's default suite, the security-focused one. There is no pack named `security`, and naming a non-existent suite fails init outright instead of falling back. This narrows what runs rather than what is reported, so it is a real loss worth recording: the bare-catch bug fixed in 0f2ef53 — a write replacing an undecryptable secret with its compile-time default — was found by a quality query, not a security one. Against that, eight of eleven findings were noise, the friction landed on every PR while the one genuine catch was a one-off, and the compiler already enforces more than the quality suite does (warnings-as-errors, AnalysisLevel latest-recommended). To bring the quality queries back without the blocking, the shape is a codeql-config with query-filters excluding the noisy rules, or paths-ignore for tests/. Verified on #8 itself: zero review threads, against eleven on #7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Closes the API gaps found by reviewing the sample app against the library: the places where a
feature existed but nothing could reach it, or where the shape it exposed was wrong. 25 commits,
each with its reasoning in the message.
The breaking change
The built-in audit trail is gone, replaced by
ISettingWriteObserver. It could not be frozeninto 1.0:
ChangedBywas the literal string"system"on every entry,ClearAsyncdeleted awhole group and recorded nothing, entries were written one round trip at a time after the write
had committed and outside its transaction,
GetHistoryAsyncwas unbounded over a table that onlygrows — and it existed only for EF Core, so a Dapper user got a
SettingAuditstable that nothingever wrote to.
Auditing returns as a general
DotNetBoost.Auditingpackage over the new hook — port in Core,implementation in a satellite package, the way
ISettingValidatorandDotNetBoost.Settings.FluentValidationalready work.Migration: implement
ISettingWriteObserver, or call.UseLoggingWriteObserver()in themeantime. Nothing drops an existing
SettingAuditstable for you.Added
ISettingWriteObserver— one notification per completed write, with the changed properties,the actor, a correlation id and a timestamp.
[Sensitive]properties arrive redacted.GET /api/settingsandGET /api/settings/{route}/schema— nothing enumerated the[SettingGroup]classes or described a group's properties, so a "form generated from the class"was really generated by guessing. Neither returns a stored value.
ISettingProjector<T>/ISettingProjector<T, TProjection>— reshape whatGETreturnswithout touching what is stored; the typed form lets the endpoint advertise the projection
instead of the stored group.
ISettingSchemaContributorand.UseSchemaContributor(), with a FluentValidationimplementation that publishes the bounds its validators already enforce.
ISettingChangeSignaland.SynchronizeThroughDistributedCache()— a write on one instanceno longer leaves the others serving stale values for up to
CacheDuration. The staleness boundbecomes
ChangeCheckInterval(5s default), not zero; the tradeoff is documented.Fixed
SetAsync(selector, value)could revert another writer's change to a different property. Itrebuilt the whole model from the cache and wrote whatever differed, so touching property A undid
someone's edit to property B over a window as wide as
CacheDuration. It now writes a single row.GetAsynchanded every caller the same mutable object. The cache held the mapped instance, soassigning to a property of what you read wrote into the entry every later reader was served from
— including
GET, which then returned the mutated value as though it were stored.Deprecated, not broken
ISettingDbContextis obsolete and does nothing — delete: ISettingDbContextand the twoDbSetproperties.
ApplySettingsConfigurationis renamedApplySettings, with the old name kept as an[Obsolete]forwarder. Both compile with a warning and go in the next release.Release
[Unreleased]becomes[1.0.0-preview.2]in the changelog — it held nothing but this branch'swork. The heading is dated 2026-10-10; update it if the release slips before tagging.
Tag
v1.0.0-preview.2on the merge commit to publish.Verification
Locally, on both
net8.0andnet10.0:dotnet build -c Releaseis clean with 0 warnings underTreatWarningsAsErrors. Unit-test linecoverage is 81.95%, up from 79.03% — the FluentValidation package went from 67.5% to 100% line
coverage to clear the gate. Branch coverage is 78.28%, so this PR also settles whether the
COVERAGE_THRESHOLD: 80gate reads lines or branches.🤖 Generated with Claude Code