feat(database): add attribute schema tables and soft-delete triggers - #69
Open
Filip-sz-informed wants to merge 22 commits into
Open
feat(database): add attribute schema tables and soft-delete triggers#69Filip-sz-informed wants to merge 22 commits into
Filip-sz-informed wants to merge 22 commits into
Conversation
Filip-sz-informed
marked this pull request as ready for review
September 4, 2026 07:19
Contributor
✅ OSS Checks PassedAll tracked OSS checks passed in this run.
Results from commit d836ed7, view the full job summary ♻️ This comment has been updated with latest results. |
Repository tests need real Postgres to exercise the plpgsql soft-delete triggers and partial unique indexes on the policy attribute schema, which the project's shared H2 test profile cannot run. AbstractPostgresRepositoryTest boots a Postgres container per test class, applies the real Flyway migrations, and leaves the existing H2-backed test setup untouched. DPAV-3154
Maps the attribute_definition table (policy attribute vocabulary: namespace, name, data type, validation metadata) with a findByNamespaceAndName lookup. JSONB columns (allowed_values, classification) map as raw String via @JdbcTypeCode(SqlTypes.JSON) - this layer carries them opaquely rather than inventing a structured shape ahead of the service layer that will interpret them. Repository tests cover the lookup and the uq_attribute_definition__namespace_name uniqueness constraint. DPAV-3156
Maps the attribute_definition_scope table (which scopes a definition is bound to, whether required there, and its default value) with a findByAttributeDefinitionId lookup returning all bindings for a definition. Repository tests cover a definition bound to multiple scopes and the uq_attribute_definition_scope__definition_scope uniqueness constraint. DPAV-3157
Maps the attribute_value table with a findByAttributeDefinitionScopeIdAndEntityIdAndIsDeletedFalse lookup for the live value(s) recorded against a given scope binding and entity. entityId stays a plain Long column, not a JPA relationship - it is a polymorphic reference whose target table varies by scope, per the migration's comment. uq_attr_value_live is a partial unique index on (attribute_definition_scope_id, entity_id, value), so it only rejects an exact-duplicate live value - it does not by itself enforce a single live value per entity for single-valued attributes. Repository tests cover the duplicate-value rejection, that a distinct value for the same binding+entity is accepted, and that re-adding a duplicate value succeeds once the prior one is soft-deleted. DPAV-3158
Verifies the migration's five AFTER DELETE triggers (trg_organisation_attribute_value_soft_delete and its consumer, producer, product, and product_consumer counterparts): deleting an owning row soft-deletes its live attribute_value rows instead of leaving them orphaned, and deleting an entity with no attribute values is a no-op against attribute_value. DPAV-3159
…a JUnit extension CI's build job failed: the first several repository test classes each timed out acquiring a JDBC connection (CannotCreateTransactionException), while classes running later in the same job passed. Relying on @Testcontainers/@container to start the shared static container in each class's beforeAll raced against Spring building that class's ApplicationContext - some classes got a HikariCP pool built before the container was actually accepting TCP connections. Start the container in a static initializer instead, before any JUnit lifecycle callback runs for any subclass. Verified locally: all 5 repository test classes together (19 tests) now pass in ~9s using a single shared Spring context, versus repeatedly timing out over several minutes before. Full suite (296 tests) still green.
…itFields SonarCloud flagged 5.6% new-code duplication (gate: <=3%) - the is_deleted/created_at/created_by/updated_at/updated_by block was copy-pasted identically across AttributeDefinition, AttributeDefinitionScope, and AttributeValue. Extracted into a @MappedSuperclass all three now extend; Hibernate still maps the fields into each entity's own table exactly as before, so no schema or behavior change.
Trivy's security-scanning check flagged 3 CRITICAL CVEs (CVE-2026-65182, CVE-2026-65905, CVE-2026-68525) in tomcat-embed-core 10.1.55, the version Spring Boot 3.5.16 manages by default. Fixed upstream in 10.1.58; 10.1.58 itself isn't published to Maven Central, so pin to 10.1.59 (next available release, also fixed) via the tomcat.version override property Spring Boot's parent POM exposes for this. Not introduced by this branch - develop's last scan predates these CVEs being published to Trivy's DB and would fail the same way if rescanned today.
Add ComparisonOperator/Combinator/FilterNode/FilterCompilationException mirroring opa-pov's filter package (adapted to this project's package layout), the base vocabulary the upcoming Specification compiler will validate and compile caller filters against.
Add AttributeType, ResourceType/ResourceAttribute/ResourceDefinition, ConfigurationResourceRegistry (static fixed columns) and DynamicAttributeResolver (per-lookup resolution against the existing attribute_definition/attribute_definition_scope tables, so a newly registered attribute is filterable without a restart). One caller attribute name resolves through ConfigurationResourceRegistry.resolve regardless of whether it turns out to be a fixed column or a dynamic attribute. Adds AttributeDefinitionScopeRepository.findByAttributeDefinition_Id AndAttributeScope_CodeAndIsDeletedFalse to resolve a dynamic attribute's live scope binding in one query instead of joining attribute_scope at filter-compile time.
…r filters
Compile a validated FilterNode into a Spring Data JPA Specification: a
fixed attribute becomes a direct CriteriaBuilder predicate on the
entity path, a dynamic attribute becomes a correlated EXISTS subquery
against attribute_value scoped by the attribute's resolved
attribute_definition_scope.id (never a caller-supplied string), with
the value cast per the attribute's declared data_type.
Settled the JSONB extraction mechanism against a real Postgres
container: the originally-planned #>> '{}' / jsonb_extract_path_text
zero-path-element call isn't reachable through JPA's
CriteriaBuilder.function, so the compiler casts the column to text via
HibernateCriteriaBuilder.cast and unquotes STRING values with btrim
instead - documented in design.md alongside the simplified EXISTS
subquery (correlates on attribute_definition_scope_id directly, no
attribute_scope join needed inside the subquery).
Full suite (347 tests) green after this change.
…vice layers Extend ProducerRepository/ConsumerRepository with JpaSpecificationExecutor, overriding findAll(Specification) with an @EntityGraph so the filtered path fetch-joins products/productConsumers instead of N+1-loading them (verified via Hibernate statistics in a real-Postgres test). Add a Specification-accepting overload to ProducerService/ConsumerService (and their impls) that ANDs the caller's compiled filter with client-id scoping - ConfigurationProviderImpl only holds these service interfaces, not the repositories, so the filter has to cross that boundary too. Full suite (353 tests) green after this change.
… endpoints Add FilterRequestParser (JSON parse + 20-comparison cap, mirroring opa_poc.api.SearchRequest's cap) and a GlobalExceptionHandler mapping for FilterCompilationException: REQUEST origin -> 400 with the exception's own message (already scoped to only the caller-supplied attribute name), POLICY origin -> 500 with a generic body, detail server-side only. Extend ConfigurationController with an optional `filter` query param on both endpoints, and ConfigurationProvider/ConfigurationProviderImpl with a 3-arg overload (existing 2-arg methods delegate to it unchanged) that builds one Specification from producer_id/consumer_id and the compiled caller filter together, replacing the old in-memory id narrowing on that path. Updates existing ConfigurationProviderImplTest/ConfigurationController Test/ConfigurationPolicyEnforcementIntegrationTest mocks for the new service-layer Specification overloads - id-narrowing correctness now lives in SpecificationPredicateCompilerTest and ProducerConsumerSpecificationRepositoryTest instead of these mocked unit tests. Full suite (365 tests) green after this change.
Cover spec.md's core observable-behaviour requirements with real Postgres, the real Specification compiler, and real service/converter beans: filter on a fixed column, filter on a dynamically registered attribute, the client-scope boundary (a filter cannot widen access to another client's records), unfiltered behaviour is unchanged, and a newly-registered dynamic attribute is filterable without a restart. Exercises ProducerService/ConsumerService directly rather than through ConfigurationProviderImpl (pulls in unrelated certificate-validation/ product-consumer machinery) or over HTTP (no @SpringBootTest/full security-stack precedent exists anywhere in this codebase to build on) - documented in the test's class Javadoc as the narrowest real-Postgres slice that actually proves the new query path end-to-end. Full suite (370 tests) green after this change.
…nerics Karpathy-guidelines/simplify pass: FilterNode.Literal mirrored opa_poc.filter's policy-emitted constant predicate, but nothing in this change emits one (no policy-emitted row filter is in scope) - it was dead code with no caller ever constructing it. Dropped, along with its compiler switch case and the countComparisons case for it. Also drops SpecificationPredicateCompiler.buildComparison's unused AttributeType parameter and replaces its repeated fully-qualified jakarta.persistence.criteria.Expression references with a plain import - no behavior change. mvn clean verify (370 tests) and spotless:check both green after this change.
- Restore pre-existing behaviour whenever no caller filter is supplied: ConfigurationProviderImpl.getFilteredActiveProducers/ getFilteredConsumers now route through the old JOIN-FETCH-based service methods (not the new Specification/@EntityGraph path) for every no-filter request, including ones that still supply producer_id/consumer_id. The old JOIN FETCH is an implicit inner join and silently excludes a producer with zero products; the new @EntityGraph fetch is an outer join and would have started including it for every caller, not just new-filter users - confirmed empirically against real Postgres (1 row via the old path, 0 via the new one for a zero-product producer). - Reject a syntactically valid but semantically incomplete filter (a comparison missing "attribute"/"operator", a group missing "combinator", a bare JSON `null`, or a null element inside "nodes") in FilterRequestParser, instead of letting it throw an unhandled NullPointerException deeper in resolution/compilation. The @NotNull/@notblank annotations on the FilterNode records were never enforced - this project has no Bean Validation provider on the classpath - so readValue() alone doesn't catch these. - Reject neq/not_in against a multi-valued dynamic attribute: each Comparison compiles to one EXISTS subquery, so neq/not_in meant "EXISTS a value that doesn't match" (true as soon as any other value is present), not "does not have this value" as a caller would expect. eq/in keep their unambiguous "has a matching value" EXISTS semantics. - Extract the repeated `(root, query, cb) -> cb.equal(root.get(field), value)` Specification idiom (independently hand-rolled 3x) into Specifications.fieldEquals. Adds regression tests for all four: routing-decision unit tests, a real-Postgres test documenting the old-vs-new join semantics, FilterRequestParser null-validation cases, and multi-valued eq/neq compiler tests. Full suite (383 tests) green, spotless clean.
BigDecimal.equals() compares scale as well as value, so a validity of "0.00" was not recognised as the ZERO sentinel for "no expiry" and would incorrectly fall through to the granted-date/validity check. Flagged by SonarCloud (new_reliability_rating C, blocking PR #69's quality gate) as java:S9351.
- ConfigurationResourceRegistry: extract a fixed(name, jpaPath, type) helper so each fixed attribute's logical name is written once instead of twice (map key + constructor arg), removing the "description"/"active"/"orgId"/"scheduleType"/"scheduleExpression" duplicated-literal (S1192) smells. - SpecificationPredicateCompiler: drop the unnecessary raw Expression cast in the IN/NOT_IN branches (S1905) - Expression<?>.in(...) works directly. - FilterRequestParser.validate: use record deconstruction patterns instead of binding-then-accessor-calls (S6878). - FilterRequestParserTest: replace 4 near-identical rejection tests with one @ParameterizedTest (S5976). - SpecificationPredicateCompilerTest/DynamicAttributeResolverTest: extract the Specification/resolver construction out of each assertThatThrownBy lambda so only the one call that can actually throw remains inside it (S5778), and drop an unused local variable (S1854/S1481). - ConfigurationProviderImplTest: drop unnecessary eq(...) matchers around constant arguments (S6068) and extract an inline mock() call to a named local variable (S9016). None of these were quality-gate blocking (new_maintainability_rating was already A) - fixed because they were visible on the Sonar PR dashboard. Full suite (383 tests) green, spotless clean.
- Rename a local "resolver" var to "underTest" in DynamicAttributeResolverTest (java:S1117 - it shadowed the field of the same name). - Add positive SpecificationPredicateCompiler tests for neq/in/ not_in/lt/lte/gte/contains against fixed columns - only eq/gt had positive coverage before, leaving most of the operator switch in buildComparison untested. - Add the CONSUMER analogue of the producer filter-present routing test, closing the two uncovered lines in ConfigurationProviderImpl.getFilteredConsumers's filter-present branch (the producer branch was already covered). Full suite (389 tests) green, spotless clean.
|
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
1 participant
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.
Sensitive Credential Checks
Motivation and Context
DPAV-3018 ("Policy-Aware Product Discovery") needs a place to store dynamic policy
attributes for organisations, consumers, producers, products, and subscriptions before
any service can register attribute definitions or resolve/write attribute values. This
PR delivers that in two parts: the Flyway migration (DPAV-3150) and the JPA
entity/repository layer on top of it, so downstream service/API work has a persistence
layer to build against.
Description
Migration (DPAV-3150):
V20260902120000__add_attribute_schema_tables.sqlcreatingattribute_scope,attribute_definition,attribute_definition_scope, andattribute_value, with uniqueness constraints, indexes, and fiveAFTER DELETEtriggers that soft-delete
attribute_valuerows when their owningorganisation/consumer/producer/product/product_consumer row is deleted.
attribute_scopeseeded with one row per core entity type(
ORGANISATION/CONSUMER/PRODUCER/PRODUCT/SUBSCRIPTION).Repository layer (DPAV-3154–DPAV-3160):
(
AttributeScope,AttributeDefinition,AttributeDefinitionScope,AttributeValue), following the project's existing entity conventions. Noentity↔DTO converters, services, or controllers — those depend on API/service
requirements not yet specified and belong to a later DPAV-3018 slice.
allowed_values,classification,default_value,value) map asraw
Stringvia@JdbcTypeCode(SqlTypes.JSON)— carried opaquely rather thaninventing a structured shape ahead of the service layer that will interpret them.
AttributeValue.entityIdis a plainLong, not a JPA relationship, since it's apolymorphic reference whose target table varies by scope.
AbstractPostgresRepositoryTestbase (Testcontainers) so repository tests runagainst real Postgres with the real Flyway migration applied — the project's shared
H2 test profile can't run
plpgsqltriggers or the schema's partial unique index.Scoped to the new tests only; the existing H2-backed suite is untouched.
uq_attr_value_live: it's a partial unique index on(attribute_definition_scope_id, entity_id, value), so it's an idempotency guardagainst an exact-duplicate live value, not a general single-valuedness constraint —
it does not by itself stop a different second value being recorded for the same
entity when
attribute_definition.multi_valued = false. That enforcement needs tojoin
attribute_definitionagainstattribute_valueand is left to the servicelayer that writes these rows next (see
design.mdin the archived change for thefull writeup).
docs/DATABASE_SCHEMA.mdupdated with the four new tables, their columns/keys, thefive soft-delete triggers, and the extended ER diagram (closes DPAV-3150 acceptance
criterion 7).
Full proposal/design/task breakdown:
openspec/changes/archive/2026-09-04-add-policy-attribute-repository-layer/.How Has This Been Tested?
@DataJpaTest+ Testcontainers repository tests (19 test methods across 5classes) covering:
findByCode,findByNamespaceAndName,findByAttributeDefinitionId,findByAttributeDefinitionScopeIdAndEntityIdAndIsDeletedFalse)uq_attribute_scope__code,uq_attribute_definition__namespace_name,uq_attribute_definition_scope__definition_scope,uq_attr_value_live)deletes each soft-delete their scoped
attribute_valuerows; a delete with noattribute values is a no-op against
attribute_value)mvn testrun against the full existing suite (277 tests) to confirm the H2-backedtests are unaffected by the new Testcontainers dependency/base class.
mvn compile/test-compileclean; Spotless applied.Screenshots (if appropriate):
N/A — persistence layer only, no UI/API surface in this PR.
Checklist: