[DMS-1252] Derive _etag from ContentVersion instead of a content hash#1091
[DMS-1252] Derive _etag from ContentVersion instead of a content hash#1091stephenfuqua wants to merge 77 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Ed-Fi DMS relational backend’s _etag semantics from a SHA-256 content hash to a composed strong validator derived from ContentVersion plus representation variant inputs, aligning runtime behavior and tests/docs with the accepted ADR (adr-etag-from-content-version.md).
Changes:
- Replace hash-based ETag generation with
ContentVersion+variantKeycomposition across relational read/write paths (including descriptors) and update optimistic concurrency (If-Match) to compare a state-significant projection. - Move HTTP
ETagheader formatting to a quoted strong-validator boundary and normalizeIf-Matchparsing to accept quoted/unquoted strong tags while rejecting weak validators. - Update unit/integration/E2E tests and redesign design docs to reflect profile-/links-sensitive ETags and projection-based
If-Matchmatching.
Reviewed changes
Copilot reviewed 77 out of 78 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dms/tests/EdFi.DataManagementService.Tests.E2E/StepDefinitions/StepDefinitions.cs | Strips quotes from ETag header when capturing _etag for scenario use. |
| src/dms/tests/EdFi.DataManagementService.Tests.E2E/StepDefinitions/ProfileStepDefinitions.cs | Normalizes stored etag header values by stripping strong-validator quotes. |
| src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/Profiles/ProfileFiltering.feature | Updates expectations to require profile-sensitive _etag values. |
| src/dms/frontend/EdFi.DataManagementService.Frontend.AspNetCore/AspNetCoreFrontend.cs | Quotes etag response header as a strong validator at the HTTP boundary. |
| src/dms/frontend/EdFi.DataManagementService.Frontend.AspNetCore.Tests.Unit/RelationalWriteSmokeTests.cs | Updates frontend smoke tests to expect quoted ETag header wrapping an opaque composed value. |
| src/dms/frontend/EdFi.DataManagementService.Frontend.AspNetCore.Tests.Unit/AspNetCoreFrontendResponseHeaderTests.cs | Verifies ETag header is quoted; adds empty-etag behavior coverage. |
| src/dms/core/EdFi.DataManagementService.Core/Utilities/ResourceEtagFormatter.cs | Removes legacy canonical-hash ETag formatter. |
| src/dms/core/EdFi.DataManagementService.Core/Utilities/EtagValue.cs | Adds helpers for composing _etag values and quoting/parsing header wire forms. |
| src/dms/core/EdFi.DataManagementService.Core/Middleware/InjectVersionMetadataToEdFiDocumentMiddleware.cs | Removes middleware-side _etag derivation from request body. |
| src/dms/core/EdFi.DataManagementService.Core/Handler/Utility.cs | Threads readable profile name into projection context for profile-sensitive ETags. |
| src/dms/core/EdFi.DataManagementService.Core/Handler/UpsertHandler.cs | Uses backend-provided ETag only (no request-body fallback). |
| src/dms/core/EdFi.DataManagementService.Core/Handler/UpdateByIdHandler.cs | Uses backend-provided ETag only (no request-body fallback). |
| src/dms/core/EdFi.DataManagementService.Core/Backend/WritePreconditionFactory.cs | Normalizes If-Match header to an unquoted opaque tag via EtagValue. |
| src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Utilities/ResourceEtagFormatterTests.cs | Removes unit tests for legacy hash ETag behavior. |
| src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Utilities/EtagValueTests.cs | Adds unit tests for composing/quoting/parsing ETag wire forms. |
| src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Middleware/InjectVersionMetadataToEdFiDocumentMiddlewareTests.cs | Removes tests asserting middleware-generated _etag. |
| src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Handler/UpsertHandlerTests.cs | Updates handler tests to no longer depend on request-body _etag fallback. |
| src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Handler/UpdateByIdHandlerTests.cs | Updates handler tests to no longer depend on request-body _etag fallback. |
| src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Handler/RelationalWriteSeamTests.cs | Removes stale request _etag injection assertions now that ETag comes from backend. |
| src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Handler/DeleteByIdHandlerTests.cs | Updates expectations for normalized If-Match (unquoted opaque tag). |
| src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Backend/WritePreconditionFactoryTests.cs | Updates tests to validate normalization and weak-tag handling. |
| src/dms/backend/EdFi.DataManagementService.Backend/RelationalWriteExecutionStateResolver.cs | Switches precondition evaluation to composed ETags and projection-based comparison. |
| src/dms/backend/EdFi.DataManagementService.Backend/RelationalReadMaterializer.cs | Composes _etag during ExternalResponse materialization; adds wiring guard requiring variant inputs + mapping set. |
| src/dms/backend/EdFi.DataManagementService.Backend/RelationalDocumentStoreRepository.cs | Supplies ETag variant inputs (profile + format) to materializer for GET/query. |
| src/dms/backend/EdFi.DataManagementService.Backend/RelationalCurrentEtagPreconditionChecker.cs | Composes current ETag from locked ContentVersion + variant inputs and compares projections. |
| src/dms/backend/EdFi.DataManagementService.Backend/RelationalCommittedRepresentationReader.cs | Replaces hydrate/materialize/hash readback with a ContentVersion scalar read + composed ETag. |
| src/dms/backend/EdFi.DataManagementService.Backend/RelationalApiMetadataFormatter.cs | Removes legacy relational metadata hash formatter. |
| src/dms/backend/EdFi.DataManagementService.Backend/ReferenceResolverServiceCollectionExtensions.cs | Registers singleton IEtagComposer for reuse across backend components. |
| src/dms/backend/EdFi.DataManagementService.Backend/Etag/VariantKey.cs | Introduces variantKey model + factory (schemaEpoch/format/profileCode/linkFlag). |
| src/dms/backend/EdFi.DataManagementService.Backend/Etag/ProfileVariantCode.cs | Adds stable profileCode derivation using SHA-256 hex prefix. |
| src/dms/backend/EdFi.DataManagementService.Backend/Etag/EtagMatchProjection.cs | Adds state-significant projection for If-Match comparisons. |
| src/dms/backend/EdFi.DataManagementService.Backend/Etag/EtagComposer.cs | Implements IEtagComposer composing {ContentVersion}-{variantKey}. |
| src/dms/backend/EdFi.DataManagementService.Backend/Etag/DescriptorVariantKey.cs | Provides fixed variant key for descriptor representations. |
| src/dms/backend/EdFi.DataManagementService.Backend/DescriptorWriteHandler.cs | Updates descriptor write responses + precondition checks to use ContentVersion-based composed ETags. |
| src/dms/backend/EdFi.DataManagementService.Backend/DescriptorReadRowReader.cs | Adds ContentVersion to descriptor read row shape. |
| src/dms/backend/EdFi.DataManagementService.Backend/DescriptorReadHandler.cs | Composes descriptor _etag from row ContentVersion + fixed descriptor variant key. |
| src/dms/backend/EdFi.DataManagementService.Backend/DescriptorDocumentMaterializer.cs | Switches descriptor external response _etag to composed ETag (no hashing). |
| src/dms/backend/EdFi.DataManagementService.Backend/DefaultRelationalWriteExecutor.cs | Wires IEtagComposer into the write execution path. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/RelationalReadMaterializerTests.cs | Updates materializer tests to assert composed ETags + new wiring guard behavior. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/RelationalDocumentStoreRepositoryTests.cs | Updates repository tests to treat write-result ETags as opaque values (no hashing). |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/RelationalCurrentEtagPreconditionCheckerTests.cs | Updates precondition checker tests to validate composed ETags and projection matching. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/RelationalCommittedRepresentationReaderTests.cs | Updates committed representation reader tests to validate scalar ContentVersion read + composed ETag. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/RelationalApiMetadataFormatterTests.cs | Removes unit tests for legacy metadata hash formatter. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/Etag/VariantKeyTests.cs | Adds unit tests for variantKey formatting rules. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/Etag/ProfileVariantCodeTests.cs | Adds unit tests for profileCode derivation. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/Etag/EtagMatchProjectionTests.cs | Adds unit tests for projection behavior and malformed-tag handling. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/Etag/EtagComposerTests.cs | Adds unit tests for composed {ContentVersion}-{variantKey} output. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/Etag/DescriptorVariantKeyTests.cs | Adds unit tests for descriptor fixed variant key. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/DescriptorWriteHandlerResponseEtagTests.cs | Updates descriptor write tests to assert composed response ETags. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/DescriptorWriteHandlerPreconditionTests.cs | Updates descriptor precondition tests for composed ETags + contentVersion return rows. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/DescriptorWriteHandlerNamespaceAuthorizationTests.cs | Updates namespace-authorization tests to include contentVersion rows for ETag composition. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/DescriptorWriteHandlerDeleteTests.cs | Wires IEtagComposer into delete tests fixture. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/DescriptorReadRowReaderTests.cs | Updates row-reader tests for added ContentVersion column. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/DescriptorReadHandlerTests.cs | Updates descriptor read handler tests to assert composed descriptor ETags. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/DescriptorReadHandlerNamespaceAuthorizationTests.cs | Updates namespace-authorization read tests for added ContentVersion column and composer wiring. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Unit/DescriptorDocumentMaterializerTests.cs | Updates materializer tests to assert composed descriptor ETags. |
| src/dms/backend/EdFi.DataManagementService.Backend.Tests.Common/RelationalGetIntegrationTestHelper.cs | Removes body-derived expected ETag; adds composed ETag shape assertion and excludes _etag from canonical body comparisons. |
| src/dms/backend/EdFi.DataManagementService.Backend.Postgresql.Tests.Integration/PostgresqlResourceLinksFlagIntegrationTests.cs | Updates link-flag integration tests: served ETags differ by linkFlag, but projections match. |
| src/dms/backend/EdFi.DataManagementService.Backend.Postgresql.Tests.Integration/PostgresqlRelationalWritePostAsUpdateSmokeTests.cs | Updates assertions to validate composed write-result ETag shape rather than content-hash. |
| src/dms/backend/EdFi.DataManagementService.Backend.Postgresql.Tests.Integration/PostgresqlRelationalWriteAuthoritativeSampleStudentSchoolAssociationSmokeTests.cs | Updates profile/link-mode ETag expectations; adds profile name threading for ETag composition. |
| src/dms/backend/EdFi.DataManagementService.Backend.Postgresql.Tests.Integration/PostgresqlRelationalWriteAuthoritativeSampleSmokeTests.cs | Updates profile-sensitive ETag expectations and cascade assertions using composed ETag shape. |
| src/dms/backend/EdFi.DataManagementService.Backend.Postgresql.Tests.Integration/PostgresqlRelationalQueryExecutionTests.cs | Wires composer into integration test materializer wrapper. |
| src/dms/backend/EdFi.DataManagementService.Backend.Postgresql.Tests.Integration/PostgresqlProfileIfMatchEtagTests.cs | Updates tests to assert profile-sensitive ETags + same ContentVersion component across profiles. |
| src/dms/backend/EdFi.DataManagementService.Backend.Postgresql.Tests.Integration/PostgresqlDescriptorReadGetTests.cs | Updates descriptor GET integration tests to assert composed ETag shape (no body hashing). |
| src/dms/backend/EdFi.DataManagementService.Backend.Mssql.Tests.Integration/MssqlResourceLinksFlagIntegrationTests.cs | MSSQL counterpart: served ETags differ across link-flag flip; projections match. |
| src/dms/backend/EdFi.DataManagementService.Backend.Mssql.Tests.Integration/MssqlRelationalWriteAuthoritativeSampleStudentSchoolAssociationSmokeTests.cs | MSSQL counterpart: updates link/profile ETag expectations; threads profile name into ETag. |
| src/dms/backend/EdFi.DataManagementService.Backend.Mssql.Tests.Integration/MssqlRelationalWriteAuthoritativeDs52SurveySmokeTests.cs | Updates cascade assertions to validate composed ETag shape rather than body-derived ETag. |
| src/dms/backend/EdFi.DataManagementService.Backend.Mssql.Tests.Integration/MssqlRelationalQueryExecutionTests.cs | Wires composer into integration test materializer wrapper. |
| src/dms/backend/EdFi.DataManagementService.Backend.Mssql.Tests.Integration/MssqlProfileIfMatchEtagTests.cs | MSSQL counterpart: profile-sensitive ETags and ContentVersion-component equivalence checks. |
| src/dms/backend/EdFi.DataManagementService.Backend.Mssql.Tests.Integration/MssqlDescriptorWriteTests.cs | Adds descriptor write ETag parity test vs follow-up GET; updates delete precondition tests. |
| src/dms/backend/EdFi.DataManagementService.Backend.Mssql.Tests.Integration/MssqlDescriptorReadGetTests.cs | Updates descriptor GET integration tests to assert composed ETag shape. |
| src/dms/backend/EdFi.DataManagementService.Backend.External/RelationalGetRequestContracts.cs | Extends readable profile projection context contract to include ProfileName for ETag composition. |
| reference/design/backend-redesign/design-docs/update-tracking.md | Updates normative design to specify composed ETag + variantKey encoding and projection-based If-Match matching. |
| reference/design/backend-redesign/design-docs/transactions-and-concurrency.md | Updates concurrency docs to reflect composed ETags and projected If-Match comparison. |
| reference/design/backend-redesign/design-docs/flattening-reconstitution.md | Updates reconstitution docs to remove hashing requirement and specify composed ETag. |
| .gitignore | Normalizes ignore rule for docs/superpowers path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stephenfuqua
force-pushed
the
etag-content-version
branch
from
eb9d44c to
415708e
Compare
|
I was doing some perf testing this weekend, and a subset of what you are working on here surfaced so I held off on making that subset change. I did take a look at the design so far and have a recommendation to move the ContentVersion lookup: SuggestionThe lookup should move into Right now the flow is:
That last step is no longer really a “committed representation read.” It does not hydrate a representation anymore; it only fetches write metadata. That metadata is part of the persistence outcome, so it belongs in The safer incremental design would be:
internal sealed record RelationalWritePersistResult(
long DocumentId,
DocumentUuid DocumentUuid,
long ContentVersion
);
That timing matters. Reading the root insert stamp is not enough because child table deletes/upserts can fire stamp triggers that bump the owning root document. The final version is only known after every table operation has run.
The “why” is mostly ownership and correctness:
This move would not immediately eliminate the database round trip unless we also teach the DML path to surface the final stamp. But it puts that round trip in the right layer, after all mutations, and makes the remaining work visible and easy to remove later. |
| see [Amendment (2026-07-05, wildcard)](#amendment-2026-07-05-if-match-wildcard-matching). \ | ||
| **Date:** 2026-06-30 (accepted 2026-07-03) \ | ||
| **Deciders:** Development team (signed off 2026-07-03). \ | ||
| **Author:** Stephen Fuqua, with analysis assistance from Claude Opus 4.8 (Claude Code). |
There was a problem hiding this comment.
I made several key amendments to ensure backwards compatibility with the ODS/API.
Before final merge, I want to consolidate the amendments directly into the flow of the document. But leaving for now so that reviewers can clearly see and confirm those changes to the requirements.
There was a problem hiding this comment.
This file was previously recreating the Descriptor object in order to hash it, hence no longer needed.
| """ | ||
| When a DELETE if-match "{IfMatch}" request is made to "/ed-fi/students/{id}" | ||
| Then it should respond with 204 | ||
| @e2e-ci-shard-1 |
There was a problem hiding this comment.
I have not studied the "sharding" concept, so I have no idea if shard-1 is appropriate or not.
| // As of the 2026-07-04 ADR amendment, profileCode is no longer state-significant for If-Match | ||
| // (EtagMatchProjection compares only ContentVersion + schemaEpoch), so using an etag from a | ||
| // profiled GET here is incidental rather than required - the unprofiled seed POST's etag would | ||
| // also match. GET never sets an ETag response header (GetByIdHandler always returns Headers: []), |
There was a problem hiding this comment.
GET never sends an ETag response handler? Is the plain reading accurate - which would indicate a potential bug?
The Ed-Fi API Design Guidelines partially misses this point. The GET Requests section does not list it as a header, but it is mentioned as a requirement in the Optimistic Concurrency section.
Note: RFC 9110 specifies that "An origin server SHOULD send an ETag for any selected representation for which detection of changes can be reasonably and consistently determined...".
I need to investigate this point in the DMS code.
There was a problem hiding this comment.
Copilot review also noted this.
There was a problem hiding this comment.
This is addressed in another PR into this branch.
| *.lscache | ||
|
|
||
| ./docs/superpowers/** | ||
| docs/superpowers/ |
There was a problem hiding this comment.
The original rule was not working properly.
stephenfuqua
changed the title
🔴 CriticalNone found. 🟠 High
Note This is an existing bug that I did not (yet) try to fix. I will create a new ticket for that, and create a pull request into this branch with the fix. -SF
Note I need to confirm this. Having a profile on descriptor bodies seems a bit strange. But if technically feasible, then yes we need to account for it. -SF
Note Will look into this. -SF 🟡 Medium
Note I agree. -SF
Note I agree. -SF
Note Technically an interesting point, but I don't know that it is relevant here. The etag is opaque, so why would there be any details in the OpenAPI descriptions? I should look carefully to confirm that. As a brand-new application, release notes / migration guidance are irrelevant. -SF
Note Not relevant, since we don't migrate data within a school year. -SF
Note I agree, this is important functionality and it needs to be exercised appropriately. -SF
Note I agree. -SF
Note I agree. -SF 🔵 Low
Note I agree. -SF
Note I agree. -SF
Note Thinking about that, and need to evaluate the impact of this change. Its not like there are a large number of profiles that would make this risk very likely. Also very low impact in the rare case that a collision occurs. -SF ⚪ Nit
Note Yeah, this wasn't worth mentioning. -SF Specialist Verdicts
SummaryTwo High functionality gaps need resolution before merge: GET-by-id responses never emit an Note This summary doesn't count things very well... -SF Reference: DMS-1252 |
…5, 8–12) (#1099) * refactor: add EtagValue.TryParse and Separator constant * refactor: give VariantKey a typed component parser/formatter (finding 5) * refactor: rename VariantKey builder to FromComponents to avoid member shadow * test: guard schema-epoch mismatch in the precondition checker (finding 9) * refactor: EtagMatchProjection delegates to the typed wire parsers (finding 5) * feat: add IServedEtagComposer service (finding 4) * feat: add IIfMatchEvaluator service (finding 4) * chore: register IServedEtagComposer and IIfMatchEvaluator * refactor: route the precondition checker through the etag services (finding 4) * refactor: route the write execution resolver through the etag services (finding 4) * refactor: compose read/write-response etags via IServedEtagComposer (finding 4) * refactor: route descriptor write compose/compare through the etag services (finding 4) * fix: update integration read-materializer wrappers to IServedEtagComposer Fallout from routing RelationalReadMaterializer through IServedEtagComposer; these integration-test wrapper constructions use target-typed new(...) and were not caught by the unit-project build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: descriptor served etag varies by readable profile (finding 2) * feat: add ETagPreconditionFailureReason to precondition-failure results (finding 3) * feat: tag precondition failures with concurrency vs target-missing reason (finding 3) * feat: reason-specific 412 body factory (finding 3) * feat: return reason-specific 412 detail from write handlers (finding 3) * test: assert distinct 412 body for wildcard-missing target (finding 3) Adds response-body assertions to etag.feature scenarios 10 and 11 (wildcard If-Match on a non-existent PUT/DELETE target) asserting the new target-does-not-exist 412 body. Concurrency scenarios 04/05 are unchanged (that body stayed byte-identical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: capture raw quoted ETag and add {IfMatchQuoted} token (finding 8) * test: quoted If-Match round-trip on PUT and DELETE (finding 8) * test: profiled/unprofiled DELETE etag parity on PostgreSQL (finding 10) * test: assert profiled etag changes on hidden-field bump before stale DELETE (finding 10) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: profiled/unprofiled DELETE etag parity on SQL Server (finding 10) * docs: correct stale legacy-content-hash comments on the read materializer (finding 11) The EtagVariant XML docs on RelationalReadMaterializationRequest and RelationalReadPageMaterializationRequest claimed a null value falls back to a legacy content hash, but ComposeEtag throws on a null EtagVariant/MappingSet for an external response. Reframe both as: required on external-response materialization; absence is a caller wiring bug; no legacy content-hash path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: debug-log the If-Match decision in the precondition checker (finding 12) * fix: sanitize the client If-Match tag in the precondition-checker debug log (finding 12) * feat: debug-log the deferred If-Match decision in the resolver (finding 12) * feat: debug-log the descriptor If-Match decision (finding 12) * refactor: guard finding-12 If-Match debug logs with IsEnabled(Debug) (finding 12) * docs: clarify the resolver missing-target debug message (finding 12) The currentState-null debug log said "precondition fails" unconditionally, but the non-wildcard PUT arm returns UpdateFailureNotExists (404 not-exists), not a 412 precondition failure. Reword to "resolving missing-target outcome" so the message is accurate across all three arms (Post conflict, wildcard-PUT 412, non-wildcard-PUT 404). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: Improve XML comment; look for last `-` for future resilience Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…5, 8–12) (#1099) * refactor: add EtagValue.TryParse and Separator constant * refactor: give VariantKey a typed component parser/formatter (finding 5) * refactor: rename VariantKey builder to FromComponents to avoid member shadow * test: guard schema-epoch mismatch in the precondition checker (finding 9) * refactor: EtagMatchProjection delegates to the typed wire parsers (finding 5) * feat: add IServedEtagComposer service (finding 4) * feat: add IIfMatchEvaluator service (finding 4) * chore: register IServedEtagComposer and IIfMatchEvaluator * refactor: route the precondition checker through the etag services (finding 4) * refactor: route the write execution resolver through the etag services (finding 4) * refactor: compose read/write-response etags via IServedEtagComposer (finding 4) * refactor: route descriptor write compose/compare through the etag services (finding 4) * fix: update integration read-materializer wrappers to IServedEtagComposer Fallout from routing RelationalReadMaterializer through IServedEtagComposer; these integration-test wrapper constructions use target-typed new(...) and were not caught by the unit-project build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: descriptor served etag varies by readable profile (finding 2) * feat: add ETagPreconditionFailureReason to precondition-failure results (finding 3) * feat: tag precondition failures with concurrency vs target-missing reason (finding 3) * feat: reason-specific 412 body factory (finding 3) * feat: return reason-specific 412 detail from write handlers (finding 3) * test: assert distinct 412 body for wildcard-missing target (finding 3) Adds response-body assertions to etag.feature scenarios 10 and 11 (wildcard If-Match on a non-existent PUT/DELETE target) asserting the new target-does-not-exist 412 body. Concurrency scenarios 04/05 are unchanged (that body stayed byte-identical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: capture raw quoted ETag and add {IfMatchQuoted} token (finding 8) * test: quoted If-Match round-trip on PUT and DELETE (finding 8) * test: profiled/unprofiled DELETE etag parity on PostgreSQL (finding 10) * test: assert profiled etag changes on hidden-field bump before stale DELETE (finding 10) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: profiled/unprofiled DELETE etag parity on SQL Server (finding 10) * docs: correct stale legacy-content-hash comments on the read materializer (finding 11) The EtagVariant XML docs on RelationalReadMaterializationRequest and RelationalReadPageMaterializationRequest claimed a null value falls back to a legacy content hash, but ComposeEtag throws on a null EtagVariant/MappingSet for an external response. Reframe both as: required on external-response materialization; absence is a caller wiring bug; no legacy content-hash path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: debug-log the If-Match decision in the precondition checker (finding 12) * fix: sanitize the client If-Match tag in the precondition-checker debug log (finding 12) * feat: debug-log the deferred If-Match decision in the resolver (finding 12) * feat: debug-log the descriptor If-Match decision (finding 12) * refactor: guard finding-12 If-Match debug logs with IsEnabled(Debug) (finding 12) * docs: clarify the resolver missing-target debug message (finding 12) The currentState-null debug log said "precondition fails" unconditionally, but the non-wildcard PUT arm returns UpdateFailureNotExists (404 not-exists), not a 412 precondition failure. Reword to "resolving missing-target outcome" so the message is accurate across all three arms (Post conflict, wildcard-PUT 412, non-wildcard-PUT 404). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: Improve XML comment; look for last `-` for future resilience Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
stephenfuqua
force-pushed
the
etag-content-version
branch
from
473ade9 to
caae22f
Compare
…5, 8–12) (#1099) * refactor: add EtagValue.TryParse and Separator constant * refactor: give VariantKey a typed component parser/formatter (finding 5) * refactor: rename VariantKey builder to FromComponents to avoid member shadow * test: guard schema-epoch mismatch in the precondition checker (finding 9) * refactor: EtagMatchProjection delegates to the typed wire parsers (finding 5) * feat: add IServedEtagComposer service (finding 4) * feat: add IIfMatchEvaluator service (finding 4) * chore: register IServedEtagComposer and IIfMatchEvaluator * refactor: route the precondition checker through the etag services (finding 4) * refactor: route the write execution resolver through the etag services (finding 4) * refactor: compose read/write-response etags via IServedEtagComposer (finding 4) * refactor: route descriptor write compose/compare through the etag services (finding 4) * fix: update integration read-materializer wrappers to IServedEtagComposer Fallout from routing RelationalReadMaterializer through IServedEtagComposer; these integration-test wrapper constructions use target-typed new(...) and were not caught by the unit-project build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: descriptor served etag varies by readable profile (finding 2) * feat: add ETagPreconditionFailureReason to precondition-failure results (finding 3) * feat: tag precondition failures with concurrency vs target-missing reason (finding 3) * feat: reason-specific 412 body factory (finding 3) * feat: return reason-specific 412 detail from write handlers (finding 3) * test: assert distinct 412 body for wildcard-missing target (finding 3) Adds response-body assertions to etag.feature scenarios 10 and 11 (wildcard If-Match on a non-existent PUT/DELETE target) asserting the new target-does-not-exist 412 body. Concurrency scenarios 04/05 are unchanged (that body stayed byte-identical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: capture raw quoted ETag and add {IfMatchQuoted} token (finding 8) * test: quoted If-Match round-trip on PUT and DELETE (finding 8) * test: profiled/unprofiled DELETE etag parity on PostgreSQL (finding 10) * test: assert profiled etag changes on hidden-field bump before stale DELETE (finding 10) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: profiled/unprofiled DELETE etag parity on SQL Server (finding 10) * docs: correct stale legacy-content-hash comments on the read materializer (finding 11) The EtagVariant XML docs on RelationalReadMaterializationRequest and RelationalReadPageMaterializationRequest claimed a null value falls back to a legacy content hash, but ComposeEtag throws on a null EtagVariant/MappingSet for an external response. Reframe both as: required on external-response materialization; absence is a caller wiring bug; no legacy content-hash path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: debug-log the If-Match decision in the precondition checker (finding 12) * fix: sanitize the client If-Match tag in the precondition-checker debug log (finding 12) * feat: debug-log the deferred If-Match decision in the resolver (finding 12) * feat: debug-log the descriptor If-Match decision (finding 12) * refactor: guard finding-12 If-Match debug logs with IsEnabled(Debug) (finding 12) * docs: clarify the resolver missing-target debug message (finding 12) The currentState-null debug log said "precondition fails" unconditionally, but the non-wildcard PUT arm returns UpdateFailureNotExists (404 not-exists), not a 412 precondition failure. Reword to "resolving missing-target outcome" so the message is accurate across all three arms (Post conflict, wildcard-PUT 412, non-wildcard-PUT 404). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: Improve XML comment; look for last `-` for future resilience Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
stephenfuqua
force-pushed
the
etag-content-version
branch
from
1e7d974 to
13f5c47
Compare
|
I have addressed Brad's review comment, and all addressable comments in my own big Claude-guided reivew. |
ReviewFindings
Simplicity Notes
Review-of-reviewI’d treat the first two as real blockers. The strongest findings are:
I also agree with these, with nuance:
The remaining lows are fair cleanup notes rather than blockers: One extra note: Jira DMS-1252 itself still contains older |
Complexity ReviewThe underlying feature is simple:
The branch implements that idea with more moving parts than the idea probably needs. Some size is justified by PG/MSSQL, descriptors, profiles, docs, and tests. But the complexity is showing up as real risk: descriptor write ETags drift from descriptor read ETags, SQL result-set ordering matters, and there are helper layers that no longer buy much. The main over-complicated areas are:
I would not call the whole implementation unreasonably large, because DMS has real cross-cutting behavior here. But I would call it more complicated than necessary. The safest simplification would be to centralize ETag composition/projection, make write paths return a typed ETag/string instead of JSON, remove dead descriptor-specific helpers, and standardize the “final ContentVersion” SQL pattern. That should reduce code while also addressing several correctness findings. |
An intermediate refactor dropped the response reader's SELECT and composed the write-response etag from the persist-time (root INSERT ... RETURNING) ContentVersion, per Option 4's "zero marginal cost" assumption. PR #1091 review found that value is stale once child-table deletes/upserts fire stamp triggers that bump the owning root document's ContentVersion. Add a 2026-07-08 amendment documenting the reason the final ContentVersion read was restored and relocated into the persister (read after every table mutation, returned as persistence metadata on RelationalWritePersistResult), so the write etag matches a follow-up GET. The read is a lightweight lookup in the correct layer, not the hydrate-materialize-hash readback the ADR eliminated, and leaves a clear path to reclaiming the round trip later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up on the review commentsVerified each point against the code. Six items landed; four I'm pushing back on with reasoning below. Implemented1. Profile-code descriptor write-response ETags ( 2. MSSQL descriptor INSERT surfaces 3. ADR: why the final 4. ADR: 5. Dropped the 6. Write path returns the etag as a string, not a wrapped Not changed, with reasoning"Collapse the ~8-type helper stack into one "Descriptors should reuse the generic build-served-etag path" — they already do: "Standardize the final "Profile-code contract split between docs and impl" — not duplicate definitions. Single implementation (
|
Re-reviewFindings
Simplicity Notes One small remaining simplification: |
Review follow-ups addressedWorked through the review comments on this branch. Six commits, net −302 lines (24 files, +268/−570). All changes verified by build + tests; PostgreSQL integration tests run against a real database via a throwaway container.
1.
|
Added E2E coverage for the served-ETag contractTwo E2E scenarios in
|
Adds the reviewer-approved superpowers implementation plan for deriving _etag from ContentVersion + a representation variantKey, plus the source ADR and staged design-doc edits it references. Plan and ADR are DRAFT pending development-team review; no code changed. Drafted with AI assistance (Claude Opus 4.8); human review required before acting on the plan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… _etag If-Match compares only the state-significant projection of the etag (ContentVersion, schemaEpoch, profileCode); format and linkFlag are excluded, so link/format differences no longer 412 while a cross-profile If-Match still returns 412. Updates the ADR's "If-Match comparison" section (now decided) and the matching companion design-doc wording. Drafted with AI assistance (Claude Opus 4.8); human review required before acting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ve etags Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tKey Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds 5 wildcard fixtures per engine (PUT/DELETE existing -> success, PUT/DELETE missing -> 412, POST-insert -> 412). Running these revealed a gap: a genuinely-missing PUT target resolves not-exists early in RelationalDocumentStoreRepository.ResolveTargetContextAsync, before the write executor runs, so it bypassed the B3 missing-target override and returned 404 for a wildcard. Threads the write precondition into that resolution and returns 412 for a wildcard, matching RFC 7232. Adds a fast unit regression for the repository-level path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This reverts commit b3a219b.
…design docs Update transactions-and-concurrency.md and update-tracking.md to describe the 2026-07-05 amendments: bare unquoted If-Match: * as an RFC 7232 wildcard existence precondition (412 on missing PUT/DELETE target and POST-insert), and acceptance of unquoted If-Match values as equivalent to quoted (emission stays quoted; W/ weak tags still rejected). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RelationalWriteNoProfilePersister.PersistAsync now issues a trailing SELECT of dms.Document.ContentVersion (via RelationalDocumentLockCommandBuilder) after all deletes and upserts, and populates the previously-defaulted ContentVersion field on RelationalWritePersistResult. Reader-side behavior is unchanged; this only makes the value available on the result.
…drop reader SELECT
Mirrors the existing DocumentId <= 0 guard in RelationalWritePersistedTargetValidator so a future persister change that forgets to set a positive ContentVersion fails loudly instead of silently composing an etag from version 0.
…5, 8–12) (#1099) * refactor: add EtagValue.TryParse and Separator constant * refactor: give VariantKey a typed component parser/formatter (finding 5) * refactor: rename VariantKey builder to FromComponents to avoid member shadow * test: guard schema-epoch mismatch in the precondition checker (finding 9) * refactor: EtagMatchProjection delegates to the typed wire parsers (finding 5) * feat: add IServedEtagComposer service (finding 4) * feat: add IIfMatchEvaluator service (finding 4) * chore: register IServedEtagComposer and IIfMatchEvaluator * refactor: route the precondition checker through the etag services (finding 4) * refactor: route the write execution resolver through the etag services (finding 4) * refactor: compose read/write-response etags via IServedEtagComposer (finding 4) * refactor: route descriptor write compose/compare through the etag services (finding 4) * fix: update integration read-materializer wrappers to IServedEtagComposer Fallout from routing RelationalReadMaterializer through IServedEtagComposer; these integration-test wrapper constructions use target-typed new(...) and were not caught by the unit-project build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: descriptor served etag varies by readable profile (finding 2) * feat: add ETagPreconditionFailureReason to precondition-failure results (finding 3) * feat: tag precondition failures with concurrency vs target-missing reason (finding 3) * feat: reason-specific 412 body factory (finding 3) * feat: return reason-specific 412 detail from write handlers (finding 3) * test: assert distinct 412 body for wildcard-missing target (finding 3) Adds response-body assertions to etag.feature scenarios 10 and 11 (wildcard If-Match on a non-existent PUT/DELETE target) asserting the new target-does-not-exist 412 body. Concurrency scenarios 04/05 are unchanged (that body stayed byte-identical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: capture raw quoted ETag and add {IfMatchQuoted} token (finding 8) * test: quoted If-Match round-trip on PUT and DELETE (finding 8) * test: profiled/unprofiled DELETE etag parity on PostgreSQL (finding 10) * test: assert profiled etag changes on hidden-field bump before stale DELETE (finding 10) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: profiled/unprofiled DELETE etag parity on SQL Server (finding 10) * docs: correct stale legacy-content-hash comments on the read materializer (finding 11) The EtagVariant XML docs on RelationalReadMaterializationRequest and RelationalReadPageMaterializationRequest claimed a null value falls back to a legacy content hash, but ComposeEtag throws on a null EtagVariant/MappingSet for an external response. Reframe both as: required on external-response materialization; absence is a caller wiring bug; no legacy content-hash path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: debug-log the If-Match decision in the precondition checker (finding 12) * fix: sanitize the client If-Match tag in the precondition-checker debug log (finding 12) * feat: debug-log the deferred If-Match decision in the resolver (finding 12) * feat: debug-log the descriptor If-Match decision (finding 12) * refactor: guard finding-12 If-Match debug logs with IsEnabled(Debug) (finding 12) * docs: clarify the resolver missing-target debug message (finding 12) The currentState-null debug log said "precondition fails" unconditionally, but the non-wildcard PUT arm returns UpdateFailureNotExists (404 not-exists), not a 412 precondition failure. Reword to "resolving missing-target outcome" so the message is accurate across all three arms (Post conflict, wildcard-PUT 412, non-wildcard-PUT 404). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: Improve XML comment; look for last `-` for future resilience Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The e2e assertion required a profile-projected descriptor read's _etag to equal the full (non-profile) etag. That expectation dates to the content-hash era (#947) and contradicts the composed-etag contract on this branch, where a profile-projected representation carries a distinct, profile-sensitive etag (RFC 7232 strong-validator semantics). This mirrors the resource contract in ProfileFiltering scenario 07 and is covered by DescriptorReadHandlerTests (It_applies_readable_profile_projection_and_varies_the_etag_by_profile). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Descriptor POST/PUT success paths (and the no-op current-etag path) always composed the response ETag with ProfileName: null, so a profiled descriptor write returned an unprofiled ETag while a profiled descriptor GET returns a profile-coded one. The repository descriptor branches also dropped BackendProfileWriteContext entirely. Thread the profile name through DescriptorWriteRequest, forward it from both repository descriptor branches, and compose every descriptor write response ETag (insert, upsert-update, PUT update, and the no-op current etag) with it. If-Match comparison stays profile-insensitive per the 2026-07-04 ADR amendment because EtagMatchProjection projects profileCode out, so the current etag can safely carry the profile code. Adds unit coverage for profile-coded insert/update/no-op ETags and Postgresql + Mssql integration tests proving profiled write ETags match a follow-up profiled GET and differ from the unprofiled representation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iling SELECT BuildMssqlInsertCommand put the row-producing statement first — OUTPUT INSERTED.[ContentVersion] on the Document insert, followed by the descriptor and referential-identity inserts. The reader read that first result set and returned, relying on the whole batch executing server-side after the OUTPUT row was consumed rather than on the row-producing statement being unambiguous. Capture the insert-time ContentVersion into a table variable via OUTPUT ... INTO, run every insert, then return it with a trailing SELECT, so the final statement is the one that produces the reader's single result set — matching the PostgreSQL insert CTE and every UPDATE/upsert builder. Behavior is unchanged; the descriptor write integration tests exercise the insert path end-to-end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An intermediate refactor dropped the response reader's SELECT and composed the write-response etag from the persist-time (root INSERT ... RETURNING) ContentVersion, per Option 4's "zero marginal cost" assumption. PR #1091 review found that value is stale once child-table deletes/upserts fire stamp triggers that bump the owning root document's ContentVersion. Add a 2026-07-08 amendment documenting the reason the final ContentVersion read was restored and relocated into the persister (read after every table mutation, returned as persistence metadata on RelationalWritePersistResult), so the write etag matches a follow-up GET. The read is a lightweight lookup in the correct layer, not the hydrate-materialize-hash readback the ADR eliminated, and leaves a clear path to reclaiming the round trip later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The variantKey spec described profileCode as a stable compile-time integer index within the MappingSet, but the implementation (ProfileVariantCode.Of) derives it as an 8-hex SHA-256 prefix of the readable profile name (or "_" when unprofiled) — a MappingSet exposes no enumerable profile catalog for stable ordinals. Add a 2026-07-08 amendment plus an inline note on the variantKey encoding recording the shipped form and affirming it upholds the ADR's core decision: hashing the tiny, static profile-name descriptor is not hashing the representation JSON. It needs no readback and no per-document work, so it cannot reintroduce the write-path bottleneck the ADR removed; the etag's state signal still comes entirely from ContentVersion, and profileCode is projected out of If-Match anyway. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removes the IEtagComposer interface, its injectable implementation, and its DI registration. The interface existed only as a substitution seam over a pure string-formatting helper that would never be mocked, so it bought nothing. EtagComposer stays as a static helper because the (ContentVersion, VariantKey) -> opaque wire value bridge cannot live in Core: EtagValue (Core) cannot reference the backend-only VariantKey type. ServedEtagComposer now delegates to the static and no longer takes a constructor dependency; all call sites updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…JsonNode
RelationalCommittedRepresentationReader composed the served _etag as a string,
boxed it into a { "_etag": ... } JsonObject, and returned Task<JsonNode>; the
sole consumer (RelationalWriteExecutorResults) immediately unwrapped it back to
a string via GetCommittedResponseEtag, whose validation could only fail if this
code were inconsistent with itself. That JsonNode round-trip was the old
hydrate/readback response shape left behind after the readback was removed.
The reader now returns Task<string> (the opaque etag). The result builders take
the etag directly and guard non-empty via RequireEtag; GetCommittedResponseEtag
is deleted. No behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The descriptor write etag helper read only the first result set, so it relied on the trailing SELECT "ContentVersion" being the first result set the driver exposes ahead of the batch UPDATE/INSERT/MERGE statements. That holds for Npgsql and SqlClient today, but is an implicit dependency on driver result-set ordering. Scan result sets until the first row instead, stopping at the ContentVersion the trailing SELECT produces, matching the do/while NextResultAsync pattern already used in RelationalDeleteExecution. Behavior is unchanged on both dialects; this only removes the ordering assumption. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The DELETE If-Match precondition re-locked the target and hydrated its full current state (with descriptor projection) only to read back a ContentVersion the caller had already captured when it locked the row. DELETE serves no body and applies no profile lens, so the current etag needs only that locked ContentVersion plus the schema epoch. Replace the delete precondition's async CheckAsync (read plan + lock + load) with a synchronous Evaluate that composes the served etag from the locked ContentVersion and evaluates If-Match — no re-lock, no hydration, no read plan. Existence and concurrency are already settled by the repository's upfront resolve/lock, so the result is never null; drop the now-dead null branch and the delete read-plan preparation/guard. The guarded-session integration assertions now expect a single document lock (lock -> authorize -> delete) rather than a post-authorization second lock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
update-tracking.md still described three things the code no longer does: - "no database readback / no database dependency" for _etag — now clarified to "no document hashing and no representation readback," noting the write path reads a single lightweight scalar ContentVersion in the persistence layer (per the ADR's 2026-07-08 final-ContentVersion-read amendment). The eliminated hydrate-materialize-hash readback stays eliminated. - profileCode as a "compile-time index" — corrected to the shipped encoding: "_" or the first 8 lowercase hex chars of SHA-256(UTF-8(profileName)), per ProfileVariantCode.Of and the ADR's 2026-07-08 profileCode-hash amendment. adr-etag-from-content-version.md still said descriptor write-response ETags "remain unprofiled." A superseding 2026-07-08 note records that DescriptorWriteHandler now composes every success-response etag with the request's ProfileName (fix efaa4f5), so a profiled write returns the same profile-variant _etag a profiled GET serves; If-Match stays profile-insensitive. transactions-and-concurrency.md carried the same "computed with no readback" claim and is aligned to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
EtagMatchProjection's summary claimed "a malformed tag yields a sentinel that cannot equal any well-formed projection," but If-Match compares only ContentVersion + schemaEpoch (2026-07-04 amendment) and does not validate the three ignored variantKey positions. So a four-part tag with empty or unrecognized ignored components (e.g. "5-a1b2c3d4...") projects to the same "5-a1b2c3d4" as a well-formed tag and matches — contradicting the stated contract, though with no lost-update risk. Document the tolerance rather than validate the components: validation would re-couple If-Match to the format/profile/link selectors the 2026-07-04 amendment deliberately decoupled (breaking cross-format/profile/link If-Match) and would add no safety, since a match still requires the correct ContentVersion and schemaEpoch. - EtagMatchProjection: define "malformed" as structural only, and spell out that the ignored positions are intentionally not validated. - VariantKey.TryParseComponents: note it validates part count, not values. - Add a test pinning the tolerance (the reviewer's "5-a1b2c3d4..." example matches; a wrong epoch/version still does not). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rite executor Once the etag readback was eliminated, IRelationalCommittedRepresentationReader was a readback-era abstraction that no longer read anything: an async interface whose implementation wrapped only IServedEtagComposer + ResourceLinksOptions and returned Task.FromResult over a pure string composition. Inline it into DefaultRelationalWriteExecutor as a synchronous private ComposeCommittedEtag helper. The executor already had IServedEtagComposer; it now also takes IOptions<ResourceLinksOptions> for the served linkFlag. Both write success paths (applied write and guarded no-op) compose the etag directly from the persisted/observed ContentVersion, with no dms.Document query, hydrate, or hashing. Delete the interface, its implementation, its DI registration, and its dedicated test. Update the executor tests to assert the real composed etag instead of stubbing the reader, and drop the three DI-assertion probes for the removed service. Reconcile the ContentVersion ADR's 2026-07-08 amendment, which named the now-removed reader. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ionCheckResult The sole production consumer of the result (RelationalWriteExecutionStateResolver) reads only TargetContext, CurrentState, and IsMatch; the composed CurrentEtag was returned but never used outside tests. Remove the field from the record. The checker still composes the current served etag locally — it is the input to the If-Match evaluation and the debug log — but no longer surfaces it as dead result state. Drop the corresponding test assertions and the now-unused currentEtag parameter/argument in the executor test helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Scenario 14 verifies the API-boundary contract rather than merely that
some ETag exists: the response header is a quoted strong validator, the
opaque value matches "{ContentVersion}-{schemaEpoch}.{format}.{profileCode}.{linkFlag}",
and the unquoted body _etag from a follow-up GET equals the quote-stripped
header value. Adds a reusable "the ETag value matches the pattern" step.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Scenario 15 proves the ADR concern that the write response reflects the post-write ContentVersion after a child-table mutation: it POSTs a studentEducationOrganizationAssociation, changes only an address (child collection) value via PUT, and asserts the served ETag advanced, that the now-stale ETag is rejected as If-Match (412), and that the current ETag still satisfies If-Match (204). Adds "the ETag is stored in request variable" / "the ETag differs from request variable" steps and teaches the PUT if-match step to resolve a stored-variable placeholder so a stale validator can be replayed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
stephenfuqua
force-pushed
the
etag-content-version
branch
from
65f2c45 to
0bc8bd1
Compare
Summary
Changes the redesigned relational backend's
_etagfrom a SHA-256 content hash to a composed strong validator, implementingreference/adr-etag-from-content-version.md(accepted 2026-07-03).ContentVersion=dms.Document.ContentVersion(opaque string, never parsed numerically).schemaEpoch= first 8 hex of the in-forceEffectiveSchemaHash;format=j(JSON registry);profileCode=_or 8-hex SHA-256 of the profile name;linkFlag=l/nfromResourceLinks:Enabled.Why
The old hash could not be computed from the request body (persisted state differs after merges/cascades/normalization), so every write did a full hydrate-materialize-hash readback inside the write transaction solely to populate a response header. Composition removes that readback entirely —
_etagis now a string built from a stored counter plus precomputed representation tokens, with no DB dependency.Behavioral changes
ETagis served quoted, neverW/. Different representations of the same state (readable profile, links on/off) now carry different_etagvalues — a deliberate reversal of the prior profile/link-insensitive contract, per the ADR.If-Matchcompares the state-significant projection only (ContentVersion,schemaEpoch,profileCode);formatandlinkFlagare excluded, so they never cause a spurious412. A cross-profileIf-Matchdoes412even at the sameContentVersion.412-ing profiled updates.Scope
Etag/building blocks (EtagComposer,VariantKey/VariantKeyFactory,ProfileVariantCode,EtagMatchProjection,EtagValue,DescriptorVariantKey) with unit tests.ContentVersionstamp trigger: INSERT viaRETURNING/OUTPUT, UPDATE via follow-upSELECT).ResourceEtagFormatter,RelationalApiMetadataFormatter) and the write-path hash readback.update-tracking.md(owning contract + normativevariantKeyencoding),transactions-and-concurrency.md,flattening-reconstitution.md.Test Plan
etag.feature,ProfileFiltering.feature07–08) — updated + compile-verified;🤖 Generated with Claude Code