feat: Enforce relation docID types - #4833
Draft
edjroz wants to merge 8 commits into
Draft
Conversation
edjroz
force-pushed
the
feat/enforce-relation-docid-types
branch
from
June 2, 2026 15:39
84492be to
26d5cd5
Compare
Adds referential integrity enforcement for relation fields on AddDocument and UpdateDocument mutations. When a `_<relation>ID` field is set, the referenced document must exist in the correct target collection; otherwise a `relation target document not found` error is returned.
Adds post-merge relation validation in executeMerge. After mergeComposites, each merged document's primary DocID relation fields are checked against the target collection. A missing target document is treated as a skip (the referenced doc may not have arrived yet via P2P) so existing sync behaviour is preserved.
Callers should not be able to link to documents they cannot read. These tests are currently failing — they document the expected behaviour before the implementation is in place. Also adds DocMap support to UpdateDoc so relation fields can reference dynamically assigned DocIDs without hardcoding strings.
A caller who cannot read a document may not link to it via a relation field. validateRelationDocIDs now calls checkAccessOfDocWithACP after confirming existence, returning ErrRelationTargetNotFound on access denial. The shared ACP DAC fixture removes the now-invalid public-employee-to-private-company link.
Covers docExistsAndNotDeleted, validateRelationDocIDs, and validateMergeRelationDocIDs with real in-memory Badger, no mocks.
…tion Covers UpdateWithFilter rejecting non-existent and soft-deleted relation targets, plus transaction isolation scenarios for the relation validator.
Asserts that a merge succeeds when the relation target is absent on the receiving node — validateMergeRelationDocIDs skips rather than errors.
edjroz
force-pushed
the
feat/enforce-relation-docid-types
branch
from
June 10, 2026 20:15
26d5cd5 to
9ec1eae
Compare
…ement Make setupEmployeeCompanyDB return concrete *collection so tests reach package-private methods without per-call forcetypeassert. Suppress SA4006 on validateMergeRelationDocIDs's exists check (placeholder for ACP-on-merge follow-up). Apply gofmt to two test files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Resolves #4811
Description
Scope
Enforces that a
_<relation>IDfield must point to a document that (a) exists and has not been soft-deleted in the declared target collection, and (b) is readable by the caller under ACP. Prior to thischange, any arbitrary DocID string — including one from the wrong collection, a deleted document, or a private document — was silently accepted.
The fix is applied in two paths:
AddDocument/UpdateDocumentreturn a hard error if validation fails.mergeComposites; a missing target is treated as a skip (it may arrive later) rather than an error.Entry Points
validateRelationDocIDscollection.add(),collection.update()validateMergedRelationDocIDsmergeProcessor.executeMerge()skipRelationValidationContextbasicImportAlgorithm — Local Mutation Validation (Write Path)
FieldKind_DocIDrelation field.GetRelatedCollection; skip if unknown locally.ErrRelationTargetNotFoundif not.ErrRelationTargetNotFoundif denied (same error — avoids leaking that the document exists but is private).Algorithm — P2P Merge Validation (Sync Path)
Outer loop (
validateMergedRelationDocIDs):validateMergeRelationDocIDsfor per-field validation.Per-document field walk (
validateMergeRelationDocIDs):Out of Scope
CollectionIDin their hash, a DocID from collection A cannot produce a valid DocID for collection B, so this cannot happen from an honest peer. Detecting it for a malicious peer requires scanning all known collections and a pending-marker store — deferred.Local Mutation Validation (Write Path)
validateRelationDocIDs— validates all dirty primaryFieldKind_DocIDfields on everyAddDocument/UpdateDocumentcalldocExistsAndNotDeleted— low-level existence check that bypasses ACP to confirm the target document exists regardless of caller permissionscheckAccessOfDocWithACP— a caller who cannot read the target document cannot link to itskipRelationValidationContext— allows backup import to bypass validation for cross-collection forward referencesErrRelationTargetNotFounderrorP2P Merge Validation (Sync Path)
validateMergedRelationDocIDsonmergeProcessor— runs aftermergeComposites, beforetxn.Commit()validateMergeRelationDocIDs— same field-walk as the write path but treats a missing target as a skip rather than an errorTests
ExpectedError(relation target not found)AddDocsequences in 8 delete/transaction tests to satisfy the new creation-order invariantTestBackupSelfRefImport_SplitPrimaryRelationWithSecondCollection_NoErrorto useDocIndexinstead of a hardcoded stale DocIDTestP2POneToManyPeerWithAddUpdateRelationValidation_NoErrorfor the sync path happy pathDocMapsupport toUpdateDocaction for dynamic DocID substitution in update testsHow has this been tested?
Test changes fall into five categories:
1. Red → ExpectedError — tests that previously passed because the bug allowed dangling relation references; they now assert the new validation error.
TestMutationAddOneToOne_WithNonExistentRelation_ErrorTestMutationAddOneToOne_WithWrongTypeRelation_ErrorTestMutationAddOneToOne_UseAliasWithNonExistingRelationPrimarySide_ErrorTestMutationUpdateOneToOne_WithNonExistentRelation_ErrorTestMutationAddOneToMany_NonExistingRelationManySide_ErrorTestMutationAddOneToMany_AliasedRelationNameNonExistingRelationManySide_Error2. Dependency reordering — tests whose
AddDocsequence created a referencing document before its target; reordered to satisfy the new invariant.TestRelationalDeletionOfADocumentUsingSingleKey_Success(andWithAlias,WithMultipleDocumentsWithAliasvariants)TestTxnDeletionOfRelatedDoc…(two variants)TestATxnCanReadARecord…(two variants)3. DocIndex substitution — a test that used a hardcoded Book DocID now uses
DocIndexfor dynamic resolution.TestBackupSelfRefImport_SplitPrimaryRelationWithSecondCollection_NoError4. New ACP mutation tests — seven new tests covering the ACP read-permission check on relation fields.
TestACP_MutationAdd_RelationTarget_PrivateDoc_NoIdentity_ErrorTestACP_MutationAdd_RelationTarget_PrivateDoc_WrongIdentity_ErrorTestACP_MutationAdd_RelationTarget_PrivateDoc_OwnerIdentity_NoErrorTestACP_MutationAdd_RelationTarget_PrivateDoc_GrantedRead_NoErrorTestACP_MutationAdd_RelationTarget_PublicDoc_NoIdentity_NoErrorTestACP_MutationUpdate_RelationTarget_PrivateDoc_NoIdentity_ErrorTestACP_MutationUpdate_RelationTarget_PrivateDoc_OwnerIdentity_NoError5. New P2P test — verifies that the sync-path validation does not break normal P2P sync when the relation target is present on the receiving node.
TestP2POneToManyPeerWithAddUpdateRelationValidation_NoErrorTested on: