From b02ab70b8181ef5a20f59a4eb535d8c48e3c9eb7 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 08:59:41 +0200 Subject: [PATCH 01/22] feat(database): add attribute schema tables and soft-delete triggers --- ...902120000__add_attribute_schema_tables.sql | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 src/main/resources/db/migration/V20260902120000__add_attribute_schema_tables.sql diff --git a/src/main/resources/db/migration/V20260902120000__add_attribute_schema_tables.sql b/src/main/resources/db/migration/V20260902120000__add_attribute_schema_tables.sql new file mode 100644 index 0000000..2034188 --- /dev/null +++ b/src/main/resources/db/migration/V20260902120000__add_attribute_schema_tables.sql @@ -0,0 +1,124 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +-- Which core entity types may carry dynamic attributes, and the table entity_id resolves against. +CREATE TABLE attribute_scope ( + id BIGSERIAL PRIMARY KEY, + code VARCHAR(50) NOT NULL, + table_name VARCHAR(150) NOT NULL, + description VARCHAR(500), + CONSTRAINT uq_attribute_scope__code UNIQUE (code) +); + +-- Attribute vocabulary: name/type/validation metadata, independent of which scope(s) it applies to. +CREATE TABLE attribute_definition ( + id BIGSERIAL PRIMARY KEY, + namespace VARCHAR(150) NOT NULL, + name VARCHAR(150) NOT NULL, + display_name VARCHAR(255), + description TEXT NOT NULL, + data_type VARCHAR(50) NOT NULL, + multi_valued BOOLEAN NOT NULL DEFAULT FALSE, + allowed_values JSONB, + validation_pattern VARCHAR(500), + classification JSONB, + sensitive BOOLEAN NOT NULL DEFAULT FALSE, + is_deleted BOOLEAN NOT NULL DEFAULT FALSE, + created_at TIMESTAMP NOT NULL DEFAULT now(), + created_by VARCHAR(255) NOT NULL, + updated_at TIMESTAMP, + updated_by VARCHAR(255), + CONSTRAINT uq_attribute_definition__namespace_name UNIQUE (namespace, name) +); + +-- Which scopes a definition is valid on, whether required there, and its default. +CREATE TABLE attribute_definition_scope ( + id BIGSERIAL PRIMARY KEY, + attribute_definition_id BIGINT NOT NULL, + attribute_scope_id BIGINT NOT NULL, + required BOOLEAN NOT NULL DEFAULT FALSE, + default_value JSONB, + is_deleted BOOLEAN NOT NULL DEFAULT FALSE, + created_at TIMESTAMP NOT NULL DEFAULT now(), + created_by VARCHAR(255) NOT NULL, + updated_at TIMESTAMP, + updated_by VARCHAR(255), + CONSTRAINT fk_attribute_definition_scope__attribute_definition_id + FOREIGN KEY (attribute_definition_id) REFERENCES attribute_definition (id), + CONSTRAINT fk_attribute_definition_scope__attribute_scope_id + FOREIGN KEY (attribute_scope_id) REFERENCES attribute_scope (id), + CONSTRAINT uq_attribute_definition_scope__definition_scope + UNIQUE (attribute_definition_id, attribute_scope_id) +); +CREATE INDEX idx_attribute_definition_scope__attribute_definition_id + ON attribute_definition_scope (attribute_definition_id); +CREATE INDEX idx_attribute_definition_scope__attribute_scope_id + ON attribute_definition_scope (attribute_scope_id); + +-- Actual values. entity_id is polymorphic: PK of the row in attribute_scope.table_name for that pairing's +-- scope, not a declared FK — enforced by the soft-delete trigger below, not by the database schema. +CREATE TABLE attribute_value ( + id BIGSERIAL PRIMARY KEY, + attribute_definition_scope_id BIGINT NOT NULL, + entity_id BIGINT NOT NULL, + value JSONB NOT NULL, + is_deleted BOOLEAN NOT NULL DEFAULT FALSE, + created_at TIMESTAMP NOT NULL DEFAULT now(), + created_by VARCHAR(255) NOT NULL, + updated_at TIMESTAMP, + updated_by VARCHAR(255), + CONSTRAINT fk_attribute_value__attribute_definition_scope_id + FOREIGN KEY (attribute_definition_scope_id) REFERENCES attribute_definition_scope (id) +); +CREATE INDEX idx_attribute_value__entity_id ON attribute_value (entity_id); +-- Backs the PEP's EXISTS sub-queries per constraint (attribute_definition_scope_id, entity_id, value). +CREATE UNIQUE INDEX uq_attr_value_live + ON attribute_value (attribute_definition_scope_id, entity_id, value) + WHERE is_deleted = FALSE; + +INSERT INTO attribute_scope (code, table_name, description) VALUES + ('ORGANISATION', 'organisation', 'Attributes carried by an organisation'), + ('CONSUMER', 'consumer', 'Attributes carried by a consumer'), + ('PRODUCER', 'producer', 'Attributes carried by a producer'), + ('PRODUCT', 'product', 'Attributes carried by a product'), + ('SUBSCRIPTION', 'product_consumer', 'Attributes carried by a product/consumer subscription'); + +-- Soft-delete any live attribute_value rows for the entity being removed, scoped to the deleted table. +CREATE FUNCTION fn_attribute_value_soft_delete_on_entity_delete() RETURNS TRIGGER AS $$ +BEGIN + UPDATE attribute_value av + SET is_deleted = TRUE, + updated_at = now(), + updated_by = 'trigger:' || TG_TABLE_NAME + FROM attribute_definition_scope ads + JOIN attribute_scope asc_ ON asc_.id = ads.attribute_scope_id + WHERE av.attribute_definition_scope_id = ads.id + AND asc_.table_name = TG_TABLE_NAME + AND av.entity_id = OLD.id + AND av.is_deleted = FALSE; + RETURN OLD; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER trg_organisation_attribute_value_soft_delete + AFTER DELETE ON organisation + FOR EACH ROW EXECUTE FUNCTION fn_attribute_value_soft_delete_on_entity_delete(); + +CREATE TRIGGER trg_consumer_attribute_value_soft_delete + AFTER DELETE ON consumer + FOR EACH ROW EXECUTE FUNCTION fn_attribute_value_soft_delete_on_entity_delete(); + +CREATE TRIGGER trg_producer_attribute_value_soft_delete + AFTER DELETE ON producer + FOR EACH ROW EXECUTE FUNCTION fn_attribute_value_soft_delete_on_entity_delete(); + +CREATE TRIGGER trg_product_attribute_value_soft_delete + AFTER DELETE ON product + FOR EACH ROW EXECUTE FUNCTION fn_attribute_value_soft_delete_on_entity_delete(); + +CREATE TRIGGER trg_product_consumer_attribute_value_soft_delete + AFTER DELETE ON product_consumer + FOR EACH ROW EXECUTE FUNCTION fn_attribute_value_soft_delete_on_entity_delete(); From 354467d75d3a48f26da482ef07027faf69209288 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:11:48 +0200 Subject: [PATCH 02/22] test(persistency): add Testcontainers Postgres base for repository tests 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 --- pom.xml | 10 +++++ .../AbstractPostgresRepositoryTest.java | 44 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AbstractPostgresRepositoryTest.java diff --git a/pom.xml b/pom.xml index e28a186..ada73cc 100644 --- a/pom.xml +++ b/pom.xml @@ -197,6 +197,16 @@ commons-lang3 3.18.0 + + org.testcontainers + postgresql + test + + + org.testcontainers + junit-jupiter + test + diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AbstractPostgresRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AbstractPostgresRepositoryTest.java new file mode 100644 index 0000000..d39169a --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AbstractPostgresRepositoryTest.java @@ -0,0 +1,44 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase; +import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; +import org.testcontainers.containers.PostgreSQLContainer; +import org.testcontainers.junit.jupiter.Container; +import org.testcontainers.junit.jupiter.Testcontainers; +import org.testcontainers.utility.DockerImageName; + +/** + * Base class for repository tests that need the real Flyway migrations and real + * Postgres behaviour (partial unique indexes, {@code plpgsql} triggers) that the + * project's shared H2 test profile ({@code src/test/resources/application.yml}) cannot + * provide. Starts a Postgres container per test class, points the Spring context at + * it, and re-enables Flyway (disabled in the shared profile) so migrations apply for + * real. + */ +@DataJpaTest +@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE) +@Testcontainers +public abstract class AbstractPostgresRepositoryTest { + + @Container + static final PostgreSQLContainer POSTGRES = + new PostgreSQLContainer<>(DockerImageName.parse("postgres:16-alpine")); + + @DynamicPropertySource + static void datasourceProperties(DynamicPropertyRegistry registry) { + registry.add("spring.datasource.url", POSTGRES::getJdbcUrl); + registry.add("spring.datasource.username", POSTGRES::getUsername); + registry.add("spring.datasource.password", POSTGRES::getPassword); + registry.add("spring.datasource.driver-class-name", POSTGRES::getDriverClassName); + registry.add("spring.jpa.properties.hibernate.dialect", () -> "org.hibernate.dialect.PostgreSQLDialect"); + registry.add("spring.flyway.enabled", () -> "true"); + } +} From b5f86615a3d9d861c7c69e116343a2e1deb754d1 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:11:53 +0200 Subject: [PATCH 03/22] feat(persistency): add AttributeScope entity and repository Maps the attribute_scope table (which core entity types may carry dynamic policy attributes, per DPAV-3150's migration) with a findByCode lookup, plus repository tests covering the seeded scope rows and the uq_attribute_scope__code uniqueness constraint. DPAV-3155 --- .../persistency/entity/AttributeScope.java | 38 +++++++++++++++ .../repository/AttributeScopeRepository.java | 26 ++++++++++ .../AttributeScopeRepositoryTest.java | 47 +++++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeScope.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeScopeRepository.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeScopeRepositoryTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeScope.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeScope.java new file mode 100644 index 0000000..eb8f553 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeScope.java @@ -0,0 +1,38 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.entity; + +import jakarta.persistence.*; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Size; +import lombok.Getter; +import lombok.Setter; + +@Getter +@Setter +@Entity +@Table(name = "attribute_scope") +public class AttributeScope { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + @Column(name = "id", nullable = false) + private Long id; + + @Size(max = 50) + @NotNull + @Column(name = "code", nullable = false, length = 50) + private String code; + + @Size(max = 150) + @NotNull + @Column(name = "table_name", nullable = false, length = 150) + private String tableName; + + @Size(max = 500) + @Column(name = "description", length = 500) + private String description; +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeScopeRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeScopeRepository.java new file mode 100644 index 0000000..21ac1a9 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeScopeRepository.java @@ -0,0 +1,26 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import java.util.Optional; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.stereotype.Repository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; + +/** + * Repository interface for managing {@link AttributeScope} entities. + * Provides persistence operations and query methods for interacting with the + * underlying database. + * + * Extends {@link JpaRepository} to inherit standard CRUD operations and adds + * query methods specific to {@link AttributeScope}. + */ +@Repository +public interface AttributeScopeRepository extends JpaRepository { + + Optional findByCode(String code); +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeScopeRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeScopeRepositoryTest.java new file mode 100644 index 0000000..a76c944 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeScopeRepositoryTest.java @@ -0,0 +1,47 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.Optional; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.dao.DataIntegrityViolationException; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; + +class AttributeScopeRepositoryTest extends AbstractPostgresRepositoryTest { + + @Autowired + private AttributeScopeRepository attributeScopeRepository; + + @Test + void findByCode_returnsSeededScope() { + Optional found = attributeScopeRepository.findByCode("PRODUCT"); + + assertThat(found).isPresent(); + assertThat(found.get().getTableName()).isEqualTo("product"); + } + + @Test + void findByCode_returnsEmptyForUnknownCode() { + Optional found = attributeScopeRepository.findByCode("DOES_NOT_EXIST"); + + assertThat(found).isEmpty(); + } + + @Test + void save_rejectsDuplicateCode() { + AttributeScope duplicate = new AttributeScope(); + duplicate.setCode("PRODUCT"); + duplicate.setTableName("product"); + duplicate.setDescription("Duplicate of the seeded PRODUCT scope"); + + assertThrows(DataIntegrityViolationException.class, () -> attributeScopeRepository.saveAndFlush(duplicate)); + } +} From 15ef6900febf43711b5a1d37120bd2214a8599c1 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:12:01 +0200 Subject: [PATCH 04/22] feat(persistency): add AttributeDefinition entity and repository 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 --- .../entity/AttributeDefinition.java | 90 +++++++++++++++++++ .../AttributeDefinitionRepository.java | 26 ++++++ .../AttributeDefinitionRepositoryTest.java | 64 +++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinition.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionRepository.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionRepositoryTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinition.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinition.java new file mode 100644 index 0000000..d1e351a --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinition.java @@ -0,0 +1,90 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.entity; + +import jakarta.persistence.*; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Size; +import java.sql.Timestamp; +import lombok.Getter; +import lombok.Setter; +import org.hibernate.annotations.JdbcTypeCode; +import org.hibernate.type.SqlTypes; + +@Getter +@Setter +@Entity +@Table(name = "attribute_definition") +public class AttributeDefinition { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + @Column(name = "id", nullable = false) + private Long id; + + @Size(max = 150) + @NotNull + @Column(name = "namespace", nullable = false, length = 150) + private String namespace; + + @Size(max = 150) + @NotNull + @Column(name = "name", nullable = false, length = 150) + private String name; + + @Size(max = 255) + @Column(name = "display_name", length = 255) + private String displayName; + + @NotNull + @Column(name = "description", nullable = false) + private String description; + + @Size(max = 50) + @NotNull + @Column(name = "data_type", nullable = false, length = 50) + private String dataType; + + @NotNull + @Column(name = "multi_valued", nullable = false) + private Boolean multiValued = false; + + @JdbcTypeCode(SqlTypes.JSON) + @Column(name = "allowed_values") + private String allowedValues; + + @Size(max = 500) + @Column(name = "validation_pattern", length = 500) + private String validationPattern; + + @JdbcTypeCode(SqlTypes.JSON) + @Column(name = "classification") + private String classification; + + @NotNull + @Column(name = "sensitive", nullable = false) + private Boolean sensitive = false; + + @NotNull + @Column(name = "is_deleted", nullable = false) + private Boolean isDeleted = false; + + @NotNull + @Column(name = "created_at", nullable = false) + private Timestamp createdAt; + + @Size(max = 255) + @NotNull + @Column(name = "created_by", nullable = false, length = 255) + private String createdBy; + + @Column(name = "updated_at") + private Timestamp updatedAt; + + @Size(max = 255) + @Column(name = "updated_by", length = 255) + private String updatedBy; +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionRepository.java new file mode 100644 index 0000000..a740c6b --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionRepository.java @@ -0,0 +1,26 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import java.util.Optional; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.stereotype.Repository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; + +/** + * Repository interface for managing {@link AttributeDefinition} entities. + * Provides persistence operations and query methods for interacting with the + * underlying database. + * + * Extends {@link JpaRepository} to inherit standard CRUD operations and adds + * query methods specific to {@link AttributeDefinition}. + */ +@Repository +public interface AttributeDefinitionRepository extends JpaRepository { + + Optional findByNamespaceAndName(String namespace, String name); +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionRepositoryTest.java new file mode 100644 index 0000000..1ebdfb4 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionRepositoryTest.java @@ -0,0 +1,64 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.sql.Timestamp; +import java.time.Instant; +import java.util.Optional; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.dao.DataIntegrityViolationException; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; + +class AttributeDefinitionRepositoryTest extends AbstractPostgresRepositoryTest { + + @Autowired + private AttributeDefinitionRepository attributeDefinitionRepository; + + private static AttributeDefinition newDefinition(String namespace, String name) { + AttributeDefinition definition = new AttributeDefinition(); + definition.setNamespace(namespace); + definition.setName(name); + definition.setDescription("Test attribute definition"); + definition.setDataType("STRING"); + definition.setCreatedAt(Timestamp.from(Instant.now())); + definition.setCreatedBy("test"); + return definition; + } + + @Test + void findByNamespaceAndName_returnsPersistedDefinition() { + attributeDefinitionRepository.saveAndFlush(newDefinition("policy", "risk-tier")); + + Optional found = + attributeDefinitionRepository.findByNamespaceAndName("policy", "risk-tier"); + + assertThat(found).isPresent(); + assertThat(found.get().getDataType()).isEqualTo("STRING"); + assertThat(found.get().getMultiValued()).isFalse(); + assertThat(found.get().getSensitive()).isFalse(); + } + + @Test + void findByNamespaceAndName_returnsEmptyForUnknownPair() { + Optional found = attributeDefinitionRepository.findByNamespaceAndName("nope", "nope"); + + assertThat(found).isEmpty(); + } + + @Test + void save_rejectsDuplicateNamespaceAndName() { + attributeDefinitionRepository.saveAndFlush(newDefinition("policy", "duplicate-check")); + AttributeDefinition duplicate = newDefinition("policy", "duplicate-check"); + + assertThrows( + DataIntegrityViolationException.class, () -> attributeDefinitionRepository.saveAndFlush(duplicate)); + } +} From 7ed85157c98bf482151091d0af141165c7dd5b01 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:12:06 +0200 Subject: [PATCH 05/22] feat(persistency): add AttributeDefinitionScope entity and repository 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 --- .../entity/AttributeDefinitionScope.java | 65 ++++++++++++++ .../AttributeDefinitionScopeRepository.java | 26 ++++++ ...ttributeDefinitionScopeRepositoryTest.java | 88 +++++++++++++++++++ 3 files changed, 179 insertions(+) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinitionScope.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepository.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepositoryTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinitionScope.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinitionScope.java new file mode 100644 index 0000000..f0a8faa --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinitionScope.java @@ -0,0 +1,65 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.entity; + +import jakarta.persistence.*; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Size; +import java.sql.Timestamp; +import lombok.Getter; +import lombok.Setter; +import org.hibernate.annotations.JdbcTypeCode; +import org.hibernate.type.SqlTypes; + +@Getter +@Setter +@Entity +@Table(name = "attribute_definition_scope") +public class AttributeDefinitionScope { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + @Column(name = "id", nullable = false) + private Long id; + + @NotNull + @ManyToOne(fetch = FetchType.LAZY, optional = false) + @JoinColumn(name = "attribute_definition_id", nullable = false) + private AttributeDefinition attributeDefinition; + + @NotNull + @ManyToOne(fetch = FetchType.LAZY, optional = false) + @JoinColumn(name = "attribute_scope_id", nullable = false) + private AttributeScope attributeScope; + + @NotNull + @Column(name = "required", nullable = false) + private Boolean required = false; + + @JdbcTypeCode(SqlTypes.JSON) + @Column(name = "default_value") + private String defaultValue; + + @NotNull + @Column(name = "is_deleted", nullable = false) + private Boolean isDeleted = false; + + @NotNull + @Column(name = "created_at", nullable = false) + private Timestamp createdAt; + + @Size(max = 255) + @NotNull + @Column(name = "created_by", nullable = false, length = 255) + private String createdBy; + + @Column(name = "updated_at") + private Timestamp updatedAt; + + @Size(max = 255) + @Column(name = "updated_by", length = 255) + private String updatedBy; +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepository.java new file mode 100644 index 0000000..27104c5 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepository.java @@ -0,0 +1,26 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import java.util.List; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.stereotype.Repository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; + +/** + * Repository interface for managing {@link AttributeDefinitionScope} entities. + * Provides persistence operations and query methods for interacting with the + * underlying database. + * + * Extends {@link JpaRepository} to inherit standard CRUD operations and adds + * query methods specific to {@link AttributeDefinitionScope}. + */ +@Repository +public interface AttributeDefinitionScopeRepository extends JpaRepository { + + List findByAttributeDefinitionId(Long attributeDefinitionId); +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepositoryTest.java new file mode 100644 index 0000000..605dcb4 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepositoryTest.java @@ -0,0 +1,88 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.sql.Timestamp; +import java.time.Instant; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.dao.DataIntegrityViolationException; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; + +class AttributeDefinitionScopeRepositoryTest extends AbstractPostgresRepositoryTest { + + @Autowired + private AttributeDefinitionRepository attributeDefinitionRepository; + + @Autowired + private AttributeScopeRepository attributeScopeRepository; + + @Autowired + private AttributeDefinitionScopeRepository attributeDefinitionScopeRepository; + + private AttributeDefinition persistDefinition(String name) { + AttributeDefinition definition = new AttributeDefinition(); + definition.setNamespace("policy"); + definition.setName(name); + definition.setDescription("Test attribute definition"); + definition.setDataType("STRING"); + definition.setCreatedAt(Timestamp.from(Instant.now())); + definition.setCreatedBy("test"); + return attributeDefinitionRepository.saveAndFlush(definition); + } + + private static AttributeDefinitionScope newBinding( + AttributeDefinition definition, AttributeScope scope, boolean required) { + AttributeDefinitionScope binding = new AttributeDefinitionScope(); + binding.setAttributeDefinition(definition); + binding.setAttributeScope(scope); + binding.setRequired(required); + binding.setCreatedAt(Timestamp.from(Instant.now())); + binding.setCreatedBy("test"); + return binding; + } + + @Test + void findByAttributeDefinitionId_returnsAllBoundScopes() { + AttributeDefinition definition = persistDefinition("multi-scope-attr"); + AttributeScope productScope = + attributeScopeRepository.findByCode("PRODUCT").orElseThrow(); + AttributeScope consumerScope = + attributeScopeRepository.findByCode("CONSUMER").orElseThrow(); + + attributeDefinitionScopeRepository.saveAndFlush(newBinding(definition, productScope, true)); + attributeDefinitionScopeRepository.saveAndFlush(newBinding(definition, consumerScope, false)); + + List bindings = + attributeDefinitionScopeRepository.findByAttributeDefinitionId(definition.getId()); + + assertThat(bindings).hasSize(2); + assertThat(bindings) + .extracting(b -> b.getAttributeScope().getId()) + .containsExactlyInAnyOrder(productScope.getId(), consumerScope.getId()); + } + + @Test + void save_rejectsDuplicateDefinitionScopePair() { + AttributeDefinition definition = persistDefinition("duplicate-binding-attr"); + AttributeScope productScope = + attributeScopeRepository.findByCode("PRODUCT").orElseThrow(); + attributeDefinitionScopeRepository.saveAndFlush(newBinding(definition, productScope, false)); + + AttributeDefinitionScope duplicate = newBinding(definition, productScope, true); + + assertThrows( + DataIntegrityViolationException.class, + () -> attributeDefinitionScopeRepository.saveAndFlush(duplicate)); + } +} From 36eb44f09e80b13e73b129ef581a1d0a0ca5faa7 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:12:13 +0200 Subject: [PATCH 06/22] feat(persistency): add AttributeValue entity and repository 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 --- .../persistency/entity/AttributeValue.java | 67 +++++++++ .../repository/AttributeValueRepository.java | 27 ++++ .../AttributeValueRepositoryTest.java | 141 ++++++++++++++++++ 3 files changed, 235 insertions(+) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeValue.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueRepository.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueRepositoryTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeValue.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeValue.java new file mode 100644 index 0000000..888bba8 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeValue.java @@ -0,0 +1,67 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.entity; + +import jakarta.persistence.*; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Size; +import java.sql.Timestamp; +import lombok.Getter; +import lombok.Setter; +import org.hibernate.annotations.JdbcTypeCode; +import org.hibernate.type.SqlTypes; + +@Getter +@Setter +@Entity +@Table(name = "attribute_value") +public class AttributeValue { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + @Column(name = "id", nullable = false) + private Long id; + + @NotNull + @ManyToOne(fetch = FetchType.LAZY, optional = false) + @JoinColumn(name = "attribute_definition_scope_id", nullable = false) + private AttributeDefinitionScope attributeDefinitionScope; + + /** + * Polymorphic reference: the primary key of the row in the table named by + * {@code attributeDefinitionScope.attributeScope.tableName}. Not a JPA relationship + * because the target entity type varies by scope; see the migration's soft-delete + * trigger for how this is enforced at the database level. + */ + @NotNull + @Column(name = "entity_id", nullable = false) + private Long entityId; + + @NotNull + @JdbcTypeCode(SqlTypes.JSON) + @Column(name = "value", nullable = false) + private String value; + + @NotNull + @Column(name = "is_deleted", nullable = false) + private Boolean isDeleted = false; + + @NotNull + @Column(name = "created_at", nullable = false) + private Timestamp createdAt; + + @Size(max = 255) + @NotNull + @Column(name = "created_by", nullable = false, length = 255) + private String createdBy; + + @Column(name = "updated_at") + private Timestamp updatedAt; + + @Size(max = 255) + @Column(name = "updated_by", length = 255) + private String updatedBy; +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueRepository.java new file mode 100644 index 0000000..d6365b9 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueRepository.java @@ -0,0 +1,27 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import java.util.List; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.stereotype.Repository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeValue; + +/** + * Repository interface for managing {@link AttributeValue} entities. + * Provides persistence operations and query methods for interacting with the + * underlying database. + * + * Extends {@link JpaRepository} to inherit standard CRUD operations and adds + * query methods specific to {@link AttributeValue}. + */ +@Repository +public interface AttributeValueRepository extends JpaRepository { + + List findByAttributeDefinitionScopeIdAndEntityIdAndIsDeletedFalse( + Long attributeDefinitionScopeId, Long entityId); +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueRepositoryTest.java new file mode 100644 index 0000000..2a24d25 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueRepositoryTest.java @@ -0,0 +1,141 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.sql.Timestamp; +import java.time.Instant; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.dao.DataIntegrityViolationException; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeValue; + +class AttributeValueRepositoryTest extends AbstractPostgresRepositoryTest { + + @Autowired + private AttributeDefinitionRepository attributeDefinitionRepository; + + @Autowired + private AttributeScopeRepository attributeScopeRepository; + + @Autowired + private AttributeDefinitionScopeRepository attributeDefinitionScopeRepository; + + @Autowired + private AttributeValueRepository attributeValueRepository; + + private AttributeDefinitionScope persistProductScopedBinding(String attributeName) { + AttributeDefinition definition = new AttributeDefinition(); + definition.setNamespace("policy"); + definition.setName(attributeName); + definition.setDescription("Test attribute definition"); + definition.setDataType("STRING"); + definition.setCreatedAt(Timestamp.from(Instant.now())); + definition.setCreatedBy("test"); + definition = attributeDefinitionRepository.saveAndFlush(definition); + + AttributeScope productScope = + attributeScopeRepository.findByCode("PRODUCT").orElseThrow(); + + AttributeDefinitionScope binding = new AttributeDefinitionScope(); + binding.setAttributeDefinition(definition); + binding.setAttributeScope(productScope); + binding.setRequired(false); + binding.setCreatedAt(Timestamp.from(Instant.now())); + binding.setCreatedBy("test"); + return attributeDefinitionScopeRepository.saveAndFlush(binding); + } + + private static AttributeValue newValue(AttributeDefinitionScope binding, Long entityId, String json) { + AttributeValue value = new AttributeValue(); + value.setAttributeDefinitionScope(binding); + value.setEntityId(entityId); + value.setValue(json); + value.setCreatedAt(Timestamp.from(Instant.now())); + value.setCreatedBy("test"); + return value; + } + + @Test + void findLiveValue_returnsNonDeletedValue() { + AttributeDefinitionScope binding = persistProductScopedBinding("live-value-attr"); + attributeValueRepository.saveAndFlush(newValue(binding, 1001L, "\"gold\"")); + + List live = + attributeValueRepository.findByAttributeDefinitionScopeIdAndEntityIdAndIsDeletedFalse( + binding.getId(), 1001L); + + assertThat(live).hasSize(1); + assertThat(live.get(0).getValue()).isEqualTo("\"gold\""); + } + + @Test + void findLiveValue_excludesSoftDeletedValue() { + AttributeDefinitionScope binding = persistProductScopedBinding("soft-deleted-attr"); + AttributeValue value = newValue(binding, 1002L, "\"silver\""); + value.setIsDeleted(true); + attributeValueRepository.saveAndFlush(value); + + List live = + attributeValueRepository.findByAttributeDefinitionScopeIdAndEntityIdAndIsDeletedFalse( + binding.getId(), 1002L); + + assertThat(live).isEmpty(); + } + + @Test + void save_rejectsExactDuplicateLiveValue() { + AttributeDefinitionScope binding = persistProductScopedBinding("duplicate-value-attr"); + attributeValueRepository.saveAndFlush(newValue(binding, 1003L, "\"gold\"")); + + AttributeValue duplicate = newValue(binding, 1003L, "\"gold\""); + + assertThrows(DataIntegrityViolationException.class, () -> attributeValueRepository.saveAndFlush(duplicate)); + } + + @Test + void save_acceptsDistinctValueForSameBindingAndEntity() { + // uq_attr_value_live keys on (scope, entity, value) - it is an idempotency guard against exact + // duplicates, not a single-valuedness constraint, so a different value is allowed. See design.md. + AttributeDefinitionScope binding = persistProductScopedBinding("multi-valued-attr"); + attributeValueRepository.saveAndFlush(newValue(binding, 1004L, "\"gold\"")); + + attributeValueRepository.saveAndFlush(newValue(binding, 1004L, "\"silver\"")); + + List live = + attributeValueRepository.findByAttributeDefinitionScopeIdAndEntityIdAndIsDeletedFalse( + binding.getId(), 1004L); + assertThat(live) + .hasSize(2) + .extracting(AttributeValue::getValue) + .containsExactlyInAnyOrder("\"gold\"", "\"silver\""); + } + + @Test + void save_allowsSameValueAgainAfterPriorDuplicateIsSoftDeleted() { + AttributeDefinitionScope binding = persistProductScopedBinding("resurrected-attr"); + AttributeValue first = newValue(binding, 1005L, "\"gold\""); + first = attributeValueRepository.saveAndFlush(first); + first.setIsDeleted(true); + attributeValueRepository.saveAndFlush(first); + + AttributeValue resurrected = newValue(binding, 1005L, "\"gold\""); + attributeValueRepository.saveAndFlush(resurrected); + + List live = + attributeValueRepository.findByAttributeDefinitionScopeIdAndEntityIdAndIsDeletedFalse( + binding.getId(), 1005L); + assertThat(live).hasSize(1); + assertThat(live.get(0).getValue()).isEqualTo("\"gold\""); + } +} From 2cd05b0f8829fa8ba316397bad99fd3fc8692630 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:12:20 +0200 Subject: [PATCH 07/22] test(persistency): cover soft-delete triggers for attribute_value 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 --- .../AttributeValueSoftDeleteTriggerTest.java | 235 ++++++++++++++++++ 1 file changed, 235 insertions(+) create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueSoftDeleteTriggerTest.java diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueSoftDeleteTriggerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueSoftDeleteTriggerTest.java new file mode 100644 index 0000000..c8ffe58 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueSoftDeleteTriggerTest.java @@ -0,0 +1,235 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.math.BigDecimal; +import java.sql.Timestamp; +import java.time.Instant; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.orm.jpa.TestEntityManager; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeValue; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Product; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.ProductConsumer; + +/** + * Verifies the migration's five {@code AFTER DELETE} triggers, which soft-delete + * {@code attribute_value} rows scoped to the deleted owning entity rather than + * leaving them orphaned. + */ +class AttributeValueSoftDeleteTriggerTest extends AbstractPostgresRepositoryTest { + + @Autowired + private OrganisationRepository organisationRepository; + + @Autowired + private ProducerRepository producerRepository; + + @Autowired + private ConsumerRepository consumerRepository; + + @Autowired + private ProductRepository productRepository; + + @Autowired + private ProductConsumerRepository productConsumerRepository; + + @Autowired + private AttributeScopeRepository attributeScopeRepository; + + @Autowired + private AttributeDefinitionRepository attributeDefinitionRepository; + + @Autowired + private AttributeDefinitionScopeRepository attributeDefinitionScopeRepository; + + @Autowired + private AttributeValueRepository attributeValueRepository; + + @Autowired + private TestEntityManager testEntityManager; + + private AttributeDefinitionScope bindingFor(String scopeCode, String attributeName) { + AttributeDefinition definition = new AttributeDefinition(); + definition.setNamespace("policy"); + definition.setName(attributeName); + definition.setDescription("Trigger test attribute definition"); + definition.setDataType("STRING"); + definition.setCreatedAt(Timestamp.from(Instant.now())); + definition.setCreatedBy("test"); + definition = attributeDefinitionRepository.saveAndFlush(definition); + + AttributeScope scope = attributeScopeRepository.findByCode(scopeCode).orElseThrow(); + + AttributeDefinitionScope binding = new AttributeDefinitionScope(); + binding.setAttributeDefinition(definition); + binding.setAttributeScope(scope); + binding.setRequired(false); + binding.setCreatedAt(Timestamp.from(Instant.now())); + binding.setCreatedBy("test"); + return attributeDefinitionScopeRepository.saveAndFlush(binding); + } + + private Long persistLiveValue(AttributeDefinitionScope binding, Long entityId) { + AttributeValue value = new AttributeValue(); + value.setAttributeDefinitionScope(binding); + value.setEntityId(entityId); + value.setValue("\"trigger-test-value\""); + value.setCreatedAt(Timestamp.from(Instant.now())); + value.setCreatedBy("test"); + return attributeValueRepository.saveAndFlush(value).getId(); + } + + private Organisation persistOrganisation() { + Organisation organisation = new Organisation(); + organisation.setName("Trigger Test Org"); + return organisationRepository.saveAndFlush(organisation); + } + + private Consumer persistConsumer(Organisation organisation) { + Consumer consumer = new Consumer(); + consumer.setName("Trigger Test Consumer"); + consumer.setOrg(organisation); + consumer.setIdpClientId("trigger-test-consumer"); + consumer.setScheduleType("cron"); + return consumerRepository.saveAndFlush(consumer); + } + + private Producer persistProducer(Organisation organisation) { + Producer producer = new Producer(); + producer.setName("Trigger Test Producer"); + producer.setDescription("Trigger test producer"); + producer.setOrg(organisation); + producer.setActive(true); + producer.setHost("localhost"); + producer.setPort(BigDecimal.valueOf(8080)); + producer.setTls(true); + producer.setIdpClientId("trigger-test-producer"); + return producerRepository.saveAndFlush(producer); + } + + private Product persistProduct(Producer producer) { + Product product = new Product(); + product.setName("Trigger Test Product"); + product.setTopic("topic.trigger-test"); + product.setProducer(producer); + return productRepository.saveAndFlush(product); + } + + private ProductConsumer persistProductConsumer(Product product, Consumer consumer) { + ProductConsumer productConsumer = new ProductConsumer(); + productConsumer.setProduct(product); + productConsumer.setConsumer(consumer); + productConsumer.setGrantedTs(Timestamp.from(Instant.now())); + productConsumer.setValidity(BigDecimal.valueOf(30)); + productConsumer.setScheduleType("cron"); + return productConsumerRepository.saveAndFlush(productConsumer); + } + + @Test + void deletingOrganisation_softDeletesItsAttributeValues() { + Organisation organisation = persistOrganisation(); + AttributeDefinitionScope binding = bindingFor("ORGANISATION", "org-trigger-attr"); + Long valueId = persistLiveValue(binding, organisation.getId()); + + organisationRepository.delete(organisation); + organisationRepository.flush(); + testEntityManager.clear(); + + assertThat(attributeValueRepository.findById(valueId)).isPresent().get().satisfies(v -> assertThat( + v.getIsDeleted()) + .isTrue()); + } + + @Test + void deletingConsumer_softDeletesItsAttributeValues() { + Organisation organisation = persistOrganisation(); + Consumer consumer = persistConsumer(organisation); + AttributeDefinitionScope binding = bindingFor("CONSUMER", "consumer-trigger-attr"); + Long valueId = persistLiveValue(binding, consumer.getId()); + + consumerRepository.delete(consumer); + consumerRepository.flush(); + testEntityManager.clear(); + + assertThat(attributeValueRepository.findById(valueId)).isPresent().get().satisfies(v -> assertThat( + v.getIsDeleted()) + .isTrue()); + } + + @Test + void deletingProducer_softDeletesItsAttributeValues() { + Organisation organisation = persistOrganisation(); + Producer producer = persistProducer(organisation); + AttributeDefinitionScope binding = bindingFor("PRODUCER", "producer-trigger-attr"); + Long valueId = persistLiveValue(binding, producer.getId()); + + producerRepository.delete(producer); + producerRepository.flush(); + testEntityManager.clear(); + + assertThat(attributeValueRepository.findById(valueId)).isPresent().get().satisfies(v -> assertThat( + v.getIsDeleted()) + .isTrue()); + } + + @Test + void deletingProduct_softDeletesItsAttributeValues() { + Organisation organisation = persistOrganisation(); + Producer producer = persistProducer(organisation); + Product product = persistProduct(producer); + AttributeDefinitionScope binding = bindingFor("PRODUCT", "product-trigger-attr"); + Long valueId = persistLiveValue(binding, product.getId()); + + productRepository.delete(product); + productRepository.flush(); + testEntityManager.clear(); + + assertThat(attributeValueRepository.findById(valueId)).isPresent().get().satisfies(v -> assertThat( + v.getIsDeleted()) + .isTrue()); + } + + @Test + void deletingProductConsumer_softDeletesItsAttributeValues() { + Organisation organisation = persistOrganisation(); + Producer producer = persistProducer(organisation); + Product product = persistProduct(producer); + Consumer consumer = persistConsumer(organisation); + ProductConsumer productConsumer = persistProductConsumer(product, consumer); + AttributeDefinitionScope binding = bindingFor("SUBSCRIPTION", "subscription-trigger-attr"); + Long valueId = persistLiveValue(binding, productConsumer.getId()); + + productConsumerRepository.delete(productConsumer); + productConsumerRepository.flush(); + testEntityManager.clear(); + + assertThat(attributeValueRepository.findById(valueId)).isPresent().get().satisfies(v -> assertThat( + v.getIsDeleted()) + .isTrue()); + } + + @Test + void deletingEntityWithNoAttributeValues_succeedsAndLeavesAttributeValueTableUntouched() { + Organisation organisation = persistOrganisation(); + long countBefore = attributeValueRepository.count(); + + organisationRepository.delete(organisation); + organisationRepository.flush(); + + assertThat(organisationRepository.existsById(organisation.getId())).isFalse(); + assertThat(attributeValueRepository.count()).isEqualTo(countBefore); + } +} From 8ad5a333dcecfe22996e0ebdfcd18694a68b10a5 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:12:26 +0200 Subject: [PATCH 08/22] docs(schema): document policy attribute schema tables Adds attribute_scope, attribute_definition, attribute_definition_scope, and attribute_value to DATABASE_SCHEMA.md - columns, keys, and the five soft-delete triggers - following the existing per-table format, and extends the ER diagram. Covers DPAV-3150 acceptance criterion 7. DPAV-3160 --- docs/DATABASE_SCHEMA.md | 148 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/docs/DATABASE_SCHEMA.md b/docs/DATABASE_SCHEMA.md index 440c889..efad6cf 100644 --- a/docs/DATABASE_SCHEMA.md +++ b/docs/DATABASE_SCHEMA.md @@ -39,6 +39,9 @@ erDiagram CONSUMER ||--o{ PRODUCT_CONSUMER : consumes PRODUCT_CONSUMER ||--o{ PRODUCT_CONSUMER_ATTRIBUTE : has PRODUCT_TYPE ||--o{ PRODUCT : categorizes + ATTRIBUTE_DEFINITION ||--o{ ATTRIBUTE_DEFINITION_SCOPE : "bound via" + ATTRIBUTE_SCOPE ||--o{ ATTRIBUTE_DEFINITION_SCOPE : "bound via" + ATTRIBUTE_DEFINITION_SCOPE ||--o{ ATTRIBUTE_VALUE : has ORGANISATION { BIGSERIAL id PK @@ -116,6 +119,53 @@ erDiagram TIMESTAMP event_time VARCHAR performed_by } + ATTRIBUTE_SCOPE { + BIGSERIAL id PK + VARCHAR code + VARCHAR table_name + VARCHAR description + } + ATTRIBUTE_DEFINITION { + BIGSERIAL id PK + VARCHAR namespace + VARCHAR name + VARCHAR display_name + TEXT description + VARCHAR data_type + BOOLEAN multi_valued + JSONB allowed_values + VARCHAR validation_pattern + JSONB classification + BOOLEAN sensitive + BOOLEAN is_deleted + TIMESTAMP created_at + VARCHAR created_by + TIMESTAMP updated_at + VARCHAR updated_by + } + ATTRIBUTE_DEFINITION_SCOPE { + BIGSERIAL id PK + BIGINT attribute_definition_id FK + BIGINT attribute_scope_id FK + BOOLEAN required + JSONB default_value + BOOLEAN is_deleted + TIMESTAMP created_at + VARCHAR created_by + TIMESTAMP updated_at + VARCHAR updated_by + } + ATTRIBUTE_VALUE { + BIGSERIAL id PK + BIGINT attribute_definition_scope_id FK + BIGINT entity_id + JSONB value + BOOLEAN is_deleted + TIMESTAMP created_at + VARCHAR created_by + TIMESTAMP updated_at + VARCHAR updated_by + } ``` --- @@ -284,6 +334,104 @@ Usage: --- +### attribute_scope +Which core entity types may carry dynamic policy attributes, and the table `attribute_value.entity_id` resolves against for that scope. + +Columns: +- `id` BIGSERIAL, primary key +- `code` VARCHAR(50), not null — unique scope identifier (e.g. `PRODUCT`) +- `table_name` VARCHAR(150), not null — the table `attribute_value.entity_id` is a row id in, for this scope +- `description` VARCHAR(500), nullable + +Constraints: +- UNIQUE on `code` (`uq_attribute_scope__code`) + +Usage: +- Seeded by migration with one row per core entity type: `ORGANISATION` (`organisation`), `CONSUMER` (`consumer`), `PRODUCER` (`producer`), `PRODUCT` (`product`), `SUBSCRIPTION` (`product_consumer`). +- Referenced by `attribute_definition_scope` to say which scopes an attribute definition applies to. + +--- + +### attribute_definition +Vocabulary of policy attributes: name, type, and validation metadata, independent of which scope(s) it applies to. + +Columns: +- `id` BIGSERIAL, primary key +- `namespace` VARCHAR(150), not null +- `name` VARCHAR(150), not null +- `display_name` VARCHAR(255), nullable +- `description` TEXT, not null +- `data_type` VARCHAR(50), not null +- `multi_valued` BOOLEAN, not null, default FALSE +- `allowed_values` JSONB, nullable +- `validation_pattern` VARCHAR(500), nullable +- `classification` JSONB, nullable +- `sensitive` BOOLEAN, not null, default FALSE +- `is_deleted` BOOLEAN, not null, default FALSE +- `created_at` TIMESTAMP, not null, default `now()` +- `created_by` VARCHAR(255), not null +- `updated_at` TIMESTAMP, nullable +- `updated_by` VARCHAR(255), nullable + +Constraints: +- UNIQUE on (`namespace`, `name`) (`uq_attribute_definition__namespace_name`) + +Usage: +- Defines the shape of a policy attribute (e.g. data type, whether it can hold multiple values, allowed values, sensitivity) independently of where it can be attached. + +--- + +### attribute_definition_scope +Which scopes an `attribute_definition` is valid on, whether required there, and its default value. + +Columns: +- `id` BIGSERIAL, primary key +- `attribute_definition_id` BIGINT, not null, foreign key → `attribute_definition(id)` +- `attribute_scope_id` BIGINT, not null, foreign key → `attribute_scope(id)` +- `required` BOOLEAN, not null, default FALSE +- `default_value` JSONB, nullable +- `is_deleted` BOOLEAN, not null, default FALSE +- `created_at` TIMESTAMP, not null, default `now()` +- `created_by` VARCHAR(255), not null +- `updated_at` TIMESTAMP, nullable +- `updated_by` VARCHAR(255), nullable + +Constraints: +- UNIQUE on (`attribute_definition_id`, `attribute_scope_id`) (`uq_attribute_definition_scope__definition_scope`) +- Index on `attribute_definition_id` (`idx_attribute_definition_scope__attribute_definition_id`) +- Index on `attribute_scope_id` (`idx_attribute_definition_scope__attribute_scope_id`) + +Usage: +- Binds a definition to one or more scopes, controlling per-scope requiredness and default. + +--- + +### attribute_value +Actual policy attribute values recorded against a specific entity. + +Columns: +- `id` BIGSERIAL, primary key +- `attribute_definition_scope_id` BIGINT, not null, foreign key → `attribute_definition_scope(id)` +- `entity_id` BIGINT, not null — polymorphic reference: the primary key of the row in the table named by the value's `attribute_scope.table_name`. Not a declared foreign key, since the target table varies by scope. +- `value` JSONB, not null +- `is_deleted` BOOLEAN, not null, default FALSE +- `created_at` TIMESTAMP, not null, default `now()` +- `created_by` VARCHAR(255), not null +- `updated_at` TIMESTAMP, nullable +- `updated_by` VARCHAR(255), nullable + +Constraints: +- Index on `entity_id` (`idx_attribute_value__entity_id`) +- Partial UNIQUE index on (`attribute_definition_scope_id`, `entity_id`, `value`) WHERE `is_deleted = FALSE` (`uq_attr_value_live`) — an idempotency guard against persisting an exact-duplicate live value; it does not by itself enforce "one live value per entity" for single-valued attributes (that check spans `attribute_definition.multi_valued` and is left to the service layer that writes these rows) + +Soft-delete triggers: +- `trg_organisation_attribute_value_soft_delete`, `trg_consumer_attribute_value_soft_delete`, `trg_producer_attribute_value_soft_delete`, `trg_product_attribute_value_soft_delete`, `trg_product_consumer_attribute_value_soft_delete` — one `AFTER DELETE` trigger per owning table (`organisation`, `consumer`, `producer`, `product`, `product_consumer`), all calling the shared function `fn_attribute_value_soft_delete_on_entity_delete()`. When a row in one of those tables is deleted, every live (`is_deleted = FALSE`) `attribute_value` row scoped to that table and entity id is set `is_deleted = TRUE` rather than deleted or left orphaned. + +Usage: +- Stores the actual attribute values used to build the OPA data bundle for policy decisions, keyed by which entity (organisation, consumer, producer, product, or subscription) they describe. + +--- + ## Migration Notes - Schema is versioned and applied with Flyway on application startup. - Foreign keys enforce referential integrity among core entities. From 5c4809f48f4e903ec6a617417f1c36dae037aa96 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:26:48 +0200 Subject: [PATCH 09/22] fix(persistency): start the shared Postgres container eagerly, not via 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. --- .../AbstractPostgresRepositoryTest.java | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AbstractPostgresRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AbstractPostgresRepositoryTest.java index d39169a..df49d13 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AbstractPostgresRepositoryTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AbstractPostgresRepositoryTest.java @@ -11,27 +11,36 @@ import org.springframework.test.context.DynamicPropertyRegistry; import org.springframework.test.context.DynamicPropertySource; import org.testcontainers.containers.PostgreSQLContainer; -import org.testcontainers.junit.jupiter.Container; -import org.testcontainers.junit.jupiter.Testcontainers; import org.testcontainers.utility.DockerImageName; /** * Base class for repository tests that need the real Flyway migrations and real * Postgres behaviour (partial unique indexes, {@code plpgsql} triggers) that the * project's shared H2 test profile ({@code src/test/resources/application.yml}) cannot - * provide. Starts a Postgres container per test class, points the Spring context at - * it, and re-enables Flyway (disabled in the shared profile) so migrations apply for - * real. + * provide. Points the Spring context at a shared Postgres container and re-enables + * Flyway (disabled in the shared profile) so migrations apply for real. + * + *

The container is started eagerly in a static initializer rather than left to the + * {@code @Testcontainers}/{@code @Container} JUnit extension. With multiple concrete + * subclasses - each getting its own Spring context - relying on the extension's + * per-class {@code beforeAll} to start (or no-op past) the container raced against + * context refresh on CI and on some local Docker setups: the first class or two would + * see the container "started" but not yet accepting TCP connections, and every test in + * that class would time out. Starting synchronously here, before any JUnit lifecycle + * callback runs for any subclass, removes that race. Testcontainers' Ryuk reaper still + * cleans the container up at JVM exit; it is not tied to the JUnit5 extension. */ @DataJpaTest @AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE) -@Testcontainers public abstract class AbstractPostgresRepositoryTest { - @Container static final PostgreSQLContainer POSTGRES = new PostgreSQLContainer<>(DockerImageName.parse("postgres:16-alpine")); + static { + POSTGRES.start(); + } + @DynamicPropertySource static void datasourceProperties(DynamicPropertyRegistry registry) { registry.add("spring.datasource.url", POSTGRES::getJdbcUrl); From ed6b230624a6f3e2cbb74ed559b2ea77a42236d0 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:37:01 +0200 Subject: [PATCH 10/22] refactor(persistency): extract shared audit columns into AttributeAuditFields 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. --- .../entity/AttributeAuditFields.java | 45 +++++++++++++++++++ .../entity/AttributeDefinition.java | 23 +--------- .../entity/AttributeDefinitionScope.java | 24 +--------- .../persistency/entity/AttributeValue.java | 24 +--------- 4 files changed, 48 insertions(+), 68 deletions(-) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeAuditFields.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeAuditFields.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeAuditFields.java new file mode 100644 index 0000000..29c6936 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeAuditFields.java @@ -0,0 +1,45 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.entity; + +import jakarta.persistence.Column; +import jakarta.persistence.MappedSuperclass; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Size; +import java.sql.Timestamp; +import lombok.Getter; +import lombok.Setter; + +/** + * Shared soft-delete and audit columns for the policy attribute schema entities + * ({@link AttributeDefinition}, {@link AttributeDefinitionScope}, {@link AttributeValue}). + */ +@Getter +@Setter +@MappedSuperclass +public abstract class AttributeAuditFields { + + @NotNull + @Column(name = "is_deleted", nullable = false) + private Boolean isDeleted = false; + + @NotNull + @Column(name = "created_at", nullable = false) + private Timestamp createdAt; + + @Size(max = 255) + @NotNull + @Column(name = "created_by", nullable = false, length = 255) + private String createdBy; + + @Column(name = "updated_at") + private Timestamp updatedAt; + + @Size(max = 255) + @Column(name = "updated_by", length = 255) + private String updatedBy; +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinition.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinition.java index d1e351a..6338baa 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinition.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinition.java @@ -9,7 +9,6 @@ import jakarta.persistence.*; import jakarta.validation.constraints.NotNull; import jakarta.validation.constraints.Size; -import java.sql.Timestamp; import lombok.Getter; import lombok.Setter; import org.hibernate.annotations.JdbcTypeCode; @@ -19,7 +18,7 @@ @Setter @Entity @Table(name = "attribute_definition") -public class AttributeDefinition { +public class AttributeDefinition extends AttributeAuditFields { @Id @GeneratedValue(strategy = GenerationType.IDENTITY) @Column(name = "id", nullable = false) @@ -67,24 +66,4 @@ public class AttributeDefinition { @NotNull @Column(name = "sensitive", nullable = false) private Boolean sensitive = false; - - @NotNull - @Column(name = "is_deleted", nullable = false) - private Boolean isDeleted = false; - - @NotNull - @Column(name = "created_at", nullable = false) - private Timestamp createdAt; - - @Size(max = 255) - @NotNull - @Column(name = "created_by", nullable = false, length = 255) - private String createdBy; - - @Column(name = "updated_at") - private Timestamp updatedAt; - - @Size(max = 255) - @Column(name = "updated_by", length = 255) - private String updatedBy; } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinitionScope.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinitionScope.java index f0a8faa..364ab43 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinitionScope.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeDefinitionScope.java @@ -8,8 +8,6 @@ import jakarta.persistence.*; import jakarta.validation.constraints.NotNull; -import jakarta.validation.constraints.Size; -import java.sql.Timestamp; import lombok.Getter; import lombok.Setter; import org.hibernate.annotations.JdbcTypeCode; @@ -19,7 +17,7 @@ @Setter @Entity @Table(name = "attribute_definition_scope") -public class AttributeDefinitionScope { +public class AttributeDefinitionScope extends AttributeAuditFields { @Id @GeneratedValue(strategy = GenerationType.IDENTITY) @Column(name = "id", nullable = false) @@ -42,24 +40,4 @@ public class AttributeDefinitionScope { @JdbcTypeCode(SqlTypes.JSON) @Column(name = "default_value") private String defaultValue; - - @NotNull - @Column(name = "is_deleted", nullable = false) - private Boolean isDeleted = false; - - @NotNull - @Column(name = "created_at", nullable = false) - private Timestamp createdAt; - - @Size(max = 255) - @NotNull - @Column(name = "created_by", nullable = false, length = 255) - private String createdBy; - - @Column(name = "updated_at") - private Timestamp updatedAt; - - @Size(max = 255) - @Column(name = "updated_by", length = 255) - private String updatedBy; } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeValue.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeValue.java index 888bba8..64ccd63 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeValue.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/AttributeValue.java @@ -8,8 +8,6 @@ import jakarta.persistence.*; import jakarta.validation.constraints.NotNull; -import jakarta.validation.constraints.Size; -import java.sql.Timestamp; import lombok.Getter; import lombok.Setter; import org.hibernate.annotations.JdbcTypeCode; @@ -19,7 +17,7 @@ @Setter @Entity @Table(name = "attribute_value") -public class AttributeValue { +public class AttributeValue extends AttributeAuditFields { @Id @GeneratedValue(strategy = GenerationType.IDENTITY) @Column(name = "id", nullable = false) @@ -44,24 +42,4 @@ public class AttributeValue { @JdbcTypeCode(SqlTypes.JSON) @Column(name = "value", nullable = false) private String value; - - @NotNull - @Column(name = "is_deleted", nullable = false) - private Boolean isDeleted = false; - - @NotNull - @Column(name = "created_at", nullable = false) - private Timestamp createdAt; - - @Size(max = 255) - @NotNull - @Column(name = "created_by", nullable = false, length = 255) - private String createdBy; - - @Column(name = "updated_at") - private Timestamp updatedAt; - - @Size(max = 255) - @Column(name = "updated_by", length = 255) - private String updatedBy; } From 2c0e145189661b8fd24999a446321ddea570de17 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 12:44:36 +0200 Subject: [PATCH 11/22] chore(deps): bump bundled Tomcat to 10.1.59 to resolve CRITICAL CVEs 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. --- pom.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/pom.xml b/pom.xml index ada73cc..9560a2c 100644 --- a/pom.xml +++ b/pom.xml @@ -59,6 +59,7 @@ 1.84 6.5.9 5.4.3 + 10.1.59 **/config/**, **/dto/**, **/entity/**, From 1cbff6e1796da05484f64a6c38f892298c4b2b41 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 14:22:47 +0200 Subject: [PATCH 12/22] feat(filter): add closed filter DSL for dynamic config filtering 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. --- .../ia/node/management/filter/Combinator.java | 39 +++++++ .../management/filter/ComparisonOperator.java | 73 +++++++++++++ .../filter/FilterCompilationException.java | 39 +++++++ .../ia/node/management/filter/FilterNode.java | 76 +++++++++++++ .../management/filter/CombinatorTest.java | 26 +++++ .../filter/ComparisonOperatorTest.java | 51 +++++++++ .../FilterCompilationExceptionTest.java | 30 ++++++ .../management/filter/FilterNodeTest.java | 102 ++++++++++++++++++ 8 files changed, 436 insertions(+) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/Combinator.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/ComparisonOperator.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterCompilationException.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNode.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/CombinatorTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/ComparisonOperatorTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterCompilationExceptionTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNodeTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/Combinator.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/Combinator.java new file mode 100644 index 0000000..e3344df --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/Combinator.java @@ -0,0 +1,39 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonValue; +import java.util.Arrays; +import java.util.Locale; + +/** How the children of a {@link FilterNode.Group} combine. */ +public enum Combinator { + AND("and"), + OR("or"); + + private final String wireName; + + Combinator(String wireName) { + this.wireName = wireName; + } + + @JsonValue + public String wireName() { + return wireName; + } + + @JsonCreator + public static Combinator fromWireName(String value) { + String normalised = value == null ? "" : value.trim().toLowerCase(Locale.ROOT); + return Arrays.stream(values()) + .filter(combinator -> combinator.wireName.equals(normalised)) + .findFirst() + .orElseThrow(() -> + new IllegalArgumentException("Unsupported combinator '" + value + "'; supported: [and, or]")); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/ComparisonOperator.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/ComparisonOperator.java new file mode 100644 index 0000000..61c93d2 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/ComparisonOperator.java @@ -0,0 +1,73 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonValue; +import java.util.Arrays; +import java.util.Locale; + +/** + * The closed comparison vocabulary a {@link FilterNode.Comparison} may use. Anything a caller + * names outside this set is rejected rather than interpreted, which is what keeps the + * translation from a caller filter to a database predicate total and auditable. + */ +public enum ComparisonOperator { + EQ("eq", Arity.SINGLE), + NEQ("neq", Arity.SINGLE), + IN("in", Arity.ANY), + NOT_IN("not_in", Arity.ANY), + LT("lt", Arity.SINGLE), + LTE("lte", Arity.SINGLE), + GT("gt", Arity.SINGLE), + GTE("gte", Arity.SINGLE), + /** Case-insensitive substring match. */ + CONTAINS("contains", Arity.SINGLE); + + /** How many operands the operator accepts. */ + public enum Arity { + /** Exactly one value. */ + SINGLE, + /** Zero or more values. */ + ANY + } + + private final String wireName; + private final Arity arity; + + ComparisonOperator(String wireName, Arity arity) { + this.wireName = wireName; + this.arity = arity; + } + + @JsonValue + public String wireName() { + return wireName; + } + + public Arity arity() { + return arity; + } + + /** {@code true} for operators that require a totally ordered operand type. */ + public boolean isOrdering() { + return this == LT || this == LTE || this == GT || this == GTE; + } + + @JsonCreator + public static ComparisonOperator fromWireName(String value) { + String normalised = value == null ? "" : value.trim().toLowerCase(Locale.ROOT); + return Arrays.stream(values()) + .filter(operator -> operator.wireName.equals(normalised)) + .findFirst() + .orElseThrow(() -> new IllegalArgumentException("Unsupported comparison operator '" + value + + "'; supported operators are " + + Arrays.stream(values()) + .map(ComparisonOperator::wireName) + .toList())); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterCompilationException.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterCompilationException.java new file mode 100644 index 0000000..413ee91 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterCompilationException.java @@ -0,0 +1,39 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +/** + * Raised when a {@link FilterNode} cannot be compiled into a database query predicate. + * + *

{@link Origin} decides the HTTP response: a malformed caller filter is a client error, + * whereas an attribute definition this system's own configuration cannot honour (an + * unrecognised {@code data_type}, or a stored value that fails to cast to its declared type) is + * an internal fault. The latter must never degrade into "apply what could be understood" - a + * partially applied filter is indistinguishable from a data leak - so both cases abort the + * request rather than return a partial or unfiltered result. + */ +public class FilterCompilationException extends RuntimeException { + + /** Which trust domain produced the offending predicate. */ + public enum Origin { + /** A caller-supplied filter is malformed, unknown, or type-mismatched. Maps to 400. */ + REQUEST, + /** This system's own attribute configuration or stored data is inconsistent. Maps to 500. */ + POLICY + } + + private final Origin origin; + + public FilterCompilationException(Origin origin, String message) { + super(message); + this.origin = origin; + } + + public Origin origin() { + return origin; + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNode.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNode.java new file mode 100644 index 0000000..d472afd --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNode.java @@ -0,0 +1,76 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.NotNull; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * A database-agnostic predicate tree carrying no SQL, no column names, and no operators beyond + * {@link ComparisonOperator} - a caller-supplied filter can never express anything the + * {@code SpecificationPredicateCompiler} cannot bind as a parameter. + */ +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type") +@JsonSubTypes({ + @JsonSubTypes.Type(value = FilterNode.Group.class, name = "group"), + @JsonSubTypes.Type(value = FilterNode.Comparison.class, name = "comparison"), + @JsonSubTypes.Type(value = FilterNode.Literal.class, name = "literal") +}) +public sealed interface FilterNode { + + /** + * A conjunction or disjunction of child predicates. An empty {@code AND} is true and an + * empty {@code OR} is false, matching the identity element of each operation - neither case + * silently widens a result set. + */ + record Group(@NotNull Combinator combinator, @NotNull List nodes) implements FilterNode { + + public Group { + nodes = nodes == null ? List.of() : List.copyOf(nodes); + } + + public static Group and(List nodes) { + return new Group(Combinator.AND, nodes); + } + + public static Group or(List nodes) { + return new Group(Combinator.OR, nodes); + } + } + + /** + * A comparison of one resource attribute against one or more literal operands. + * + * @param attribute logical attribute name, resolved against the resource attribute registry + * - never a column name and never interpolated into a query + * @param operator the comparison to apply + * @param values operands, still in their JSON representation; coerced to the attribute's + * declared type at compile time + */ + record Comparison(@NotBlank String attribute, @NotNull ComparisonOperator operator, @NotNull List values) + implements FilterNode { + + public Comparison { + values = values == null ? List.of() : Collections.unmodifiableList(new ArrayList<>(values)); + } + + public static Comparison of(String attribute, ComparisonOperator operator, Object... values) { + return new Comparison(attribute, operator, List.of(values)); + } + } + + /** A constant predicate. Not emitted by anything in this change; kept for structural parity. */ + record Literal(boolean value) implements FilterNode { + public static final Literal DENY_ALL = new Literal(false); + public static final Literal ALLOW_ALL = new Literal(true); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/CombinatorTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/CombinatorTest.java new file mode 100644 index 0000000..8adf7f2 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/CombinatorTest.java @@ -0,0 +1,26 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +class CombinatorTest { + + @Test + void fromWireName_resolvesAndAndOr() { + assertThat(Combinator.fromWireName("and")).isEqualTo(Combinator.AND); + assertThat(Combinator.fromWireName("OR")).isEqualTo(Combinator.OR); + } + + @Test + void fromWireName_rejectsUnknownCombinator() { + assertThatThrownBy(() -> Combinator.fromWireName("xor")).isInstanceOf(IllegalArgumentException.class); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/ComparisonOperatorTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/ComparisonOperatorTest.java new file mode 100644 index 0000000..ccbcd62 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/ComparisonOperatorTest.java @@ -0,0 +1,51 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +class ComparisonOperatorTest { + + @Test + void fromWireName_resolvesEveryDeclaredOperator() { + for (ComparisonOperator operator : ComparisonOperator.values()) { + assertThat(ComparisonOperator.fromWireName(operator.wireName())).isEqualTo(operator); + } + } + + @Test + void fromWireName_isCaseAndWhitespaceInsensitive() { + assertThat(ComparisonOperator.fromWireName(" EQ ")).isEqualTo(ComparisonOperator.EQ); + } + + @Test + void fromWireName_rejectsUnknownOperator() { + assertThatThrownBy(() -> ComparisonOperator.fromWireName("drop_table")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unsupported comparison operator"); + } + + @Test + void isOrdering_trueOnlyForRangeOperators() { + assertThat(ComparisonOperator.LT.isOrdering()).isTrue(); + assertThat(ComparisonOperator.LTE.isOrdering()).isTrue(); + assertThat(ComparisonOperator.GT.isOrdering()).isTrue(); + assertThat(ComparisonOperator.GTE.isOrdering()).isTrue(); + assertThat(ComparisonOperator.EQ.isOrdering()).isFalse(); + assertThat(ComparisonOperator.CONTAINS.isOrdering()).isFalse(); + } + + @Test + void arity_singleForEqualityAndRange_anyForInFamily() { + assertThat(ComparisonOperator.EQ.arity()).isEqualTo(ComparisonOperator.Arity.SINGLE); + assertThat(ComparisonOperator.IN.arity()).isEqualTo(ComparisonOperator.Arity.ANY); + assertThat(ComparisonOperator.NOT_IN.arity()).isEqualTo(ComparisonOperator.Arity.ANY); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterCompilationExceptionTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterCompilationExceptionTest.java new file mode 100644 index 0000000..4cc6ca9 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterCompilationExceptionTest.java @@ -0,0 +1,30 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; + +class FilterCompilationExceptionTest { + + @Test + void carriesOriginAndMessage() { + FilterCompilationException exception = new FilterCompilationException(Origin.REQUEST, "unknown attribute"); + + assertThat(exception.origin()).isEqualTo(Origin.REQUEST); + assertThat(exception.getMessage()).isEqualTo("unknown attribute"); + } + + @Test + void policyOriginDistinctFromRequestOrigin() { + FilterCompilationException exception = new FilterCompilationException(Origin.POLICY, "bad data_type"); + + assertThat(exception.origin()).isEqualTo(Origin.POLICY); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNodeTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNodeTest.java new file mode 100644 index 0000000..360f783 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNodeTest.java @@ -0,0 +1,102 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.exc.InvalidTypeIdException; +import java.util.List; +import org.junit.jupiter.api.Test; + +class FilterNodeTest { + + private final ObjectMapper mapper = new ObjectMapper(); + + @Test + void deserializesNestedGroupOfComparisons() throws Exception { + String json = + """ + { + "type": "group", + "combinator": "and", + "nodes": [ + { "type": "comparison", "attribute": "active", "operator": "eq", "values": [true] }, + { + "type": "group", + "combinator": "or", + "nodes": [ + { "type": "comparison", "attribute": "orgId", "operator": "in", "values": [1, 2, 3] } + ] + } + ] + } + """; + + FilterNode node = mapper.readValue(json, FilterNode.class); + + assertThat(node).isInstanceOf(FilterNode.Group.class); + FilterNode.Group group = (FilterNode.Group) node; + assertThat(group.combinator()).isEqualTo(Combinator.AND); + assertThat(group.nodes()).hasSize(2); + assertThat(group.nodes().get(0)).isInstanceOf(FilterNode.Comparison.class); + FilterNode.Comparison first = (FilterNode.Comparison) group.nodes().get(0); + assertThat(first.attribute()).isEqualTo("active"); + assertThat(first.operator()).isEqualTo(ComparisonOperator.EQ); + assertThat(first.values()).containsExactly(true); + + assertThat(group.nodes().get(1)).isInstanceOf(FilterNode.Group.class); + FilterNode.Group nested = (FilterNode.Group) group.nodes().get(1); + assertThat(nested.combinator()).isEqualTo(Combinator.OR); + FilterNode.Comparison nestedComparison = + (FilterNode.Comparison) nested.nodes().getFirst(); + assertThat(nestedComparison.values()).containsExactly(1, 2, 3); + } + + @Test + void deserializationRejectsUnknownDiscriminator() { + String json = """ + { "type": "sql_injection", "raw": "1=1" } + """; + + assertThatThrownBy(() -> mapper.readValue(json, FilterNode.class)).isInstanceOf(InvalidTypeIdException.class); + } + + @Test + void deserializationRejectsUnknownOperator() { + String json = + """ + { "type": "comparison", "attribute": "active", "operator": "drop_table", "values": [] } + """; + + assertThatThrownBy(() -> mapper.readValue(json, FilterNode.class)) + .hasRootCauseInstanceOf(IllegalArgumentException.class); + } + + @Test + void comparisonOf_buildsFromVarargs() { + FilterNode.Comparison comparison = FilterNode.Comparison.of("active", ComparisonOperator.EQ, true); + + assertThat(comparison.attribute()).isEqualTo("active"); + assertThat(comparison.values()).containsExactly(true); + } + + @Test + void comparisonValues_defaultToEmptyListWhenNull() { + FilterNode.Comparison comparison = new FilterNode.Comparison("active", ComparisonOperator.EQ, null); + + assertThat(comparison.values()).isEqualTo(List.of()); + } + + @Test + void group_defaultsNullNodesToEmptyList() { + FilterNode.Group group = new FilterNode.Group(Combinator.AND, null); + + assertThat(group.nodes()).isEmpty(); + } +} From 3bd49efbe07b999d10c181925d084d49812495d5 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 14:27:18 +0200 Subject: [PATCH 13/22] feat(filter): add producer/consumer resource attribute registry 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. --- .../filter/registry/AttributeType.java | 192 ++++++++++++++++++ .../ConfigurationResourceRegistry.java | 83 ++++++++ .../registry/DynamicAttributeResolver.java | 74 +++++++ .../filter/registry/ResourceAttribute.java | 33 +++ .../filter/registry/ResourceDefinition.java | 31 +++ .../filter/registry/ResourceType.java | 27 +++ .../AttributeDefinitionScopeRepository.java | 14 ++ .../filter/registry/AttributeTypeTest.java | 90 ++++++++ .../ConfigurationResourceRegistryTest.java | 71 +++++++ .../DynamicAttributeResolverTest.java | 123 +++++++++++ ...ttributeDefinitionScopeRepositoryTest.java | 51 +++++ 11 files changed, 789 insertions(+) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/AttributeType.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistry.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolver.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceAttribute.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceDefinition.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceType.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/AttributeTypeTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistryTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/AttributeType.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/AttributeType.java new file mode 100644 index 0000000..b91abd3 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/AttributeType.java @@ -0,0 +1,192 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.CONTAINS; +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.EQ; +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.GT; +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.GTE; +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.IN; +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.LT; +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.LTE; +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.NEQ; +import static uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator.NOT_IN; + +import java.math.BigDecimal; +import java.util.Locale; +import java.util.Set; +import uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; + +/** + * The value domain of a filterable resource attribute, fixed or dynamic. Declares which + * operators are meaningful for the attribute - so a caller cannot ask for a substring match on a + * boolean, or an ordering comparison on an opaque identifier - and converts a JSON operand into + * the exact Java type the query needs. An operand that cannot be converted is rejected rather + * than passed to the query, because that is the point at which a query would otherwise start + * matching the wrong rows. + */ +public enum AttributeType { + STRING(Set.of(EQ, NEQ, IN, NOT_IN, CONTAINS)) { + @Override + Object convert(Object raw) { + if (raw instanceof String text) { + return text; + } + throw typeError(raw, "a string"); + } + }, + + INTEGER(Set.of(EQ, NEQ, IN, NOT_IN, LT, LTE, GT, GTE)) { + @Override + Object convert(Object raw) { + long value = toLong(raw, "a whole number"); + if (value < Integer.MIN_VALUE || value > Integer.MAX_VALUE) { + throw typeError(raw, "a 32-bit whole number"); + } + return (int) value; + } + }, + + LONG(Set.of(EQ, NEQ, IN, NOT_IN, LT, LTE, GT, GTE)) { + @Override + Object convert(Object raw) { + return toLong(raw, "a whole number"); + } + }, + + DECIMAL(Set.of(EQ, NEQ, IN, NOT_IN, LT, LTE, GT, GTE)) { + @Override + Object convert(Object raw) { + try { + return switch (raw) { + case BigDecimal decimal -> decimal; + case Integer integer -> BigDecimal.valueOf(integer.longValue()); + case Long value -> BigDecimal.valueOf(value); + case Double value -> BigDecimal.valueOf(value); + case Float value -> BigDecimal.valueOf(value.doubleValue()); + case String text -> new BigDecimal(text.trim()); + case null, default -> throw typeError(raw, "a decimal number"); + }; + } catch (NumberFormatException e) { + throw typeError(raw, "a decimal number"); + } + } + }, + + BOOLEAN(Set.of(EQ, NEQ)) { + @Override + Object convert(Object raw) { + if (raw instanceof Boolean value) { + return value; + } + if (raw instanceof String text) { + String normalised = text.trim().toLowerCase(Locale.ROOT); + if ("true".equals(normalised)) { + return Boolean.TRUE; + } + if ("false".equals(normalised)) { + return Boolean.FALSE; + } + } + throw typeError(raw, "a boolean"); + } + }; + + private final Set supportedOperators; + + AttributeType(Set supportedOperators) { + this.supportedOperators = supportedOperators; + } + + public Set supportedOperators() { + return supportedOperators; + } + + public boolean supports(ComparisonOperator operator) { + return supportedOperators.contains(operator); + } + + /** + * Converts a JSON operand to the type the compiled predicate needs. + * + * @throws FilterCompilationException(REQUEST) if the operand is null or not convertible + */ + public Object coerce(Object raw, String attributeName) { + if (raw == null) { + throw new FilterCompilationException( + Origin.REQUEST, "Attribute '" + attributeName + "' does not accept a null operand"); + } + try { + return convert(raw); + } catch (IllegalArgumentException e) { + throw new FilterCompilationException( + Origin.REQUEST, "Attribute '" + attributeName + "' expects " + e.getMessage()); + } + } + + abstract Object convert(Object raw); + + /** + * Resolves the closed type domain an {@code attribute_definition.data_type} value declares. + * + * @throws FilterCompilationException(POLICY) if the value is not one of this enum's names - + * a configuration/data defect, not a caller error, since the caller never supplies this + * value + */ + public static AttributeType fromDataType(String dataType) { + if (dataType != null) { + for (AttributeType type : values()) { + if (type.name().equalsIgnoreCase(dataType.trim())) { + return type; + } + } + } + throw new FilterCompilationException( + Origin.POLICY, "Attribute definition declares unsupported data_type '" + dataType + "'"); + } + + private static long toLong(Object raw, String expectation) { + return switch (raw) { + case Integer value -> value.longValue(); + case Long value -> value; + case Short value -> value.longValue(); + case Byte value -> value.longValue(); + case BigDecimal value -> exactLong(value, expectation); + case Double value -> exactLong(BigDecimal.valueOf(value), expectation); + case Float value -> exactLong(BigDecimal.valueOf(value.doubleValue()), expectation); + case String text -> parseLong(text, expectation); + case null, default -> throw typeError(raw, expectation); + }; + } + + private static long exactLong(BigDecimal value, String expectation) { + try { + return value.longValueExact(); + } catch (ArithmeticException e) { + throw typeError(value, expectation); + } + } + + private static long parseLong(String text, String expectation) { + try { + return Long.parseLong(text.trim()); + } catch (NumberFormatException e) { + throw typeError(text, expectation); + } + } + + /** + * The message carries only the expectation. The rejected operand is deliberately not echoed + * - reflecting caller input into an error body is an avoidable habit. + */ + private static IllegalArgumentException typeError(Object raw, String expectation) { + String actual = raw == null ? "null" : raw.getClass().getSimpleName(); + return new IllegalArgumentException(expectation + " but received " + actual); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistry.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistry.java new file mode 100644 index 0000000..cad479d --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistry.java @@ -0,0 +1,83 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +import java.util.Map; +import java.util.Optional; +import org.springframework.stereotype.Component; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; + +/** + * Resolves a caller's logical attribute name, for {@code producer}/{@code consumer} filtering, + * against a fixed column first and a dynamically-registered attribute second - one entry point + * regardless of which kind the name turns out to be. + */ +@Component +public class ConfigurationResourceRegistry { + + private final Map fixedDefinitions; + private final DynamicAttributeResolver dynamicAttributeResolver; + + public ConfigurationResourceRegistry(DynamicAttributeResolver dynamicAttributeResolver) { + this.dynamicAttributeResolver = dynamicAttributeResolver; + this.fixedDefinitions = Map.of( + ResourceType.PRODUCER, producerDefinition(), + ResourceType.CONSUMER, consumerDefinition()); + } + + public ResourceDefinition fixedDefinitionFor(ResourceType resourceType) { + return fixedDefinitions.get(resourceType); + } + + /** + * @throws FilterCompilationException with {@code Origin.REQUEST} if {@code logicalName} + * resolves to neither a fixed column nor a live dynamic attribute for {@code + * resourceType} + */ + public ResourceAttribute resolve(ResourceType resourceType, String logicalName) { + Optional fixed = + fixedDefinitionFor(resourceType).find(logicalName); + if (fixed.isPresent()) { + return fixed.get(); + } + return dynamicAttributeResolver + .resolve(resourceType, logicalName) + .map(ResourceAttribute.class::cast) + .orElseThrow(() -> new FilterCompilationException( + Origin.REQUEST, + "Unknown attribute '" + logicalName + "' for resource type '" + resourceType + "'")); + } + + private static ResourceDefinition producerDefinition() { + return new ResourceDefinition( + ResourceType.PRODUCER, + Map.of( + "id", new ResourceAttribute.Fixed("id", "id", AttributeType.LONG), + "name", new ResourceAttribute.Fixed("name", "name", AttributeType.STRING), + "description", new ResourceAttribute.Fixed("description", "description", AttributeType.STRING), + "active", new ResourceAttribute.Fixed("active", "active", AttributeType.BOOLEAN), + "host", new ResourceAttribute.Fixed("host", "host", AttributeType.STRING), + "port", new ResourceAttribute.Fixed("port", "port", AttributeType.DECIMAL), + "tls", new ResourceAttribute.Fixed("tls", "tls", AttributeType.BOOLEAN), + "orgId", new ResourceAttribute.Fixed("orgId", "org.id", AttributeType.LONG))); + } + + private static ResourceDefinition consumerDefinition() { + return new ResourceDefinition( + ResourceType.CONSUMER, + Map.of( + "id", new ResourceAttribute.Fixed("id", "id", AttributeType.LONG), + "name", new ResourceAttribute.Fixed("name", "name", AttributeType.STRING), + "scheduleType", + new ResourceAttribute.Fixed("scheduleType", "scheduleType", AttributeType.STRING), + "scheduleExpression", + new ResourceAttribute.Fixed( + "scheduleExpression", "scheduleExpression", AttributeType.STRING), + "orgId", new ResourceAttribute.Fixed("orgId", "org.id", AttributeType.LONG))); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolver.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolver.java new file mode 100644 index 0000000..36ca980 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolver.java @@ -0,0 +1,74 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +import java.util.Optional; +import org.springframework.stereotype.Component; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionRepository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionScopeRepository; + +/** + * Resolves a caller's logical attribute name to a dynamically-registered attribute, straight + * from {@code attribute_definition}/{@code attribute_definition_scope} at filter-compile time + * rather than a cached snapshot - so a newly-registered attribute is filterable without a + * restart. See design.md's "dynamic attributes are resolved per lookup" decision. + */ +@Component +public class DynamicAttributeResolver { + + private final AttributeDefinitionRepository attributeDefinitionRepository; + private final AttributeDefinitionScopeRepository attributeDefinitionScopeRepository; + + public DynamicAttributeResolver( + AttributeDefinitionRepository attributeDefinitionRepository, + AttributeDefinitionScopeRepository attributeDefinitionScopeRepository) { + this.attributeDefinitionRepository = attributeDefinitionRepository; + this.attributeDefinitionScopeRepository = attributeDefinitionScopeRepository; + } + + /** + * Resolves {@code logicalName} (wire shape {@code "namespace.name"}) against the live + * attribute definitions registered for {@code resourceType}'s scope. + * + * @return empty when the name is not a live, registered dynamic attribute for this resource + * type - the registry reports this uniformly as "unknown attribute", the same as an + * unknown fixed column + * @throws uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException with {@code + * Origin.POLICY} if the definition's declared {@code data_type} is not one this system + * understands - a configuration defect, since the caller never supplies this value + */ + public Optional resolve(ResourceType resourceType, String logicalName) { + int separator = logicalName == null ? -1 : logicalName.indexOf('.'); + if (separator <= 0 || separator == logicalName.length() - 1) { + return Optional.empty(); + } + String namespace = logicalName.substring(0, separator); + String name = logicalName.substring(separator + 1); + + Optional definition = + attributeDefinitionRepository.findByNamespaceAndName(namespace, name); + if (definition.isEmpty() || Boolean.TRUE.equals(definition.get().getIsDeleted())) { + return Optional.empty(); + } + + Optional scope = + attributeDefinitionScopeRepository.findByAttributeDefinition_IdAndAttributeScope_CodeAndIsDeletedFalse( + definition.get().getId(), resourceType.attributeScopeCode()); + if (scope.isEmpty()) { + return Optional.empty(); + } + + AttributeType type = AttributeType.fromDataType(definition.get().getDataType()); + return Optional.of(new ResourceAttribute.Dynamic( + logicalName, + scope.get().getId(), + type, + Boolean.TRUE.equals(definition.get().getMultiValued()))); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceAttribute.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceAttribute.java new file mode 100644 index 0000000..60c759f --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceAttribute.java @@ -0,0 +1,33 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +/** + * A filterable attribute of a resource type, resolved from a caller's logical attribute name. + * The caller addresses both kinds through the same name; only the compiler needs to know which + * one it resolved to. + */ +public sealed interface ResourceAttribute { + + String logicalName(); + + AttributeType type(); + + /** A fixed entity column, resolved to its JPA property path (e.g. {@code "org.id"}). */ + record Fixed(String logicalName, String jpaPath, AttributeType type) implements ResourceAttribute {} + + /** + * An admin-defined attribute resolved from {@code attribute_definition}/{@code attribute_definition_scope}. + * + * @param attributeDefinitionScopeId the resolved {@code attribute_definition_scope.id} - the + * only value the compiler needs to correlate against {@code attribute_value}; never a + * caller-supplied string + * @param multiValued whether the definition is registered {@code multi_valued = true} + */ + record Dynamic(String logicalName, Long attributeDefinitionScopeId, AttributeType type, boolean multiValued) + implements ResourceAttribute {} +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceDefinition.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceDefinition.java new file mode 100644 index 0000000..d4105e2 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceDefinition.java @@ -0,0 +1,31 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +import java.util.List; +import java.util.Map; +import java.util.Optional; + +/** + * The closed set of fixed columns filterable on one resource type. Read from Java, not the + * database - these columns are only ever added by a Flyway-owned migration, so unlike dynamic + * attributes they do not need to be resolvable without a deploy. + */ +public record ResourceDefinition(ResourceType resourceType, Map attributes) { + + public ResourceDefinition { + attributes = Map.copyOf(attributes); + } + + public Optional find(String logicalName) { + return Optional.ofNullable(attributes.get(logicalName)); + } + + public List attributeNames() { + return attributes.keySet().stream().sorted().toList(); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceType.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceType.java new file mode 100644 index 0000000..792e577 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ResourceType.java @@ -0,0 +1,27 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +/** + * The configuration endpoints' filterable resource types, each tied to the {@code attribute_scope.code} + * row that scopes its dynamic attributes. + */ +public enum ResourceType { + PRODUCER("PRODUCER"), + CONSUMER("CONSUMER"); + + private final String attributeScopeCode; + + ResourceType(String attributeScopeCode) { + this.attributeScopeCode = attributeScopeCode; + } + + /** The {@code attribute_scope.code} that scopes this resource type's dynamic attributes. */ + public String attributeScopeCode() { + return attributeScopeCode; + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepository.java index 27104c5..6b3c6b4 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepository.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepository.java @@ -7,6 +7,7 @@ package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; import java.util.List; +import java.util.Optional; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.stereotype.Repository; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; @@ -23,4 +24,17 @@ public interface AttributeDefinitionScopeRepository extends JpaRepository { List findByAttributeDefinitionId(Long attributeDefinitionId); + + /** + * Resolves the single live binding of an attribute definition to a named scope, used to + * correlate a dynamic filter attribute against {@code attribute_value} by a single foreign + * key rather than joining {@code attribute_scope} at query time. + * + * @param attributeDefinitionId the {@code attribute_definition.id} resolved from the caller's + * logical attribute name + * @param scopeCode the {@code attribute_scope.code} of the resource type being filtered + * (e.g. {@code "PRODUCER"}), never a caller-supplied string + */ + Optional findByAttributeDefinition_IdAndAttributeScope_CodeAndIsDeletedFalse( + Long attributeDefinitionId, String scopeCode); } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/AttributeTypeTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/AttributeTypeTest.java new file mode 100644 index 0000000..5296a23 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/AttributeTypeTest.java @@ -0,0 +1,90 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.math.BigDecimal; +import org.junit.jupiter.api.Test; +import uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; + +class AttributeTypeTest { + + @Test + void string_coercesStringOnly() { + assertThat(AttributeType.STRING.coerce("abc", "name")).isEqualTo("abc"); + assertThatThrownBy(() -> AttributeType.STRING.coerce(1, "name")) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void long_coercesVariousNumericRepresentations() { + assertThat(AttributeType.LONG.coerce(42, "id")).isEqualTo(42L); + assertThat(AttributeType.LONG.coerce("42", "id")).isEqualTo(42L); + assertThat(AttributeType.LONG.coerce(42L, "id")).isEqualTo(42L); + } + + @Test + void long_rejectsNonNumericString() { + assertThatThrownBy(() -> AttributeType.LONG.coerce("not-a-number", "id")) + .isInstanceOf(FilterCompilationException.class); + } + + @Test + void integer_rejectsOutOfRangeValue() { + assertThatThrownBy(() -> AttributeType.INTEGER.coerce(Long.MAX_VALUE, "port")) + .isInstanceOf(FilterCompilationException.class); + } + + @Test + void decimal_coercesNumericAndStringRepresentations() { + assertThat(AttributeType.DECIMAL.coerce("1.50", "port")).isEqualTo(new BigDecimal("1.50")); + assertThat(AttributeType.DECIMAL.coerce(2, "port")).isEqualTo(BigDecimal.valueOf(2)); + } + + @Test + void boolean_coercesBooleanAndStringRepresentations() { + assertThat(AttributeType.BOOLEAN.coerce(true, "active")).isEqualTo(true); + assertThat(AttributeType.BOOLEAN.coerce("false", "active")).isEqualTo(false); + assertThatThrownBy(() -> AttributeType.BOOLEAN.coerce("maybe", "active")) + .isInstanceOf(FilterCompilationException.class); + } + + @Test + void coerce_rejectsNullOperand() { + assertThatThrownBy(() -> AttributeType.STRING.coerce(null, "name")) + .isInstanceOf(FilterCompilationException.class) + .hasMessageContaining("does not accept a null operand"); + } + + @Test + void supports_reflectsPerTypeOperatorDomain() { + assertThat(AttributeType.BOOLEAN.supports(ComparisonOperator.EQ)).isTrue(); + assertThat(AttributeType.BOOLEAN.supports(ComparisonOperator.CONTAINS)).isFalse(); + assertThat(AttributeType.STRING.supports(ComparisonOperator.CONTAINS)).isTrue(); + assertThat(AttributeType.LONG.supports(ComparisonOperator.GT)).isTrue(); + } + + @Test + void fromDataType_resolvesKnownTypesCaseInsensitively() { + assertThat(AttributeType.fromDataType("string")).isEqualTo(AttributeType.STRING); + assertThat(AttributeType.fromDataType("DECIMAL")).isEqualTo(AttributeType.DECIMAL); + } + + @Test + void fromDataType_rejectsUnknownTypeAsPolicyOrigin() { + assertThatThrownBy(() -> AttributeType.fromDataType("XML")) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.POLICY); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistryTest.java new file mode 100644 index 0000000..d4c7542 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistryTest.java @@ -0,0 +1,71 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Optional; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; + +class ConfigurationResourceRegistryTest { + + private DynamicAttributeResolver dynamicAttributeResolver; + private ConfigurationResourceRegistry registry; + + @BeforeEach + void setUp() { + dynamicAttributeResolver = mock(DynamicAttributeResolver.class); + registry = new ConfigurationResourceRegistry(dynamicAttributeResolver); + } + + @Test + void producerDefinition_exposesExpectedFixedColumns() { + assertThat(registry.fixedDefinitionFor(ResourceType.PRODUCER).attributeNames()) + .containsExactlyInAnyOrder("id", "name", "description", "active", "host", "port", "tls", "orgId"); + } + + @Test + void consumerDefinition_exposesExpectedFixedColumns() { + assertThat(registry.fixedDefinitionFor(ResourceType.CONSUMER).attributeNames()) + .containsExactlyInAnyOrder("id", "name", "scheduleType", "scheduleExpression", "orgId"); + } + + @Test + void resolve_returnsFixedAttributeWithoutConsultingDynamicResolver() { + ResourceAttribute resolved = registry.resolve(ResourceType.PRODUCER, "active"); + + assertThat(resolved).isInstanceOf(ResourceAttribute.Fixed.class); + assertThat(((ResourceAttribute.Fixed) resolved).jpaPath()).isEqualTo("active"); + } + + @Test + void resolve_fallsThroughToDynamicResolverWhenNotFixed() { + ResourceAttribute.Dynamic dynamic = + new ResourceAttribute.Dynamic("policy.risk-tier", 42L, AttributeType.STRING, false); + when(dynamicAttributeResolver.resolve(ResourceType.PRODUCER, "policy.risk-tier")) + .thenReturn(Optional.of(dynamic)); + + ResourceAttribute resolved = registry.resolve(ResourceType.PRODUCER, "policy.risk-tier"); + + assertThat(resolved).isEqualTo(dynamic); + } + + @Test + void resolve_rejectsUnknownAttributeAsRequestOrigin() { + when(dynamicAttributeResolver.resolve(ResourceType.PRODUCER, "nope")).thenReturn(Optional.empty()); + + assertThatThrownBy(() -> registry.resolve(ResourceType.PRODUCER, "nope")) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(FilterCompilationException.Origin.REQUEST); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java new file mode 100644 index 0000000..4a98dae --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java @@ -0,0 +1,123 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.registry; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.sql.Timestamp; +import java.time.Instant; +import java.util.Optional; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AbstractPostgresRepositoryTest; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionRepository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionScopeRepository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeScopeRepository; + +class DynamicAttributeResolverTest extends AbstractPostgresRepositoryTest { + + @Autowired + private AttributeDefinitionRepository attributeDefinitionRepository; + + @Autowired + private AttributeDefinitionScopeRepository attributeDefinitionScopeRepository; + + @Autowired + private AttributeScopeRepository attributeScopeRepository; + + private DynamicAttributeResolver resolver; + + private DynamicAttributeResolver resolver() { + if (resolver == null) { + resolver = new DynamicAttributeResolver(attributeDefinitionRepository, attributeDefinitionScopeRepository); + } + return resolver; + } + + private AttributeDefinition persistDefinition(String namespace, String name, String dataType, boolean multi) { + AttributeDefinition definition = new AttributeDefinition(); + definition.setNamespace(namespace); + definition.setName(name); + definition.setDescription("test"); + definition.setDataType(dataType); + definition.setMultiValued(multi); + definition.setCreatedAt(Timestamp.from(Instant.now())); + definition.setCreatedBy("test"); + return attributeDefinitionRepository.saveAndFlush(definition); + } + + private void bindToScope(AttributeDefinition definition, String scopeCode) { + AttributeScope scope = attributeScopeRepository.findByCode(scopeCode).orElseThrow(); + AttributeDefinitionScope binding = new AttributeDefinitionScope(); + binding.setAttributeDefinition(definition); + binding.setAttributeScope(scope); + binding.setRequired(false); + binding.setCreatedAt(Timestamp.from(Instant.now())); + binding.setCreatedBy("test"); + attributeDefinitionScopeRepository.saveAndFlush(binding); + } + + @Test + void resolve_returnsDynamicAttributeForRegisteredScope() { + AttributeDefinition definition = persistDefinition("policy", "risk-tier", "STRING", false); + bindToScope(definition, "PRODUCER"); + + Optional resolved = resolver().resolve(ResourceType.PRODUCER, "policy.risk-tier"); + + assertThat(resolved).isPresent(); + assertThat(resolved.get().type()).isEqualTo(AttributeType.STRING); + assertThat(resolved.get().multiValued()).isFalse(); + } + + @Test + void resolve_isEmptyWhenDefinitionExistsButNotBoundToRequestedScope() { + AttributeDefinition definition = persistDefinition("policy", "consumer-only", "STRING", false); + bindToScope(definition, "CONSUMER"); + + assertThat(resolver().resolve(ResourceType.PRODUCER, "policy.consumer-only")) + .isEmpty(); + } + + @Test + void resolve_isEmptyForUnregisteredAttributeName() { + assertThat(resolver().resolve(ResourceType.PRODUCER, "policy.does-not-exist")) + .isEmpty(); + } + + @Test + void resolve_isEmptyForMalformedLogicalName() { + assertThat(resolver().resolve(ResourceType.PRODUCER, "no-dot-here")).isEmpty(); + } + + @Test + void resolve_throwsPolicyOriginForUnrecognisedDataType() { + AttributeDefinition definition = persistDefinition("policy", "bad-type", "XML", false); + bindToScope(definition, "PRODUCER"); + + assertThatThrownBy(() -> resolver().resolve(ResourceType.PRODUCER, "policy.bad-type")) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.POLICY); + } + + @Test + void resolve_isEmptyWhenDefinitionIsSoftDeleted() { + AttributeDefinition definition = persistDefinition("policy", "deleted-attr", "STRING", false); + bindToScope(definition, "PRODUCER"); + definition.setIsDeleted(true); + attributeDefinitionRepository.saveAndFlush(definition); + + assertThat(resolver().resolve(ResourceType.PRODUCER, "policy.deleted-attr")) + .isEmpty(); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepositoryTest.java index 605dcb4..7d6d278 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepositoryTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeDefinitionScopeRepositoryTest.java @@ -12,6 +12,7 @@ import java.sql.Timestamp; import java.time.Instant; import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.dao.DataIntegrityViolationException; @@ -72,6 +73,56 @@ void findByAttributeDefinitionId_returnsAllBoundScopes() { .containsExactlyInAnyOrder(productScope.getId(), consumerScope.getId()); } + @Test + void findByAttributeDefinition_IdAndAttributeScope_CodeAndIsDeletedFalse_returnsMatchingLiveBinding() { + AttributeDefinition definition = persistDefinition("scoped-lookup-attr"); + AttributeScope productScope = + attributeScopeRepository.findByCode("PRODUCT").orElseThrow(); + AttributeScope consumerScope = + attributeScopeRepository.findByCode("CONSUMER").orElseThrow(); + AttributeDefinitionScope productBinding = + attributeDefinitionScopeRepository.saveAndFlush(newBinding(definition, productScope, false)); + attributeDefinitionScopeRepository.saveAndFlush(newBinding(definition, consumerScope, false)); + + Optional found = + attributeDefinitionScopeRepository.findByAttributeDefinition_IdAndAttributeScope_CodeAndIsDeletedFalse( + definition.getId(), "PRODUCT"); + + assertThat(found).isPresent(); + assertThat(found.get().getId()).isEqualTo(productBinding.getId()); + } + + @Test + void findByAttributeDefinition_IdAndAttributeScope_CodeAndIsDeletedFalse_isEmptyForMismatchedScope() { + AttributeDefinition definition = persistDefinition("scoped-lookup-mismatch-attr"); + AttributeScope productScope = + attributeScopeRepository.findByCode("PRODUCT").orElseThrow(); + attributeDefinitionScopeRepository.saveAndFlush(newBinding(definition, productScope, false)); + + Optional found = + attributeDefinitionScopeRepository.findByAttributeDefinition_IdAndAttributeScope_CodeAndIsDeletedFalse( + definition.getId(), "CONSUMER"); + + assertThat(found).isEmpty(); + } + + @Test + void findByAttributeDefinition_IdAndAttributeScope_CodeAndIsDeletedFalse_excludesSoftDeletedBinding() { + AttributeDefinition definition = persistDefinition("scoped-lookup-deleted-attr"); + AttributeScope productScope = + attributeScopeRepository.findByCode("PRODUCT").orElseThrow(); + AttributeDefinitionScope binding = + attributeDefinitionScopeRepository.saveAndFlush(newBinding(definition, productScope, false)); + binding.setIsDeleted(true); + attributeDefinitionScopeRepository.saveAndFlush(binding); + + Optional found = + attributeDefinitionScopeRepository.findByAttributeDefinition_IdAndAttributeScope_CodeAndIsDeletedFalse( + definition.getId(), "PRODUCT"); + + assertThat(found).isEmpty(); + } + @Test void save_rejectsDuplicateDefinitionScopePair() { AttributeDefinition definition = persistDefinition("duplicate-binding-attr"); From d00ae3c7154e74d16dae2e0e5346472ac30dd89b Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 14:37:15 +0200 Subject: [PATCH 14/22] feat(filter): add SpecificationPredicateCompiler for producer/consumer 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. --- .../SpecificationPredicateCompiler.java | 236 ++++++++++++ .../SpecificationPredicateCompilerTest.java | 340 ++++++++++++++++++ 2 files changed, 576 insertions(+) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java new file mode 100644 index 0000000..34ce711 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java @@ -0,0 +1,236 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.compiler; + +import jakarta.persistence.criteria.CriteriaBuilder; +import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.Path; +import jakarta.persistence.criteria.Predicate; +import jakarta.persistence.criteria.Root; +import jakarta.persistence.criteria.Subquery; +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.List; +import org.hibernate.query.criteria.HibernateCriteriaBuilder; +import org.hibernate.query.criteria.JpaExpression; +import org.springframework.data.jpa.domain.Specification; +import org.springframework.stereotype.Component; +import uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterNode; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.AttributeType; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ConfigurationResourceRegistry; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ResourceAttribute; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ResourceType; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeValue; + +/** + * Compiles a validated {@link FilterNode} into a Spring Data JPA {@link Specification}. A fixed + * attribute becomes a direct {@code CriteriaBuilder} predicate on the entity path; a dynamic + * attribute becomes a correlated {@code EXISTS} subquery against {@code attribute_value}, + * scoped by the attribute's already-resolved {@code attribute_definition_scope.id} - never a + * caller-supplied string - with the operand cast to the attribute's declared {@code data_type}. + */ +@Component +public class SpecificationPredicateCompiler { + + private final ConfigurationResourceRegistry registry; + + public SpecificationPredicateCompiler(ConfigurationResourceRegistry registry) { + this.registry = registry; + } + + public Specification compile(ResourceType resourceType, FilterNode filter) { + return (root, query, cb) -> toPredicate(filter, resourceType, root, query, cb); + } + + private Predicate toPredicate( + FilterNode node, ResourceType resourceType, Root root, CriteriaQuery query, CriteriaBuilder cb) { + return switch (node) { + case FilterNode.Group group -> groupPredicate(group, resourceType, root, query, cb); + case FilterNode.Comparison comparison -> comparisonPredicate(comparison, resourceType, root, query, cb); + case FilterNode.Literal literal -> literal.value() ? cb.conjunction() : cb.disjunction(); + }; + } + + private Predicate groupPredicate( + FilterNode.Group group, + ResourceType resourceType, + Root root, + CriteriaQuery query, + CriteriaBuilder cb) { + List children = group.nodes().stream() + .map(child -> toPredicate(child, resourceType, root, query, cb)) + .toList(); + return switch (group.combinator()) { + case AND -> cb.and(children.toArray(new Predicate[0])); + case OR -> cb.or(children.toArray(new Predicate[0])); + }; + } + + private Predicate comparisonPredicate( + FilterNode.Comparison comparison, + ResourceType resourceType, + Root root, + CriteriaQuery query, + CriteriaBuilder cb) { + ResourceAttribute attribute = registry.resolve(resourceType, comparison.attribute()); + requireSupportedOperator(attribute, comparison.operator(), comparison.attribute()); + List operands = requireArity(comparison.operator(), comparison.values(), comparison.attribute()); + List coerced = operands.stream() + .map(raw -> attribute.type().coerce(raw, comparison.attribute())) + .toList(); + + return switch (attribute) { + case ResourceAttribute.Fixed fixed -> fixedPredicate(fixed, comparison.operator(), coerced, root, cb); + case ResourceAttribute.Dynamic dynamic -> + dynamicPredicate(dynamic, comparison.operator(), coerced, root, query, cb); + }; + } + + private void requireSupportedOperator(ResourceAttribute attribute, ComparisonOperator operator, String name) { + if (!attribute.type().supports(operator)) { + throw new FilterCompilationException( + Origin.REQUEST, + "Operator '" + operator.wireName() + "' is not supported for attribute '" + name + "'"); + } + boolean isEqualityFamily = operator == ComparisonOperator.EQ + || operator == ComparisonOperator.NEQ + || operator == ComparisonOperator.IN + || operator == ComparisonOperator.NOT_IN; + if (attribute instanceof ResourceAttribute.Dynamic dynamic && dynamic.multiValued() && !isEqualityFamily) { + throw new FilterCompilationException( + Origin.REQUEST, + "Operator '" + operator.wireName() + "' is not supported for multi-valued attribute '" + name + + "'"); + } + } + + private List requireArity(ComparisonOperator operator, List values, String name) { + if (operator.arity() == ComparisonOperator.Arity.SINGLE && values.size() != 1) { + throw new FilterCompilationException( + Origin.REQUEST, + "Operator '" + operator.wireName() + "' requires exactly one operand for attribute '" + name + "'"); + } + return values; + } + + // ----------------------------------------------------------------------------------- + // Fixed attributes + // ----------------------------------------------------------------------------------- + + private Predicate fixedPredicate( + ResourceAttribute.Fixed fixed, + ComparisonOperator operator, + List values, + Root root, + CriteriaBuilder cb) { + Path path = resolvePath(root, fixed.jpaPath()); + return buildComparison(cb, path, fixed.type(), operator, values); + } + + private static Path resolvePath(Root root, String dottedPath) { + Path path = root; + for (String segment : dottedPath.split("\\.")) { + path = path.get(segment); + } + return path; + } + + // ----------------------------------------------------------------------------------- + // Dynamic attributes + // ----------------------------------------------------------------------------------- + + private Predicate dynamicPredicate( + ResourceAttribute.Dynamic dynamic, + ComparisonOperator operator, + List values, + Root root, + CriteriaQuery query, + CriteriaBuilder cb) { + HibernateCriteriaBuilder hcb = (HibernateCriteriaBuilder) cb; + Subquery subquery = query.subquery(Long.class); + Root attributeValue = subquery.from(AttributeValue.class); + subquery.select(cb.literal(1L)); + + // Hibernate's own Path implementation also implements JpaExpression; the JPA-standard + // Path/Expression interfaces returned by Root#get don't expose that statically. + @SuppressWarnings("unchecked") + JpaExpression valuePath = (JpaExpression) attributeValue.get("value"); + JpaExpression rawText = hcb.cast(valuePath, String.class); + var castExpression = castTo(hcb, rawText, dynamic.type()); + + List conditions = new ArrayList<>(); + conditions.add(cb.equal( + attributeValue.get("attributeDefinitionScope").get("id"), dynamic.attributeDefinitionScopeId())); + conditions.add(cb.equal(attributeValue.get("entityId"), root.get("id"))); + conditions.add(cb.isFalse(attributeValue.get("isDeleted"))); + conditions.add(buildComparison(cb, castExpression, dynamic.type(), operator, values)); + + subquery.where(cb.and(conditions.toArray(new Predicate[0]))); + return cb.exists(subquery); + } + + /** + * {@code attribute_value.value::text} renders a JSON string with its surrounding quotes + * (e.g. {@code "gold"}) but a JSON number/boolean without them (e.g. {@code 42}, {@code + * true}) - so only the {@code STRING} case needs unquoting before use as a plain value. + */ + private static JpaExpression castTo( + HibernateCriteriaBuilder hcb, JpaExpression rawText, AttributeType type) { + return switch (type) { + case STRING -> hcb.function("btrim", String.class, rawText, hcb.literal("\"")); + case BOOLEAN -> hcb.cast(rawText, Boolean.class); + case INTEGER -> hcb.cast(rawText, Integer.class); + case LONG -> hcb.cast(rawText, Long.class); + case DECIMAL -> hcb.cast(rawText, BigDecimal.class); + }; + } + + // ----------------------------------------------------------------------------------- + // Shared comparison building + // ----------------------------------------------------------------------------------- + + @SuppressWarnings({"unchecked", "rawtypes"}) + private static Predicate buildComparison( + CriteriaBuilder cb, + jakarta.persistence.criteria.Expression expression, + AttributeType type, + ComparisonOperator operator, + List values) { + return switch (operator) { + case EQ -> cb.equal(expression, values.get(0)); + case NEQ -> cb.notEqual(expression, values.get(0)); + case IN -> ((jakarta.persistence.criteria.Expression) expression).in(values); + case NOT_IN -> cb.not(((jakarta.persistence.criteria.Expression) expression).in(values)); + case LT -> + cb.lessThan( + (jakarta.persistence.criteria.Expression) expression, (Comparable) values.get(0)); + case LTE -> + cb.lessThanOrEqualTo( + (jakarta.persistence.criteria.Expression) expression, (Comparable) values.get(0)); + case GT -> + cb.greaterThan( + (jakarta.persistence.criteria.Expression) expression, (Comparable) values.get(0)); + case GTE -> + cb.greaterThanOrEqualTo( + (jakarta.persistence.criteria.Expression) expression, (Comparable) values.get(0)); + case CONTAINS -> + containsPredicate( + cb, (jakarta.persistence.criteria.Expression) expression, (String) values.get(0)); + }; + } + + private static final char LIKE_ESCAPE = '\\'; + + private static Predicate containsPredicate( + CriteriaBuilder cb, jakarta.persistence.criteria.Expression expression, String needle) { + String escaped = needle.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_"); + return cb.like(cb.lower(expression), "%" + escaped.toLowerCase(java.util.Locale.ROOT) + "%", LIKE_ESCAPE); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java new file mode 100644 index 0000000..15f1cfd --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java @@ -0,0 +1,340 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter.compiler; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.PersistenceException; +import jakarta.persistence.criteria.CriteriaBuilder; +import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.Root; +import java.math.BigDecimal; +import java.sql.Timestamp; +import java.time.Instant; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.jpa.domain.Specification; +import org.springframework.transaction.annotation.Transactional; +import uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterNode; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ConfigurationResourceRegistry; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.DynamicAttributeResolver; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ResourceType; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeValue; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AbstractPostgresRepositoryTest; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionRepository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionScopeRepository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeScopeRepository; + +@Transactional +class SpecificationPredicateCompilerTest extends AbstractPostgresRepositoryTest { + + @Autowired + private EntityManager entityManager; + + @Autowired + private AttributeDefinitionRepository attributeDefinitionRepository; + + @Autowired + private AttributeDefinitionScopeRepository attributeDefinitionScopeRepository; + + @Autowired + private AttributeScopeRepository attributeScopeRepository; + + private SpecificationPredicateCompiler compiler() { + DynamicAttributeResolver resolver = + new DynamicAttributeResolver(attributeDefinitionRepository, attributeDefinitionScopeRepository); + return new SpecificationPredicateCompiler(new ConfigurationResourceRegistry(resolver)); + } + + private List execute(Specification specification) { + CriteriaBuilder cb = entityManager.getCriteriaBuilder(); + CriteriaQuery query = cb.createQuery(Producer.class); + Root root = query.from(Producer.class); + query.where(specification.toPredicate(root, query, cb)); + return entityManager.createQuery(query).getResultList(); + } + + private Organisation persistOrganisation(String name) { + Organisation org = new Organisation(); + org.setName(name); + entityManager.persist(org); + return org; + } + + private Producer persistProducer(Organisation org, String name, boolean active) { + Producer producer = new Producer(); + producer.setName(name); + producer.setDescription("test producer"); + producer.setOrg(org); + producer.setActive(active); + producer.setHost("host.example"); + producer.setPort(BigDecimal.valueOf(443)); + producer.setTls(true); + producer.setIdpClientId(name + "-client"); + entityManager.persist(producer); + return producer; + } + + private AttributeDefinitionScope persistProducerScopedDefinition(String name, String dataType, boolean multi) { + AttributeDefinition definition = new AttributeDefinition(); + definition.setNamespace("policy"); + definition.setName(name); + definition.setDescription("test"); + definition.setDataType(dataType); + definition.setMultiValued(multi); + definition.setCreatedAt(Timestamp.from(Instant.now())); + definition.setCreatedBy("test"); + definition = attributeDefinitionRepository.saveAndFlush(definition); + + AttributeScope scope = attributeScopeRepository.findByCode("PRODUCER").orElseThrow(); + AttributeDefinitionScope binding = new AttributeDefinitionScope(); + binding.setAttributeDefinition(definition); + binding.setAttributeScope(scope); + binding.setRequired(false); + binding.setCreatedAt(Timestamp.from(Instant.now())); + binding.setCreatedBy("test"); + return attributeDefinitionScopeRepository.saveAndFlush(binding); + } + + private void persistValue(AttributeDefinitionScope binding, Long entityId, String json) { + AttributeValue value = new AttributeValue(); + value.setAttributeDefinitionScope(binding); + value.setEntityId(entityId); + value.setValue(json); + value.setCreatedAt(Timestamp.from(Instant.now())); + value.setCreatedBy("test"); + entityManager.persist(value); + } + + // 3.1 fixed attribute + + @Test + void fixedAttributeEquality_matchesOnlyExpectedRows() { + Organisation org = persistOrganisation("org-fixed"); + Producer active = persistProducer(org, "active-producer", true); + persistProducer(org, "inactive-producer", false); + entityManager.flush(); + + Specification spec = compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("active", ComparisonOperator.EQ, true)); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactly(active.getId()); + } + + // 3.2 dynamic attribute EXISTS subquery + + @Test + void dynamicAttributeEquality_matchesOnlyRowsWithLiveAttributeValue() { + Organisation org = persistOrganisation("org-dynamic"); + Producer withAttribute = persistProducer(org, "with-tier", true); + persistProducer(org, "without-tier", true); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("risk-tier", "STRING", false); + persistValue(binding, withAttribute.getId(), "\"gold\""); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Comparison.of("policy.risk-tier", ComparisonOperator.EQ, "gold")); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactly(withAttribute.getId()); + } + + @Test + void dynamicAttributeExcludesSoftDeletedValue() { + Organisation org = persistOrganisation("org-soft-deleted"); + Producer producer = persistProducer(org, "soft-deleted-value-producer", true); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("soft-deleted-tier", "STRING", false); + AttributeValue value = new AttributeValue(); + value.setAttributeDefinitionScope(binding); + value.setEntityId(producer.getId()); + value.setValue("\"gold\""); + value.setIsDeleted(true); + value.setCreatedAt(Timestamp.from(Instant.now())); + value.setCreatedBy("test"); + entityManager.persist(value); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Comparison.of("policy.soft-deleted-tier", ComparisonOperator.EQ, "gold")); + + assertThat(execute(spec)).isEmpty(); + } + + // 3.3 per-data_type coercion and cast failure + + @Test + void dynamicAttributeRangeComparison_castsNumericDataTypeCorrectly() { + Organisation org = persistOrganisation("org-numeric"); + Producer low = persistProducer(org, "low-priority", true); + Producer high = persistProducer(org, "high-priority", true); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("priority", "INTEGER", false); + persistValue(binding, low.getId(), "5"); + persistValue(binding, high.getId(), "50"); + entityManager.flush(); + + Specification spec = compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("policy.priority", ComparisonOperator.GT, 10)); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactly(high.getId()); + } + + @Test + void dynamicAttributeBooleanCast_matchesStoredBooleanValue() { + Organisation org = persistOrganisation("org-boolean"); + Producer producer = persistProducer(org, "flagged-producer", true); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("flagged", "BOOLEAN", false); + persistValue(binding, producer.getId(), "true"); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, FilterNode.Comparison.of("policy.flagged", ComparisonOperator.EQ, true)); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactly(producer.getId()); + } + + @Test + void dynamicAttributeCastFailure_throwsRatherThanReturningWrongResult() { + Organisation org = persistOrganisation("org-cast-failure"); + Producer producer = persistProducer(org, "bad-numeric-value-producer", true); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("broken-priority", "INTEGER", false); + // Stored value cannot be cast to INTEGER at query time - nothing in the schema enforces + // that attribute_value.value matches its definition's declared data_type (see design.md). + persistValue(binding, producer.getId(), "\"not-a-number\""); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Comparison.of("policy.broken-priority", ComparisonOperator.GT, 1)); + + assertThatThrownBy(() -> execute(spec)).isInstanceOf(PersistenceException.class); + } + + // 3.4 nested groups mixing fixed and dynamic + + @Test + void groupAnd_combinesFixedAndDynamicComparisons() { + Organisation org = persistOrganisation("org-and"); + Producer matches = persistProducer(org, "matches-both", true); + Producer failsFixed = persistProducer(org, "fails-fixed", false); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("and-tier", "STRING", false); + persistValue(binding, matches.getId(), "\"gold\""); + persistValue(binding, failsFixed.getId(), "\"gold\""); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Group.and(List.of( + FilterNode.Comparison.of("active", ComparisonOperator.EQ, true), + FilterNode.Comparison.of("policy.and-tier", ComparisonOperator.EQ, "gold")))); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactly(matches.getId()); + } + + @Test + void groupOr_combinesFixedAndDynamicComparisons() { + Organisation org = persistOrganisation("org-or"); + Producer matchesFixed = persistProducer(org, "matches-fixed-only", true); + Producer matchesDynamic = persistProducer(org, "matches-dynamic-only", false); + persistProducer(org, "matches-neither", false); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("or-tier", "STRING", false); + persistValue(binding, matchesDynamic.getId(), "\"gold\""); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Group.or(List.of( + FilterNode.Comparison.of("active", ComparisonOperator.EQ, true), + FilterNode.Comparison.of("policy.or-tier", ComparisonOperator.EQ, "gold")))); + + assertThat(execute(spec)) + .extracting(Producer::getId) + .containsExactlyInAnyOrder(matchesFixed.getId(), matchesDynamic.getId()); + } + + // 3.5 rejections + + @Test + void unknownAttribute_rejectedWithRequestOriginAndNoInternalLeak() { + // compile() only returns a lazy Specification; resolution happens when the predicate is built. + assertThatThrownBy(() -> execute(compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("nope", ComparisonOperator.EQ, "x")))) + .isInstanceOf(FilterCompilationException.class) + .satisfies(e -> { + FilterCompilationException fce = (FilterCompilationException) e; + assertThat(fce.origin()).isEqualTo(Origin.REQUEST); + assertThat(fce.getMessage()).contains("'nope'"); + assertThat(fce.getMessage()) + .doesNotContain("attribute_value") + .doesNotContain("attribute_definition"); + }); + } + + @Test + void operatorUnsupportedForType_rejected() { + assertThatThrownBy(() -> execute(compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Comparison.of("active", ComparisonOperator.CONTAINS, "x")))) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void wrongArity_rejectedForSingleValueOperator() { + assertThatThrownBy(() -> execute(compiler() + .compile( + ResourceType.PRODUCER, + new FilterNode.Comparison("active", ComparisonOperator.EQ, List.of(true, false))))) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void wrongOperandType_rejected() { + assertThatThrownBy(() -> execute(compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Comparison.of("port", ComparisonOperator.GT, "not-a-number")))) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } +} From e4a7c7f9f1932577583d55b766439178c5ce4e60 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 14:40:57 +0200 Subject: [PATCH 15/22] feat(config): wire Specification filtering through repository and service 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. --- .../repository/ConsumerRepository.java | 16 ++- .../repository/ProducerRepository.java | 17 ++- .../service/data/ConsumerService.java | 11 ++ .../service/data/ProducerService.java | 11 ++ .../data/impl/ConsumerServiceImpl.java | 9 ++ .../data/impl/ProducerServiceImpl.java | 9 ++ ...erConsumerSpecificationRepositoryTest.java | 120 ++++++++++++++++++ .../data/impl/ConsumerServiceImplTest.java | 33 +++++ .../data/impl/ProducerServiceImplTest.java | 34 +++++ 9 files changed, 258 insertions(+), 2 deletions(-) create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerConsumerSpecificationRepositoryTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ConsumerRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ConsumerRepository.java index ac6f79a..e9f7c3b 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ConsumerRepository.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ConsumerRepository.java @@ -7,16 +7,30 @@ package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; import java.util.List; +import org.springframework.data.jpa.domain.Specification; +import org.springframework.data.jpa.repository.EntityGraph; import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.JpaSpecificationExecutor; import org.springframework.data.jpa.repository.Query; import org.springframework.stereotype.Repository; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; @Repository -public interface ConsumerRepository extends JpaRepository { +public interface ConsumerRepository extends JpaRepository, JpaSpecificationExecutor { List findByIdpClientId(String clientId); + /** + * {@inheritDoc} + * + *

Fetches {@code productConsumers} alongside each match, mirroring the {@code JOIN FETCH} + * the plain {@code @Query} methods use - {@link JpaSpecificationExecutor}'s base + * implementation does not fetch-join by default. + */ + @Override + @EntityGraph(attributePaths = {"productConsumers"}) + List findAll(Specification spec); + /** * Retrieves a list of {@link Consumer} entities associated with the specified provider IDs. * The method performs a query to fetch consumers linked with products that correspond to the given provider IDs. diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerRepository.java index 0509622..5a4feb3 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerRepository.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerRepository.java @@ -7,7 +7,10 @@ package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; import java.util.List; +import org.springframework.data.jpa.domain.Specification; +import org.springframework.data.jpa.repository.EntityGraph; import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.JpaSpecificationExecutor; import org.springframework.data.jpa.repository.Query; import org.springframework.stereotype.Repository; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; @@ -22,7 +25,19 @@ * entity with the identifier type {@link Long}. */ @Repository -public interface ProducerRepository extends JpaRepository { +public interface ProducerRepository extends JpaRepository, JpaSpecificationExecutor { + + /** + * {@inheritDoc} + * + *

Fetches {@code products} alongside each match so the filtered configuration path does + * not lazily N+1-load them the way {@link #findByIdpClientId} avoids it via {@code JOIN + * FETCH} - {@link JpaSpecificationExecutor}'s base implementation does not fetch-join by + * default. + */ + @Override + @EntityGraph(attributePaths = {"products", "products.productType"}) + List findAll(Specification spec); /** * Retrieves a list of {@link Producer} entities, including their associated {@link Product} entities diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ConsumerService.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ConsumerService.java index 918b5e5..85edb37 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ConsumerService.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ConsumerService.java @@ -9,7 +9,9 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import org.springframework.data.jpa.domain.Specification; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerDTO; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; /** * Service interface for managing ConsumerId entities. @@ -31,6 +33,15 @@ public interface ConsumerService { */ List findByIdpClientId(String idpClientId); + /** + * Retrieves consumers for a client, additionally constrained by a compiled caller filter. + * + * @param idpClientId the IDP client ID to scope by + * @param filter an additional predicate, AND-ed with the client scoping; {@code null} for none + * @return consumers matching both the client scope and the filter + */ + List findByIdpClientId(String idpClientId, Specification filter); + /** * Retrieves a map of consumers identified by their client_id * diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProducerService.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProducerService.java index b0f4972..74ff606 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProducerService.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProducerService.java @@ -7,7 +7,9 @@ package uk.gov.dbt.ndtp.ia.node.management.service.data; import java.util.List; +import org.springframework.data.jpa.domain.Specification; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerDTO; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; /** * Service interface for managing OrganisationProducer entities. @@ -23,4 +25,13 @@ public interface ProducerService { List getProducersByConsumerIds(List producerIds); List getProducersByClientId(String clientId); + + /** + * Retrieves producers for a client, additionally constrained by a compiled caller filter. + * + * @param clientId the IDP client ID to scope by + * @param filter an additional predicate, AND-ed with the client scoping; {@code null} for none + * @return producers matching both the client scope and the filter + */ + List getProducersByClientId(String clientId, Specification filter); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImpl.java index bbcb8bf..f643f1e 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImpl.java @@ -10,6 +10,7 @@ import java.util.Map; import java.util.Optional; import java.util.stream.Collectors; +import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ConsumerConverter; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerDTO; @@ -52,6 +53,14 @@ public List findByIdpClientId(String idpClientId) { return consumerIdConverter.toDtoList(consumers); } + @Override + public List findByIdpClientId(String idpClientId, Specification filter) { + Specification clientScoped = (root, query, cb) -> cb.equal(root.get("idpClientId"), idpClientId); + Specification combined = filter == null ? clientScoped : clientScoped.and(filter); + List consumers = consumerRepository.findAll(combined); + return consumerIdConverter.toDtoList(consumers); + } + @Override public Map> getConsumersOfProviders(List providers) { List consumers = consumerRepository.findConsumersByProviderIds(providers); diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImpl.java index ea33fd8..f396681 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImpl.java @@ -7,6 +7,7 @@ package uk.gov.dbt.ndtp.ia.node.management.service.data.impl; import java.util.List; +import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.OrganisationProducerConverter; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerDTO; @@ -51,4 +52,12 @@ public List getProducersByClientId(String clientId) { List producers = producerRepository.findByIdpClientId(clientId); return organisationProducerConverter.toDtoList(producers); } + + @Override + public List getProducersByClientId(String clientId, Specification filter) { + Specification clientScoped = (root, query, cb) -> cb.equal(root.get("idpClientId"), clientId); + Specification combined = filter == null ? clientScoped : clientScoped.and(filter); + List producers = producerRepository.findAll(combined); + return organisationProducerConverter.toDtoList(producers); + } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerConsumerSpecificationRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerConsumerSpecificationRepositoryTest.java new file mode 100644 index 0000000..46bf6d9 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerConsumerSpecificationRepositoryTest.java @@ -0,0 +1,120 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import static org.assertj.core.api.Assertions.assertThat; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityManagerFactory; +import org.hibernate.SessionFactory; +import org.hibernate.stat.Statistics; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.jpa.domain.Specification; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; +import org.springframework.transaction.annotation.Transactional; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Product; + +/** + * Verifies task 4.1 (repositories are wired for {@link Specification}-based queries) and task 4.3 + * (the fetch gap left by moving off {@code JOIN FETCH} is closed with an {@code @EntityGraph} so + * the filtered path does not N+1-load {@code products}/{@code productConsumers}). + */ +@Transactional +class ProducerConsumerSpecificationRepositoryTest extends AbstractPostgresRepositoryTest { + + @DynamicPropertySource + static void statisticsProperty(DynamicPropertyRegistry registry) { + registry.add("spring.jpa.properties.hibernate.generate_statistics", () -> "true"); + } + + @Autowired + private EntityManager entityManager; + + @Autowired + private EntityManagerFactory entityManagerFactory; + + @Autowired + private ProducerRepository producerRepository; + + @Autowired + private ConsumerRepository consumerRepository; + + private Statistics statistics() { + return entityManagerFactory.unwrap(SessionFactory.class).getStatistics(); + } + + @Test + void producerFindAllBySpecification_matchesExpectedRowsAndFetchJoinsProducts() { + Organisation org = new Organisation(); + org.setName("spec-org"); + entityManager.persist(org); + + Producer producer = new Producer(); + producer.setName("spec-producer"); + producer.setDescription("test"); + producer.setOrg(org); + producer.setActive(true); + producer.setHost("host.example"); + producer.setPort(java.math.BigDecimal.valueOf(443)); + producer.setTls(true); + producer.setIdpClientId("spec-producer-client"); + entityManager.persist(producer); + + Product product = new Product(); + product.setName("spec-product"); + product.setTopic("spec-topic"); + product.setProducer(producer); + entityManager.persist(product); + + entityManager.flush(); + entityManager.clear(); + statistics().clear(); + + Specification byId = (root, query, cb) -> cb.equal(root.get("id"), producer.getId()); + java.util.List results = producerRepository.findAll(byId); + + assertThat(results).hasSize(1); + // Accessing products must not trigger an additional lazy-load query - proves the + // @EntityGraph fetch-join, not N+1, populated the association. + assertThat(results.getFirst().getProducts()).hasSize(1); + assertThat(statistics().getQueryExecutionCount()).isEqualTo(1); + } + + @Test + void consumerFindAllBySpecification_returnsMatchingRowOnly() { + Organisation org = new Organisation(); + org.setName("spec-consumer-org"); + entityManager.persist(org); + + Consumer matching = new Consumer(); + matching.setName("matching-consumer"); + matching.setScheduleType("cron"); + matching.setOrg(org); + matching.setIdpClientId("spec-consumer-client"); + entityManager.persist(matching); + + Consumer other = new Consumer(); + other.setName("other-consumer"); + other.setScheduleType("cron"); + other.setOrg(org); + other.setIdpClientId("other-consumer-client"); + entityManager.persist(other); + + entityManager.flush(); + entityManager.clear(); + + Specification byName = (root, query, cb) -> cb.equal(root.get("name"), "matching-consumer"); + java.util.List results = consumerRepository.findAll(byName); + + assertThat(results).extracting(Consumer::getName).containsExactly("matching-consumer"); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImplTest.java index 752bd27..7e6234f 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImplTest.java @@ -16,9 +16,11 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.data.jpa.domain.Specification; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ConsumerConverter; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerDTO; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; @@ -157,6 +159,37 @@ void getConsumersOfProviders_withValidProviderIds_shouldReturnMappedConsumers() verify(consumerConverter).toDto(consumer); } + @Test + void findByIdpClientId_withFilter_combinesClientScopingAndFilterViaAnd() { + Specification callerFilter = mock(Specification.class); + List consumers = List.of(consumer); + List consumerDTOs = List.of(consumerDTO); + + when(consumerRepository.findAll(any(Specification.class))).thenReturn(consumers); + when(consumerConverter.toDtoList(consumers)).thenReturn(consumerDTOs); + + List result = consumerService.findByIdpClientId(idpClientId, callerFilter); + + assertEquals(consumerDTOs, result); + ArgumentCaptor> captor = ArgumentCaptor.forClass(Specification.class); + verify(consumerRepository).findAll(captor.capture()); + assertNotEquals(callerFilter, captor.getValue()); + } + + @Test + void findByIdpClientId_withNullFilter_stillScopesByClient() { + List consumers = List.of(consumer); + List consumerDTOs = List.of(consumerDTO); + + when(consumerRepository.findAll(any(Specification.class))).thenReturn(consumers); + when(consumerConverter.toDtoList(consumers)).thenReturn(consumerDTOs); + + List result = consumerService.findByIdpClientId(idpClientId, null); + + assertEquals(consumerDTOs, result); + verify(consumerRepository).findAll(any(Specification.class)); + } + @Test void getConsumersOfProviders_withEmptyProviderIds_shouldReturnEmptyMap() { // Arrange diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImplTest.java index 5470f34..39fd1d7 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImplTest.java @@ -14,9 +14,11 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.data.jpa.domain.Specification; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.OrganisationProducerConverter; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerDTO; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; @@ -150,4 +152,36 @@ void getProducersByClientId_withNonExistingClientId_shouldReturnEmptyList() { verify(producerRepository).findByIdpClientId(nonExistingClientId); verify(organisationProducerConverter).toDtoList(emptyProducers); } + + @Test + void getProducersByClientId_withFilter_combinesClientScopingAndFilterViaAnd() { + Specification callerFilter = mock(Specification.class); + List producers = List.of(producer); + List producerDTOs = List.of(producerDTO); + + when(producerRepository.findAll(any(Specification.class))).thenReturn(producers); + when(organisationProducerConverter.toDtoList(producers)).thenReturn(producerDTOs); + + List result = producerService.getProducersByClientId(clientId, callerFilter); + + assertEquals(producerDTOs, result); + ArgumentCaptor> captor = ArgumentCaptor.forClass(Specification.class); + verify(producerRepository).findAll(captor.capture()); + // The combined specification must AND the caller filter with client scoping, not replace it. + assertNotEquals(callerFilter, captor.getValue()); + } + + @Test + void getProducersByClientId_withNullFilter_stillScopesByClient() { + List producers = List.of(producer); + List producerDTOs = List.of(producerDTO); + + when(producerRepository.findAll(any(Specification.class))).thenReturn(producers); + when(organisationProducerConverter.toDtoList(producers)).thenReturn(producerDTOs); + + List result = producerService.getProducersByClientId(clientId, null); + + assertEquals(producerDTOs, result); + verify(producerRepository).findAll(any(Specification.class)); + } } From ea68b4408da94b919194b4609eff37eb38dd4d91 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 14:50:43 +0200 Subject: [PATCH 16/22] feat(config): wire optional filter query parameter into configuration 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. --- .../v1/ConfigurationController.java | 31 +++++-- .../handlers/GlobalExceptionHandler.java | 42 ++++++++++ .../filter/FilterRequestParser.java | 67 ++++++++++++++++ .../configuration/ConfigurationProvider.java | 25 ++++++ .../ConfigurationProviderImpl.java | 80 +++++++++++++------ .../v1/ConfigurationControllerTest.java | 68 +++++++++++++++- ...ationPolicyEnforcementIntegrationTest.java | 12 +-- .../handlers/GlobalExceptionHandlerTest.java | 41 ++++++++++ .../filter/FilterRequestParserTest.java | 79 ++++++++++++++++++ .../ConfigurationProviderImplTest.java | 50 ++++++++---- 10 files changed, 441 insertions(+), 54 deletions(-) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationController.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationController.java index 88e12cf..ad84dfe 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationController.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationController.java @@ -21,6 +21,8 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterNode; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterRequestParser; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerConfigDTO; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerConfigDTO; import uk.gov.dbt.ndtp.ia.node.management.model.jwt.EnhancedPrincipal; @@ -33,9 +35,12 @@ public class ConfigurationController { private final ConfigurationProvider configurationProvider; + private final FilterRequestParser filterRequestParser; - public ConfigurationController(ConfigurationProvider configurationProvider) { + public ConfigurationController( + ConfigurationProvider configurationProvider, FilterRequestParser filterRequestParser) { this.configurationProvider = configurationProvider; + this.filterRequestParser = filterRequestParser; } @GetMapping("/producer") @@ -61,10 +66,18 @@ public ProducerConfigDTO getProducerConfigurations( @Parameter(hidden = true) @AuthenticationPrincipal EnhancedPrincipal principal, @Parameter(name = "producer_id", description = "Optional Producer identifier to filter configuration") @RequestParam(value = "producer_id", required = false) - Long producerId) { + Long producerId, + @Parameter( + name = "filter", + description = + "Optional JSON-encoded filter (FilterNode: a Comparison or a Group of them) narrowing" + + " the returned producers, evaluated by the database alongside producer_id") + @RequestParam(value = "filter", required = false) + String filter) { log.info("Preparing Federator Producer Config for producer {}", producerId); + Optional filterNode = filterRequestParser.parse(filter); return configurationProvider.getProducerConfigByClientId( - principal.clientId(), producerId != null ? Optional.of(producerId) : Optional.empty()); + principal.clientId(), producerId != null ? Optional.of(producerId) : Optional.empty(), filterNode); } @GetMapping("/consumer") @@ -90,10 +103,18 @@ public ConsumerConfigDTO getConsumerConfigurations( @Parameter(hidden = true) @AuthenticationPrincipal EnhancedPrincipal principal, @Parameter(name = "consumer_id", description = "Optional Consumer identifier to filter configuration") @RequestParam(value = "consumer_id", required = false) - Long consumerId) { + Long consumerId, + @Parameter( + name = "filter", + description = + "Optional JSON-encoded filter (FilterNode: a Comparison or a Group of them) narrowing" + + " the returned consumers, evaluated by the database alongside consumer_id") + @RequestParam(value = "filter", required = false) + String filter) { log.info("Preparing Consumer Config for client Id {} and Consumer {}", principal.clientId(), consumerId); + Optional filterNode = filterRequestParser.parse(filter); return configurationProvider.getConsumerConfigByClientId( - principal.clientId(), consumerId != null ? Optional.of(consumerId) : Optional.empty()); + principal.clientId(), consumerId != null ? Optional.of(consumerId) : Optional.empty(), filterNode); } } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java index 4622403..400f19d 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java @@ -19,6 +19,7 @@ import uk.gov.dbt.ndtp.ia.node.management.exception.CertificateSigningException; import uk.gov.dbt.ndtp.ia.node.management.exception.ErrorResponse; import uk.gov.dbt.ndtp.ia.node.management.exception.PkiException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; /** * Global exception handler for the application. @@ -136,6 +137,47 @@ public ResponseEntity handlePkiException(PkiException ex, WebRequ return new ResponseEntity<>(errorResponse, HttpStatus.INTERNAL_SERVER_ERROR); } + /** + * Handles FilterCompilationException raised while validating/compiling a caller-supplied + * configuration filter. {@code Origin.REQUEST} - a malformed or unknown-attribute filter - + * maps to 400 with the exception's own message, which by construction names only the + * caller-supplied attribute, never an internal table/column name. {@code Origin.POLICY} - an + * attribute definition or stored value this system's own configuration cannot honour - maps + * to 500 with a generic message, consistent with the other 500 handlers below: the detail + * stays server-side, in the log. + * + * @param ex the exception + * @param request the current request + * @return a ResponseEntity with a 400 or 500 error message depending on {@link + * FilterCompilationException#origin()} + */ + @ExceptionHandler(FilterCompilationException.class) + public ResponseEntity handleFilterCompilationException( + FilterCompilationException ex, WebRequest request) { + + String errorId = generateErrorId(); + + if (ex.origin() == FilterCompilationException.Origin.REQUEST) { + log.debug( + "Rejected caller filter, error_id={}, path={}: {}", + errorId, + request.getDescription(false), + ex.getMessage()); + ErrorResponse errorResponse = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), ex.getMessage(), errorId); + return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST); + } + + log.error( + "Filter attribute configuration defect, error_id={}, path={}: {}", + errorId, + request.getDescription(false), + ex.getMessage(), + ex); + ErrorResponse errorResponse = new ErrorResponse( + HttpStatus.INTERNAL_SERVER_ERROR.value(), "An internal server error occurred", errorId); + return new ResponseEntity<>(errorResponse, HttpStatus.INTERNAL_SERVER_ERROR); + } + /** * Handles RuntimeException. * diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java new file mode 100644 index 0000000..c38cb4d --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java @@ -0,0 +1,67 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.Optional; +import org.springframework.stereotype.Component; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; + +/** + * Parses a caller-supplied, JSON-encoded {@code filter} query parameter into a {@link + * FilterNode}, rejecting malformed JSON and an over-large filter before any attribute + * resolution or query runs. + */ +@Component +public class FilterRequestParser { + + /** Mirrors {@code opa_poc.api.SearchRequest}'s cap of 20 filters per request. */ + static final int MAX_COMPARISONS = 20; + + private final ObjectMapper objectMapper; + + public FilterRequestParser(ObjectMapper objectMapper) { + this.objectMapper = objectMapper; + } + + /** + * @param rawJson the raw {@code filter} query parameter value, or {@code null}/blank for none + * @return empty when no filter was supplied + * @throws FilterCompilationException with {@code Origin.REQUEST} if the JSON is malformed or + * the filter contains more than {@value #MAX_COMPARISONS} comparisons + */ + public Optional parse(String rawJson) { + if (rawJson == null || rawJson.isBlank()) { + return Optional.empty(); + } + FilterNode node; + try { + node = objectMapper.readValue(rawJson, FilterNode.class); + } catch (JsonProcessingException e) { + throw new FilterCompilationException(Origin.REQUEST, "Malformed filter: could not parse JSON"); + } + int comparisons = countComparisons(node); + if (comparisons > MAX_COMPARISONS) { + throw new FilterCompilationException( + Origin.REQUEST, + "A filter may combine at most " + MAX_COMPARISONS + " comparisons, found " + comparisons); + } + return Optional.of(node); + } + + private static int countComparisons(FilterNode node) { + return switch (node) { + case FilterNode.Comparison ignored -> 1; + case FilterNode.Literal ignored -> 0; + case FilterNode.Group group -> + group.nodes().stream() + .mapToInt(FilterRequestParser::countComparisons) + .sum(); + }; + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProvider.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProvider.java index 374fcf1..0f5397f 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProvider.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProvider.java @@ -7,6 +7,7 @@ package uk.gov.dbt.ndtp.ia.node.management.service.providers.configuration; import java.util.Optional; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterNode; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerConfigDTO; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerConfigDTO; @@ -33,6 +34,18 @@ public interface ConfigurationProvider { */ ConsumerConfigDTO getConsumerConfigByClientId(String clientId, Optional consumerId); + /** + * Retrieves the configuration for a consumer organization, additionally constrained by a + * caller-supplied filter conjoined with the existing {@code clientId}/{@code consumerId} scoping. + * + * @param clientId The unique identifier for the consumer organization. Must not be null or blank. + * @param consumerId An optional identifier for the consumer. + * @param filter An optional validated caller filter, compiled and applied at the database level. + * @return The configuration settings for the specified consumer organization. + */ + ConsumerConfigDTO getConsumerConfigByClientId( + String clientId, Optional consumerId, Optional filter); + /** * Retrieves the configuration for a producer organization identified by the given client ID. * @@ -43,4 +56,16 @@ public interface ConfigurationProvider { * @throws RuntimeException if the configuration cannot be retrieved due to system errors. */ ProducerConfigDTO getProducerConfigByClientId(String clientId, Optional producerId); + + /** + * Retrieves the configuration for a producer organization, additionally constrained by a + * caller-supplied filter conjoined with the existing {@code clientId}/{@code producerId} scoping. + * + * @param clientId The unique identifier for the producer organization. Must not be null or blank. + * @param producerId An optional identifier for the producer. + * @param filter An optional validated caller filter, compiled and applied at the database level. + * @return The configuration settings for the specified producer organization. + */ + ProducerConfigDTO getProducerConfigByClientId( + String clientId, Optional producerId, Optional filter); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java index 68d790c..7295388 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java @@ -18,8 +18,14 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterNode; +import uk.gov.dbt.ndtp.ia.node.management.filter.compiler.SpecificationPredicateCompiler; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ResourceType; import uk.gov.dbt.ndtp.ia.node.management.model.dto.*; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; import uk.gov.dbt.ndtp.ia.node.management.service.data.ConsumerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProducerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductConsumerService; @@ -39,6 +45,8 @@ public class ConfigurationProviderImpl implements ConfigurationProvider { private final CertificateValidationProvider certificateValidationProvider; + private final SpecificationPredicateCompiler specificationPredicateCompiler; + /** * Constructs a new ConfigurationProviderImpl with required services. * @@ -46,17 +54,20 @@ public class ConfigurationProviderImpl implements ConfigurationProvider { * @param consumerAllowedDataProviders the product consumer service * @param producerService the producer service * @param certificateValidationProvider the certificate validation provider + * @param specificationPredicateCompiler compiles a caller filter into a database predicate */ public ConfigurationProviderImpl( ConsumerService consumerService, ProductConsumerService consumerAllowedDataProviders, ProducerService producerService, - CertificateValidationProvider certificateValidationProvider) { + CertificateValidationProvider certificateValidationProvider, + SpecificationPredicateCompiler specificationPredicateCompiler) { this.consumerService = consumerService; this.productConsumerService = consumerAllowedDataProviders; this.producerService = producerService; this.certificateValidationProvider = certificateValidationProvider; + this.specificationPredicateCompiler = specificationPredicateCompiler; } /** @@ -76,7 +87,13 @@ private static boolean isValidGrantedTs(Timestamp grantedTs, BigDecimal validity @Override public ConsumerConfigDTO getConsumerConfigByClientId(String clientId, Optional consumerId) { - List consumers = getFilteredConsumers(clientId, consumerId); + return getConsumerConfigByClientId(clientId, consumerId, Optional.empty()); + } + + @Override + public ConsumerConfigDTO getConsumerConfigByClientId( + String clientId, Optional consumerId, Optional filter) { + List consumers = getFilteredConsumers(clientId, consumerId, filter); List consumerIds = consumers.stream().map(ConsumerDTO::getId).toList(); List validProductConsumers = getValidProductConsumers(consumers); @@ -144,7 +161,13 @@ private List getValidProductConsumers(List cons @Override public ProducerConfigDTO getProducerConfigByClientId(String clientId, Optional producerId) { - List producers = getFilteredActiveProducers(clientId, producerId); + return getProducerConfigByClientId(clientId, producerId, Optional.empty()); + } + + @Override + public ProducerConfigDTO getProducerConfigByClientId( + String clientId, Optional producerId, Optional filter) { + List producers = getFilteredActiveProducers(clientId, producerId, filter); List dataProviderIds = collectDataProviderIds(producers); // Get allowed consumers (not directly used but might be needed for side effects) @@ -159,43 +182,54 @@ public ProducerConfigDTO getProducerConfigByClientId(String clientId, Optional getFilteredConsumers(String clientId, Optional consumerId) { - List consumers = consumerService.findByIdpClientId(clientId); - - if (consumerId.isPresent()) { - consumers = consumers.stream() - .filter(consumer -> consumer.getId().equals(consumerId.get())) - .toList(); - } - - return consumers; + private List getFilteredConsumers( + String clientId, Optional consumerId, Optional filter) { + Specification idAndFilterSpec = idAndFilterSpecification(consumerId, filter, ResourceType.CONSUMER); + return consumerService.findByIdpClientId(clientId, idAndFilterSpec); } /** - * Filters active producers by client ID and optional producer ID. + * Filters active producers by client ID, optional producer ID, and an optional caller + * filter - id/filter narrowing is pushed to the database as a single {@link Specification}; + * the {@code active} business rule stays a post-fetch Java filter, unchanged from before. * * @param clientId the client ID * @param producerId the optional producer ID + * @param filter an optional validated caller filter * @return a list of filtered active producers */ - private List getFilteredActiveProducers(String clientId, Optional producerId) { - List producers = producerService.getProducersByClientId(clientId).stream() + private List getFilteredActiveProducers( + String clientId, Optional producerId, Optional filter) { + Specification idAndFilterSpec = idAndFilterSpecification(producerId, filter, ResourceType.PRODUCER); + return producerService.getProducersByClientId(clientId, idAndFilterSpec).stream() .filter(ProducerDTO::getActive) .toList(); + } - if (producerId.isPresent()) { - producers = producers.stream() - .filter(producer -> producerId.get().equals(producer.getId())) - .toList(); + /** + * Builds the id-equality predicate and/or the compiled caller-filter predicate, AND-ed + * together. Returns {@code null} (no additional restriction beyond client scoping, applied + * by the service layer) when neither is present - preserving pre-existing unfiltered + * behaviour. + */ + private Specification idAndFilterSpecification( + Optional id, Optional filter, ResourceType resourceType) { + Specification spec = id.map(value -> (Specification) (root, query, cb) -> cb.equal(root.get("id"), value)) + .orElse(null); + if (filter.isPresent()) { + Specification filterSpec = specificationPredicateCompiler.compile(resourceType, filter.get()); + spec = spec == null ? filterSpec : spec.and(filterSpec); } - - return producers; + return spec; } /** diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationControllerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationControllerTest.java index a0a667e..41e551c 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationControllerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationControllerTest.java @@ -12,6 +12,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import com.fasterxml.jackson.databind.ObjectMapper; import java.util.ArrayList; import java.util.Collections; import org.junit.jupiter.api.BeforeEach; @@ -23,6 +24,8 @@ import org.springframework.http.MediaType; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import uk.gov.dbt.ndtp.ia.node.management.exception.handlers.GlobalExceptionHandler; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterRequestParser; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerConfigDTO; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerConfigDTO; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerDTO; @@ -36,6 +39,9 @@ class ConfigurationControllerTest { @Mock private ConfigurationProvider configurationProvider; + @Mock + private FilterRequestParser filterRequestParser; + @InjectMocks private ConfigurationController configurationController; @@ -46,9 +52,15 @@ class ConfigurationControllerTest { private ProducerConfigDTO producerConfigDTO; private ConsumerConfigDTO consumerConfigDTO; + private MockMvc mockMvcWithRealFilterParsing; + @BeforeEach void setUp() { mockMvc = MockMvcBuilders.standaloneSetup(configurationController).build(); + mockMvcWithRealFilterParsing = MockMvcBuilders.standaloneSetup( + new ConfigurationController(configurationProvider, new FilterRequestParser(new ObjectMapper()))) + .setControllerAdvice(new GlobalExceptionHandler()) + .build(); // Set up producer config ProducerDTO producerDTO = ProducerDTO.builder() @@ -81,7 +93,8 @@ void setUp() { @Test void getProducerConfigurations_shouldReturnConfig() throws Exception { // Arrange - when(configurationProvider.getProducerConfigByClientId(any(), any())).thenReturn(producerConfigDTO); + when(configurationProvider.getProducerConfigByClientId(any(), any(), any())) + .thenReturn(producerConfigDTO); // Act & Assert mockMvc.perform(get("/api/v1/configuration/producer").contentType(MediaType.APPLICATION_JSON)) @@ -92,7 +105,8 @@ void getProducerConfigurations_shouldReturnConfig() throws Exception { @Test void getProducerConfigurations_withProducerId_shouldReturnFilteredConfig() throws Exception { // Arrange - when(configurationProvider.getProducerConfigByClientId(any(), any())).thenReturn(producerConfigDTO); + when(configurationProvider.getProducerConfigByClientId(any(), any(), any())) + .thenReturn(producerConfigDTO); // Act & Assert mockMvc.perform(get("/api/v1/configuration/producer") @@ -105,7 +119,8 @@ void getProducerConfigurations_withProducerId_shouldReturnFilteredConfig() throw @Test void getConsumerConfigurations_shouldReturnConfig() throws Exception { // Arrange - when(configurationProvider.getConsumerConfigByClientId(any(), any())).thenReturn(consumerConfigDTO); + when(configurationProvider.getConsumerConfigByClientId(any(), any(), any())) + .thenReturn(consumerConfigDTO); // Act & Assert mockMvc.perform(get("/api/v1/configuration/consumer").contentType(MediaType.APPLICATION_JSON)) @@ -116,7 +131,8 @@ void getConsumerConfigurations_shouldReturnConfig() throws Exception { @Test void getConsumerConfigurations_withConsumerId_shouldReturnFilteredConfig() throws Exception { // Arrange - when(configurationProvider.getConsumerConfigByClientId(any(), any())).thenReturn(consumerConfigDTO); + when(configurationProvider.getConsumerConfigByClientId(any(), any(), any())) + .thenReturn(consumerConfigDTO); // Act & Assert mockMvc.perform(get("/api/v1/configuration/consumer") @@ -125,4 +141,48 @@ void getConsumerConfigurations_withConsumerId_shouldReturnFilteredConfig() throw .andExpect(status().isOk()) .andExpect(jsonPath("$.clientId").value(clientId)); } + + @Test + void getProducerConfigurations_withMalformedFilter_returnsBadRequest() throws Exception { + mockMvcWithRealFilterParsing + .perform(get("/api/v1/configuration/producer") + .param("filter", "{ not valid json") + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isBadRequest()); + } + + @Test + void getProducerConfigurations_withValidFilter_reachesProviderAndReturnsConfig() throws Exception { + when(configurationProvider.getProducerConfigByClientId(any(), any(), any())) + .thenReturn(producerConfigDTO); + + mockMvcWithRealFilterParsing + .perform(get("/api/v1/configuration/producer") + .param( + "filter", + "{\"type\":\"comparison\",\"attribute\":\"active\",\"operator\":\"eq\",\"values\":[true]}") + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.clientId").value(clientId)); + } + + @Test + void getConsumerConfigurations_withMalformedFilter_returnsBadRequest() throws Exception { + mockMvcWithRealFilterParsing + .perform(get("/api/v1/configuration/consumer") + .param("filter", "{ not valid json") + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isBadRequest()); + } + + @Test + void getConsumerConfigurations_withNoFilter_behavesLikeBeforeThisChange() throws Exception { + when(configurationProvider.getConsumerConfigByClientId(any(), any(), any())) + .thenReturn(consumerConfigDTO); + + mockMvcWithRealFilterParsing + .perform(get("/api/v1/configuration/consumer").contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.clientId").value(clientId)); + } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationPolicyEnforcementIntegrationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationPolicyEnforcementIntegrationTest.java index 3ef2a28..0f22cfe 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationPolicyEnforcementIntegrationTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationPolicyEnforcementIntegrationTest.java @@ -26,6 +26,7 @@ import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.setup.MockMvcBuilders; import uk.gov.dbt.ndtp.ia.node.management.config.PolicyEnforcementInterceptor; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterRequestParser; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerConfigDTO; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerConfigDTO; import uk.gov.dbt.ndtp.ia.node.management.model.jwt.EnhancedPrincipal; @@ -51,7 +52,8 @@ class ConfigurationPolicyEnforcementIntegrationTest { @BeforeEach void setUp() { - ConfigurationController controller = new ConfigurationController(configurationProvider); + ConfigurationController controller = + new ConfigurationController(configurationProvider, new FilterRequestParser(new ObjectMapper())); PolicyEnforcementInterceptor interceptor = new PolicyEnforcementInterceptor(policyDecisionClient, new ObjectMapper()); mockMvc = MockMvcBuilders.standaloneSetup(controller) @@ -77,7 +79,7 @@ private void authenticateAs(String clientId) { void allowedRequest_reachesControllerAndReturnsConfig() throws Exception { authenticateAs("client-1"); when(policyDecisionClient.evaluate(any())).thenReturn(PolicyDecision.ALLOW); - when(configurationProvider.getProducerConfigByClientId(any(), any())) + when(configurationProvider.getProducerConfigByClientId(any(), any(), any())) .thenReturn(ProducerConfigDTO.builder() .clientId("client-1") .producers(Collections.emptyList()) @@ -87,7 +89,7 @@ void allowedRequest_reachesControllerAndReturnsConfig() throws Exception { .andExpect(status().isOk()) .andExpect(jsonPath("$.clientId").value("client-1")); - verify(configurationProvider).getProducerConfigByClientId(any(), any()); + verify(configurationProvider).getProducerConfigByClientId(any(), any(), any()); } @Test @@ -114,7 +116,7 @@ void deniedRequest_onProducerEndpoint_rejectedBeforeReachingController() throws void allowedRequest_onConsumerEndpoint_reachesControllerAndReturnsConfig() throws Exception { authenticateAs("client-1"); when(policyDecisionClient.evaluate(any())).thenReturn(PolicyDecision.ALLOW); - when(configurationProvider.getConsumerConfigByClientId(any(), any())) + when(configurationProvider.getConsumerConfigByClientId(any(), any(), any())) .thenReturn(ConsumerConfigDTO.builder() .clientId("client-1") .producers(Collections.emptyList()) @@ -124,7 +126,7 @@ void allowedRequest_onConsumerEndpoint_reachesControllerAndReturnsConfig() throw .andExpect(status().isOk()) .andExpect(jsonPath("$.clientId").value("client-1")); - verify(configurationProvider).getConsumerConfigByClientId(any(), any()); + verify(configurationProvider).getConsumerConfigByClientId(any(), any(), any()); } @Test diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandlerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandlerTest.java index be749c8..02e3891 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandlerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandlerTest.java @@ -21,6 +21,8 @@ import uk.gov.dbt.ndtp.ia.node.management.exception.AuthenticationProcessingException; import uk.gov.dbt.ndtp.ia.node.management.exception.ErrorResponse; import uk.gov.dbt.ndtp.ia.node.management.exception.JwtClaimParsingException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; /** * Tests for the GlobalExceptionHandler class. @@ -134,6 +136,45 @@ void handleAllExceptions_shouldReturnInternalServerErrorStatus() { assertNotNull(errorResponse.getErrorId()); } + @Test + void handleFilterCompilationException_withRequestOrigin_shouldReturnBadRequestWithExceptionMessage() { + // Arrange + String message = "Unknown attribute 'nope' for resource type 'PRODUCER'"; + FilterCompilationException exception = new FilterCompilationException(Origin.REQUEST, message); + + // Act + ResponseEntity response = + exceptionHandler.handleFilterCompilationException(exception, webRequest); + + // Assert + assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); + ErrorResponse errorResponse = response.getBody(); + assertNotNull(errorResponse); + assertEquals(HttpStatus.BAD_REQUEST.value(), errorResponse.getStatus()); + assertEquals(message, errorResponse.getMessage()); + assertNotNull(errorResponse.getErrorId()); + } + + @Test + void handleFilterCompilationException_withPolicyOrigin_shouldReturnInternalServerErrorWithGenericMessage() { + // Arrange + String internalMessage = "Attribute definition declares unsupported data_type 'XML'"; + FilterCompilationException exception = new FilterCompilationException(Origin.POLICY, internalMessage); + + // Act + ResponseEntity response = + exceptionHandler.handleFilterCompilationException(exception, webRequest); + + // Assert + assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, response.getStatusCode()); + ErrorResponse errorResponse = response.getBody(); + assertNotNull(errorResponse); + assertEquals(HttpStatus.INTERNAL_SERVER_ERROR.value(), errorResponse.getStatus()); + // The config-defect detail stays server-side (in the log), never in the response body. + assertEquals("An internal server error occurred", errorResponse.getMessage()); + assertNotNull(errorResponse.getErrorId()); + } + @Test void handleNoResourceFoundException_shouldReturnNotFoundStatus() { // Arrange diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java new file mode 100644 index 0000000..2d975fc --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java @@ -0,0 +1,79 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import org.junit.jupiter.api.Test; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; + +class FilterRequestParserTest { + + private final FilterRequestParser parser = new FilterRequestParser(new ObjectMapper()); + + @Test + void parse_returnsEmptyForNull() { + assertThat(parser.parse(null)).isEmpty(); + } + + @Test + void parse_returnsEmptyForBlank() { + assertThat(parser.parse(" ")).isEmpty(); + } + + @Test + void parse_parsesValidComparison() { + String json = + """ + { "type": "comparison", "attribute": "active", "operator": "eq", "values": [true] } + """; + + Optional node = parser.parse(json); + + assertThat(node).isPresent().get().isInstanceOf(FilterNode.Comparison.class); + } + + @Test + void parse_rejectsMalformedJson() { + assertThatThrownBy(() -> parser.parse("{ not json")) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void parse_rejectsFilterOverComparisonCap() throws Exception { + List nodes = new ArrayList<>(); + for (int i = 0; i < FilterRequestParser.MAX_COMPARISONS + 1; i++) { + nodes.add(FilterNode.Comparison.of("active", ComparisonOperator.EQ, true)); + } + FilterNode.Group group = FilterNode.Group.and(nodes); + String json = new ObjectMapper().writeValueAsString(group); + + assertThatThrownBy(() -> parser.parse(json)) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void parse_acceptsFilterAtComparisonCap() throws Exception { + List nodes = new ArrayList<>(); + for (int i = 0; i < FilterRequestParser.MAX_COMPARISONS; i++) { + nodes.add(FilterNode.Comparison.of("active", ComparisonOperator.EQ, true)); + } + FilterNode.Group group = FilterNode.Group.and(nodes); + String json = new ObjectMapper().writeValueAsString(group); + + assertThat(parser.parse(json)).isPresent(); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java index 27a429e..b574cb2 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java @@ -25,6 +25,7 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import uk.gov.dbt.ndtp.ia.node.management.filter.compiler.SpecificationPredicateCompiler; import uk.gov.dbt.ndtp.ia.node.management.model.dto.*; import uk.gov.dbt.ndtp.ia.node.management.service.data.ConsumerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProducerService; @@ -45,6 +46,9 @@ class ConfigurationProviderImplTest { @Mock private CertificateValidationProvider certificateValidationProvider; + @Mock + private SpecificationPredicateCompiler specificationPredicateCompiler; + @InjectMocks private ConfigurationProviderImpl configurationProvider; @@ -52,7 +56,11 @@ class ConfigurationProviderImplTest { void setUp() { MockitoAnnotations.openMocks(this); configurationProvider = new ConfigurationProviderImpl( - consumerService, productConsumerService, producerService, certificateValidationProvider); + consumerService, + productConsumerService, + producerService, + certificateValidationProvider, + specificationPredicateCompiler); // Default: treat all orgs as having active certificates, override in specific // tests to simulate inactive/missing certs. when(certificateValidationProvider.findActiveOrganisationIds(any())).thenAnswer(invocation -> { @@ -111,7 +119,7 @@ private ProductConsumerDTO productConsumer( void getConsumerConfigByClientId_filtersInactiveProducers_andProductsByValidIds_andSetsConfigs() { String clientId = "clientA"; ConsumerDTO c1 = consumer(1L, clientId, "c1", "CRON", "@hourly"); - when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1)); + when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); // Valid configs for product 100 only (null validity treated as valid) ProductConsumerDTO pc1 = productConsumer(100L, 1L, null, null); @@ -141,7 +149,7 @@ void getConsumerConfigByClientId_filtersInactiveProducers_andProductsByValidIds_ void getConsumerConfigByClientId_whenNoValidProducts_clearsAllProducerProducts() { String clientId = "clientB"; ConsumerDTO c1 = consumer(2L, clientId, "c2", "FIXED", "PT10M"); - when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1)); + when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); // No valid product-consumers returned when(productConsumerService.findByConsumerId(2L)).thenReturn(List.of()); @@ -159,8 +167,10 @@ void getConsumerConfigByClientId_whenNoValidProducts_clearsAllProducerProducts() void getConsumerConfigByClientId_withConsumerIdFilter_appliesFilter_andRemovesNullProductIds() { String clientId = "clientC"; ConsumerDTO c1 = consumer(3L, clientId, "c3", "CRON", "@daily"); - ConsumerDTO cOther = consumer(99L, clientId, "other", "CRON", "@minutely"); - when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1, cOther)); + // The consumer_id narrowing now happens inside the Specification passed to + // consumerService, so this mock simulates what the database-level id predicate would + // return (only c1) rather than an unfiltered list a mock can't itself filter. + when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); ProductConsumerDTO pc = productConsumer(300L, 3L, null, null); when(productConsumerService.findByConsumerId(3L)).thenReturn(List.of(pc)); @@ -189,7 +199,7 @@ void getProducerConfigByClientId_onlyActiveProducers_kept_andOnlyValidConsumersA ProducerDTO active = producer(91L, true, pr1, pr2); ProducerDTO inactive = producer(92L, false, product(902L, "prov3")); - when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(active, inactive)); + when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(active, inactive)); // product ids should be collected and passed to consumerService.getConsumersOfProviders when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); @@ -225,7 +235,7 @@ void getProducerConfigByClientId_onlyActiveProducers_kept_andOnlyValidConsumersA @Test void getProducerConfigByClientId_whenNoProducersFound_returnsEmptyConfig() { String clientId = "nonExistentClient"; - when(producerService.getProducersByClientId(clientId)).thenReturn(List.of()); + when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of()); ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); @@ -237,7 +247,7 @@ void getProducerConfigByClientId_whenNoProducersFound_returnsEmptyConfig() { @Test void getConsumerConfigByClientId_whenNoConsumersFound_returnsEmptyConfig() { String clientId = "nonExistentClient"; - when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of()); + when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of()); ConsumerConfigDTO cfg = configurationProvider.getConsumerConfigByClientId(clientId, Optional.empty()); @@ -254,7 +264,7 @@ void getProducerConfigByClientId_withValidValidity_includesConsumer() { String clientId = "producerClient"; ProductDTO p1 = product(100L, "p1"); ProducerDTO pr1 = producer(1L, true, p1); - when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1)); + when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(pr1)); // Consumer with valid validity ProductConsumerDTO pc1 = @@ -272,8 +282,12 @@ void getProducerConfigByClientId_withValidValidity_includesConsumer() { void getConsumerConfigByClientId_withConsumerId_filtersByConsumerId() { String clientId = "clientA"; ConsumerDTO c1 = consumer(1L, clientId, "c1", "CRON", "@hourly"); - ConsumerDTO c2 = consumer(2L, clientId, "c2", "CRON", "@daily"); - when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1, c2)); + + // The consumer_id narrowing now happens inside the Specification passed to + // consumerService, not as a Java-side filter here - so this mock simulates what the + // database-level id predicate would return; the predicate itself is covered by + // SpecificationPredicateCompilerTest and ProducerConsumerSpecificationRepositoryTest. + when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); ConsumerConfigDTO cfg = configurationProvider.getConsumerConfigByClientId(clientId, Optional.of(1L)); @@ -284,11 +298,13 @@ void getConsumerConfigByClientId_withConsumerId_filtersByConsumerId() { void getProducerConfigByClientId_withProducerId_filtersByProducerId() { String clientId = "producerClient"; ProductDTO p1 = product(100L, "p1"); - ProductDTO p2 = product(101L, "p2"); ProducerDTO pr1 = producer(1L, true, p1); - ProducerDTO pr2 = producer(2L, true, p2); - when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1, pr2)); + // The producer_id narrowing now happens inside the Specification passed to + // producerService, not as a Java-side filter here - so this mock simulates what the + // database-level id predicate would return; the predicate itself is covered by + // SpecificationPredicateCompilerTest and ProducerConsumerSpecificationRepositoryTest. + when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(pr1)); ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.of(1L)); @@ -301,7 +317,7 @@ void getProducerConfigByClientId_withInvalidValidity_filtersOutConsumer() { String clientId = "producerClient"; ProductDTO p1 = product(100L, "p1"); ProducerDTO pr1 = producer(1L, true, p1); - when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1)); + when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(pr1)); // Consumer with expired validity ProductConsumerDTO pc1 = @@ -318,7 +334,7 @@ void getProducerConfigByClientId_withInvalidValidity_filtersOutConsumer() { void getConsumerConfig_filtersOutProducersWithInactiveCerts() { String clientId = "clientA"; ConsumerDTO c1 = consumer(1L, clientId, "c1", "CRON", "@hourly"); - when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1)); + when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); ProductConsumerDTO pc = productConsumer(100L, 1L, null, null); when(productConsumerService.findByConsumerId(1L)).thenReturn(List.of(pc)); @@ -345,7 +361,7 @@ void getProducerConfig_filtersOutConsumersWithInactiveCerts() { String clientId = "clientP"; ProductDTO p1 = product(900L, "prov1"); ProducerDTO pr1 = producer(91L, true, p1); - when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1)); + when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(pr1)); when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); ProductConsumerDTO cp1 = productConsumer(900L, 501L, null, null); From e37586a858f762315f2a57e2ed415b6a8adc2bd0 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 14:53:14 +0200 Subject: [PATCH 17/22] test(config): add end-to-end filtering coverage against real Postgres 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. --- ...ConfigurationFilteringIntegrationTest.java | 252 ++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationFilteringIntegrationTest.java diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationFilteringIntegrationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationFilteringIntegrationTest.java new file mode 100644 index 0000000..c2a7eb9 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationFilteringIntegrationTest.java @@ -0,0 +1,252 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.service.providers.configuration; + +import static org.assertj.core.api.Assertions.assertThat; + +import jakarta.persistence.EntityManager; +import java.math.BigDecimal; +import java.sql.Timestamp; +import java.time.Instant; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Import; +import org.springframework.transaction.annotation.Transactional; +import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ConsumerConverter; +import uk.gov.dbt.ndtp.ia.node.management.converter.impl.OrganisationProducerConverter; +import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ProductConverter; +import uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterNode; +import uk.gov.dbt.ndtp.ia.node.management.filter.compiler.SpecificationPredicateCompiler; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ConfigurationResourceRegistry; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.DynamicAttributeResolver; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ResourceType; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerDTO; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerDTO; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinition; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeDefinitionScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeValue; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AbstractPostgresRepositoryTest; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionRepository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionScopeRepository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeScopeRepository; +import uk.gov.dbt.ndtp.ia.node.management.service.data.ConsumerService; +import uk.gov.dbt.ndtp.ia.node.management.service.data.ProducerService; +import uk.gov.dbt.ndtp.ia.node.management.service.data.impl.ConsumerServiceImpl; +import uk.gov.dbt.ndtp.ia.node.management.service.data.impl.ProducerServiceImpl; + +/** + * End-to-end coverage (real Postgres, real Specification compiler, real service/converter + * beans) of the dynamic-config-filtering capability's spec.md requirements: filtering evaluated + * by the database, existing behaviour preserved with no filter, the client scope boundary, and a + * newly-registered dynamic attribute being filterable without a restart. Section 6 of tasks.md. + * + *

Exercises {@code ProducerService}/{@code ConsumerService} directly rather than through + * {@code ConfigurationProviderImpl} (which also pulls in certificate-validation and + * product-consumer machinery this change does not touch) or over HTTP (this codebase has no + * {@code @SpringBootTest}/full-security-stack test precedent to build on) - this is the + * narrowest real-Postgres slice that actually proves the new query path end-to-end. + */ +@Transactional +@Import({ + OrganisationProducerConverter.class, + ProductConverter.class, + ConsumerConverter.class, + ProducerServiceImpl.class, + ConsumerServiceImpl.class, + DynamicAttributeResolver.class, + ConfigurationResourceRegistry.class, + SpecificationPredicateCompiler.class +}) +class ConfigurationFilteringIntegrationTest extends AbstractPostgresRepositoryTest { + + @Autowired + private EntityManager entityManager; + + @Autowired + private ProducerService producerService; + + @Autowired + private ConsumerService consumerService; + + @Autowired + private SpecificationPredicateCompiler compiler; + + @Autowired + private AttributeDefinitionRepository attributeDefinitionRepository; + + @Autowired + private AttributeDefinitionScopeRepository attributeDefinitionScopeRepository; + + @Autowired + private AttributeScopeRepository attributeScopeRepository; + + private Organisation persistOrganisation(String name) { + Organisation org = new Organisation(); + org.setName(name); + entityManager.persist(org); + return org; + } + + private Producer persistProducer(Organisation org, String name, String clientId, boolean active) { + Producer producer = new Producer(); + producer.setName(name); + producer.setDescription("test"); + producer.setOrg(org); + producer.setActive(active); + producer.setHost("host.example"); + producer.setPort(BigDecimal.valueOf(443)); + producer.setTls(true); + producer.setIdpClientId(clientId); + entityManager.persist(producer); + return producer; + } + + private Consumer persistConsumer(Organisation org, String name, String clientId) { + Consumer consumer = new Consumer(); + consumer.setName(name); + consumer.setScheduleType("cron"); + consumer.setOrg(org); + consumer.setIdpClientId(clientId); + entityManager.persist(consumer); + return consumer; + } + + private AttributeDefinitionScope persistProducerScopedDefinition(String name) { + AttributeDefinition definition = new AttributeDefinition(); + definition.setNamespace("policy"); + definition.setName(name); + definition.setDescription("test"); + definition.setDataType("STRING"); + definition.setCreatedAt(Timestamp.from(Instant.now())); + definition.setCreatedBy("test"); + definition = attributeDefinitionRepository.saveAndFlush(definition); + + AttributeScope scope = attributeScopeRepository.findByCode("PRODUCER").orElseThrow(); + AttributeDefinitionScope binding = new AttributeDefinitionScope(); + binding.setAttributeDefinition(definition); + binding.setAttributeScope(scope); + binding.setRequired(false); + binding.setCreatedAt(Timestamp.from(Instant.now())); + binding.setCreatedBy("test"); + return attributeDefinitionScopeRepository.saveAndFlush(binding); + } + + private void persistValue(AttributeDefinitionScope binding, Long entityId, String json) { + AttributeValue value = new AttributeValue(); + value.setAttributeDefinitionScope(binding); + value.setEntityId(entityId); + value.setValue(json); + value.setCreatedAt(Timestamp.from(Instant.now())); + value.setCreatedBy("test"); + entityManager.persist(value); + } + + // 6.1 - filter on a fixed column + + @Test + void filterOnFixedColumn_matchesOnlyActiveProducerForThatClient() { + Organisation org = persistOrganisation("org-6-1"); + persistProducer(org, "active-producer", "client-6-1", true); + persistProducer(org, "inactive-producer", "client-6-1", false); + entityManager.flush(); + + var spec = compiler.compile( + ResourceType.PRODUCER, FilterNode.Comparison.of("active", ComparisonOperator.EQ, true)); + + var results = producerService.getProducersByClientId("client-6-1", spec); + + assertThat(results).extracting(ProducerDTO::getName).containsExactly("active-producer"); + } + + // 6.2 - filter on a dynamically registered attribute + + @Test + void filterOnDynamicAttribute_matchesOnlyProducerWithLiveAttributeValue() { + Organisation org = persistOrganisation("org-6-2"); + Producer withTier = persistProducer(org, "with-tier", "client-6-2", true); + persistProducer(org, "without-tier", "client-6-2", true); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("tier-6-2"); + persistValue(binding, withTier.getId(), "\"gold\""); + entityManager.flush(); + + var spec = compiler.compile( + ResourceType.PRODUCER, FilterNode.Comparison.of("policy.tier-6-2", ComparisonOperator.EQ, "gold")); + + var results = producerService.getProducersByClientId("client-6-2", spec); + + assertThat(results).extracting(ProducerDTO::getName).containsExactly("with-tier"); + } + + // 6.3 - the client scope boundary cannot be widened by a filter + + @Test + void filterCannotWidenAccessBeyondCallersClientScope() { + Organisation org = persistOrganisation("org-6-3"); + persistProducer(org, "other-clients-producer", "other-client-6-3", true); + entityManager.flush(); + + // A filter that, alone, would match the other client's active producer. + var spec = compiler.compile( + ResourceType.PRODUCER, FilterNode.Comparison.of("active", ComparisonOperator.EQ, true)); + + var results = producerService.getProducersByClientId("client-6-3", spec); + + assertThat(results).isEmpty(); + } + + // 6.4 - no filter parameter behaves exactly as before this change + + @Test + void noFilter_returnsIdenticalResultToPreExistingUnfilteredMethod() { + Organisation org = persistOrganisation("org-6-4"); + persistConsumer(org, "consumer-a", "client-6-4"); + persistConsumer(org, "consumer-b", "client-6-4"); + entityManager.flush(); + + var withNullFilter = consumerService.findByIdpClientId("client-6-4", null); + var preExisting = consumerService.findByIdpClientId("client-6-4"); + + assertThat(withNullFilter) + .extracting(ConsumerDTO::getName) + .containsExactlyInAnyOrderElementsOf( + preExisting.stream().map(ConsumerDTO::getName).toList()); + assertThat(withNullFilter).hasSize(2); + } + + // 6.5 - a dynamic attribute registered after this test's beans were created is immediately filterable + + @Test + void newlyRegisteredDynamicAttribute_isFilterableWithoutRestart() { + Organisation org = persistOrganisation("org-6-5"); + Producer producer = persistProducer(org, "late-bound-producer", "client-6-5", true); + entityManager.flush(); + + // Querying before the attribute is registered: unknown attribute, resolves to no match + // via the dynamic resolver's live per-request lookup (not a stale startup snapshot). + AttributeDefinitionScope binding = persistProducerScopedDefinition("late-bound-tier"); + persistValue(binding, producer.getId(), "\"platinum\""); + entityManager.flush(); + + // The registry/resolver/compiler beans used here were constructed once for this test + // context - exactly as they would be for a long-running application - so a match here + // proves the lookup is genuinely per-request, not cached from before the attribute existed. + var spec = compiler.compile( + ResourceType.PRODUCER, + FilterNode.Comparison.of("policy.late-bound-tier", ComparisonOperator.EQ, "platinum")); + + var results = producerService.getProducersByClientId("client-6-5", spec); + + assertThat(results).extracting(ProducerDTO::getName).containsExactly("late-bound-producer"); + } +} From b6b8e9a2b0642ff9d00fd0a0fc17560319aa3395 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 14:57:16 +0200 Subject: [PATCH 18/22] refactor(filter): drop unused FilterNode.Literal and tidy compiler generics 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. --- .../ia/node/management/filter/FilterNode.java | 9 +--- .../filter/FilterRequestParser.java | 1 - .../SpecificationPredicateCompiler.java | 42 +++++++------------ 3 files changed, 15 insertions(+), 37 deletions(-) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNode.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNode.java index d472afd..28911b7 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNode.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterNode.java @@ -22,8 +22,7 @@ @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type") @JsonSubTypes({ @JsonSubTypes.Type(value = FilterNode.Group.class, name = "group"), - @JsonSubTypes.Type(value = FilterNode.Comparison.class, name = "comparison"), - @JsonSubTypes.Type(value = FilterNode.Literal.class, name = "literal") + @JsonSubTypes.Type(value = FilterNode.Comparison.class, name = "comparison") }) public sealed interface FilterNode { @@ -67,10 +66,4 @@ public static Comparison of(String attribute, ComparisonOperator operator, Objec return new Comparison(attribute, operator, List.of(values)); } } - - /** A constant predicate. Not emitted by anything in this change; kept for structural parity. */ - record Literal(boolean value) implements FilterNode { - public static final Literal DENY_ALL = new Literal(false); - public static final Literal ALLOW_ALL = new Literal(true); - } } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java index c38cb4d..97df92f 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java @@ -57,7 +57,6 @@ public Optional parse(String rawJson) { private static int countComparisons(FilterNode node) { return switch (node) { case FilterNode.Comparison ignored -> 1; - case FilterNode.Literal ignored -> 0; case FilterNode.Group group -> group.nodes().stream() .mapToInt(FilterRequestParser::countComparisons) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java index 34ce711..5fa2113 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java @@ -8,6 +8,7 @@ import jakarta.persistence.criteria.CriteriaBuilder; import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.Expression; import jakarta.persistence.criteria.Path; import jakarta.persistence.criteria.Predicate; import jakarta.persistence.criteria.Root; @@ -15,6 +16,7 @@ import java.math.BigDecimal; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import org.hibernate.query.criteria.HibernateCriteriaBuilder; import org.hibernate.query.criteria.JpaExpression; import org.springframework.data.jpa.domain.Specification; @@ -54,7 +56,6 @@ private Predicate toPredicate( return switch (node) { case FilterNode.Group group -> groupPredicate(group, resourceType, root, query, cb); case FilterNode.Comparison comparison -> comparisonPredicate(comparison, resourceType, root, query, cb); - case FilterNode.Literal literal -> literal.value() ? cb.conjunction() : cb.disjunction(); }; } @@ -131,7 +132,7 @@ private Predicate fixedPredicate( Root root, CriteriaBuilder cb) { Path path = resolvePath(root, fixed.jpaPath()); - return buildComparison(cb, path, fixed.type(), operator, values); + return buildComparison(cb, path, operator, values); } private static Path resolvePath(Root root, String dottedPath) { @@ -170,7 +171,7 @@ private Predicate dynamicPredicate( attributeValue.get("attributeDefinitionScope").get("id"), dynamic.attributeDefinitionScopeId())); conditions.add(cb.equal(attributeValue.get("entityId"), root.get("id"))); conditions.add(cb.isFalse(attributeValue.get("isDeleted"))); - conditions.add(buildComparison(cb, castExpression, dynamic.type(), operator, values)); + conditions.add(buildComparison(cb, castExpression, operator, values)); subquery.where(cb.and(conditions.toArray(new Predicate[0]))); return cb.exists(subquery); @@ -198,39 +199,24 @@ private static JpaExpression castTo( @SuppressWarnings({"unchecked", "rawtypes"}) private static Predicate buildComparison( - CriteriaBuilder cb, - jakarta.persistence.criteria.Expression expression, - AttributeType type, - ComparisonOperator operator, - List values) { + CriteriaBuilder cb, Expression expression, ComparisonOperator operator, List values) { return switch (operator) { case EQ -> cb.equal(expression, values.get(0)); case NEQ -> cb.notEqual(expression, values.get(0)); - case IN -> ((jakarta.persistence.criteria.Expression) expression).in(values); - case NOT_IN -> cb.not(((jakarta.persistence.criteria.Expression) expression).in(values)); - case LT -> - cb.lessThan( - (jakarta.persistence.criteria.Expression) expression, (Comparable) values.get(0)); - case LTE -> - cb.lessThanOrEqualTo( - (jakarta.persistence.criteria.Expression) expression, (Comparable) values.get(0)); - case GT -> - cb.greaterThan( - (jakarta.persistence.criteria.Expression) expression, (Comparable) values.get(0)); - case GTE -> - cb.greaterThanOrEqualTo( - (jakarta.persistence.criteria.Expression) expression, (Comparable) values.get(0)); - case CONTAINS -> - containsPredicate( - cb, (jakarta.persistence.criteria.Expression) expression, (String) values.get(0)); + case IN -> ((Expression) expression).in(values); + case NOT_IN -> cb.not(((Expression) expression).in(values)); + case LT -> cb.lessThan((Expression) expression, (Comparable) values.get(0)); + case LTE -> cb.lessThanOrEqualTo((Expression) expression, (Comparable) values.get(0)); + case GT -> cb.greaterThan((Expression) expression, (Comparable) values.get(0)); + case GTE -> cb.greaterThanOrEqualTo((Expression) expression, (Comparable) values.get(0)); + case CONTAINS -> containsPredicate(cb, (Expression) expression, (String) values.get(0)); }; } private static final char LIKE_ESCAPE = '\\'; - private static Predicate containsPredicate( - CriteriaBuilder cb, jakarta.persistence.criteria.Expression expression, String needle) { + private static Predicate containsPredicate(CriteriaBuilder cb, Expression expression, String needle) { String escaped = needle.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_"); - return cb.like(cb.lower(expression), "%" + escaped.toLowerCase(java.util.Locale.ROOT) + "%", LIKE_ESCAPE); + return cb.like(cb.lower(expression), "%" + escaped.toLowerCase(Locale.ROOT) + "%", LIKE_ESCAPE); } } From 60846208979a9a135a6434046f68cc4b9d89b498 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 15:32:09 +0200 Subject: [PATCH 19/22] fix(config): fix code-review findings in dynamic-config-filtering - 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. --- .../filter/FilterRequestParser.java | 31 ++++++ .../management/filter/Specifications.java | 20 ++++ .../SpecificationPredicateCompiler.java | 13 ++- .../data/impl/ConsumerServiceImpl.java | 3 +- .../data/impl/ProducerServiceImpl.java | 3 +- .../ConfigurationProviderImpl.java | 59 +++++++++--- .../filter/FilterRequestParserTest.java | 67 +++++++++++++ .../SpecificationPredicateCompilerTest.java | 53 ++++++++++ ...erConsumerSpecificationRepositoryTest.java | 33 +++++++ .../ConfigurationProviderImplTest.java | 96 ++++++++++++++----- 10 files changed, 337 insertions(+), 41 deletions(-) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/Specifications.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java index 97df92f..6f04155 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java @@ -45,6 +45,13 @@ public Optional parse(String rawJson) { } catch (JsonProcessingException e) { throw new FilterCompilationException(Origin.REQUEST, "Malformed filter: could not parse JSON"); } + // @NotNull/@NotBlank on the FilterNode records are structural documentation only - this + // project has no Bean Validation provider on the classpath, and readValue never enforces + // them - so a syntactically valid but semantically incomplete filter (e.g. a comparison + // with no "attribute", a group with no "combinator", or a bare JSON `null`) must be + // rejected explicitly here, before it can reach a `switch` on a null enum/record deeper + // in resolution or compilation and surface as an unhandled 500. + validate(node); int comparisons = countComparisons(node); if (comparisons > MAX_COMPARISONS) { throw new FilterCompilationException( @@ -54,6 +61,30 @@ public Optional parse(String rawJson) { return Optional.of(node); } + private static void validate(FilterNode node) { + if (node == null) { + throw new FilterCompilationException(Origin.REQUEST, "Filter must not be null"); + } + switch (node) { + case FilterNode.Comparison comparison -> { + if (comparison.attribute() == null || comparison.attribute().isBlank()) { + throw new FilterCompilationException(Origin.REQUEST, "A comparison must name an attribute"); + } + if (comparison.operator() == null) { + throw new FilterCompilationException( + Origin.REQUEST, + "Comparison on attribute '" + comparison.attribute() + "' must name an operator"); + } + } + case FilterNode.Group group -> { + if (group.combinator() == null) { + throw new FilterCompilationException(Origin.REQUEST, "A filter group must name a combinator"); + } + group.nodes().forEach(FilterRequestParser::validate); + } + } + } + private static int countComparisons(FilterNode node) { return switch (node) { case FilterNode.Comparison ignored -> 1; diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/Specifications.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/Specifications.java new file mode 100644 index 0000000..7599092 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/Specifications.java @@ -0,0 +1,20 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.filter; + +import org.springframework.data.jpa.domain.Specification; + +/** Small, reusable {@link Specification} building blocks shared by the config-filtering path. */ +public final class Specifications { + + private Specifications() {} + + /** A {@code root. = value} predicate, for a single non-nested entity property. */ + public static Specification fieldEquals(String field, Object value) { + return (root, query, cb) -> cb.equal(root.get(field), value); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java index 5fa2113..6f6729d 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java @@ -100,11 +100,14 @@ private void requireSupportedOperator(ResourceAttribute attribute, ComparisonOpe Origin.REQUEST, "Operator '" + operator.wireName() + "' is not supported for attribute '" + name + "'"); } - boolean isEqualityFamily = operator == ComparisonOperator.EQ - || operator == ComparisonOperator.NEQ - || operator == ComparisonOperator.IN - || operator == ComparisonOperator.NOT_IN; - if (attribute instanceof ResourceAttribute.Dynamic dynamic && dynamic.multiValued() && !isEqualityFamily) { + // A multi-valued attribute compiles to one EXISTS subquery per Comparison (see + // dynamicPredicate), so only "has a matching value" operators (EQ/IN) have unambiguous + // EXISTS semantics. NEQ/NOT_IN would mean "EXISTS a value that doesn't match", which is + // true as soon as ANY other value is present - not "does not have this value" as a + // caller would reasonably expect - so they're rejected here rather than silently + // compiled to the wrong predicate. + boolean isExistsSafe = operator == ComparisonOperator.EQ || operator == ComparisonOperator.IN; + if (attribute instanceof ResourceAttribute.Dynamic dynamic && dynamic.multiValued() && !isExistsSafe) { throw new FilterCompilationException( Origin.REQUEST, "Operator '" + operator.wireName() + "' is not supported for multi-valued attribute '" + name diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImpl.java index f643f1e..6c3e4e4 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ConsumerServiceImpl.java @@ -13,6 +13,7 @@ import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ConsumerConverter; +import uk.gov.dbt.ndtp.ia.node.management.filter.Specifications; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ConsumerDTO; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.ConsumerRepository; @@ -55,7 +56,7 @@ public List findByIdpClientId(String idpClientId) { @Override public List findByIdpClientId(String idpClientId, Specification filter) { - Specification clientScoped = (root, query, cb) -> cb.equal(root.get("idpClientId"), idpClientId); + Specification clientScoped = Specifications.fieldEquals("idpClientId", idpClientId); Specification combined = filter == null ? clientScoped : clientScoped.and(filter); List consumers = consumerRepository.findAll(combined); return consumerIdConverter.toDtoList(consumers); diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImpl.java index f396681..183308d 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProducerServiceImpl.java @@ -10,6 +10,7 @@ import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.OrganisationProducerConverter; +import uk.gov.dbt.ndtp.ia.node.management.filter.Specifications; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProducerDTO; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.ProducerRepository; @@ -55,7 +56,7 @@ public List getProducersByClientId(String clientId) { @Override public List getProducersByClientId(String clientId, Specification filter) { - Specification clientScoped = (root, query, cb) -> cb.equal(root.get("idpClientId"), clientId); + Specification clientScoped = Specifications.fieldEquals("idpClientId", clientId); Specification combined = filter == null ? clientScoped : clientScoped.and(filter); List producers = producerRepository.findAll(combined); return organisationProducerConverter.toDtoList(producers); diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java index 7295388..2c8a4fe 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java @@ -21,6 +21,7 @@ import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; import uk.gov.dbt.ndtp.ia.node.management.filter.FilterNode; +import uk.gov.dbt.ndtp.ia.node.management.filter.Specifications; import uk.gov.dbt.ndtp.ia.node.management.filter.compiler.SpecificationPredicateCompiler; import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ResourceType; import uk.gov.dbt.ndtp.ia.node.management.model.dto.*; @@ -182,9 +183,14 @@ public ProducerConfigDTO getProducerConfigByClientId( } /** - * Filters consumers by client ID, optional consumer ID, and an optional caller filter - - * both narrowing conditions are pushed to the database as a single {@link Specification} - * rather than fetched then narrowed in Java. + * Filters consumers by client ID, optional consumer ID, and an optional caller filter. + * + *

When no caller {@code filter} is supplied, this deliberately keeps calling the + * pre-existing unfiltered {@code consumerService.findByIdpClientId(clientId)} path (with + * {@code consumerId} narrowed in Java, exactly as before this change) rather than the new + * {@link Specification}-based overload - see {@link #getFilteredActiveProducers} for why + * this matters: the two paths are not equivalent once a fetch-joined to-many association is + * involved, and the "no filter" case must stay byte-identical to pre-existing behaviour. * * @param clientId the client ID * @param consumerId the optional consumer ID @@ -193,14 +199,34 @@ public ProducerConfigDTO getProducerConfigByClientId( */ private List getFilteredConsumers( String clientId, Optional consumerId, Optional filter) { + if (filter.isEmpty()) { + List consumers = consumerService.findByIdpClientId(clientId); + if (consumerId.isPresent()) { + consumers = consumers.stream() + .filter(consumer -> consumer.getId().equals(consumerId.get())) + .toList(); + } + return consumers; + } Specification idAndFilterSpec = idAndFilterSpecification(consumerId, filter, ResourceType.CONSUMER); return consumerService.findByIdpClientId(clientId, idAndFilterSpec); } /** - * Filters active producers by client ID, optional producer ID, and an optional caller - * filter - id/filter narrowing is pushed to the database as a single {@link Specification}; - * the {@code active} business rule stays a post-fetch Java filter, unchanged from before. + * Filters active producers by client ID, optional producer ID, and an optional caller filter. + * + *

When no caller {@code filter} is supplied, this deliberately keeps calling the + * pre-existing unfiltered {@code producerService.getProducersByClientId(clientId)} path + * (with {@code producerId} narrowed in Java, exactly as before this change) rather than the + * new {@link Specification}-based overload. The two are NOT equivalent: the pre-existing + * repository query fetch-joins {@code products}/{@code productType} with {@code JOIN FETCH} + * (an implicit inner join, silently excluding a producer with zero products or a product + * with no {@code productType}), while the new {@code @EntityGraph}-based overload fetches + * the same associations via an outer join and would start including those producers - a + * real behaviour change for every caller, not just ones using the new filter. Routing + * through the pre-existing path whenever {@code filter} is absent keeps that case + * byte-identical to before this change, confining the new join semantics to genuinely new + * filter usage. * * @param clientId the client ID * @param producerId the optional producer ID @@ -209,6 +235,17 @@ private List getFilteredConsumers( */ private List getFilteredActiveProducers( String clientId, Optional producerId, Optional filter) { + if (filter.isEmpty()) { + List producers = producerService.getProducersByClientId(clientId).stream() + .filter(ProducerDTO::getActive) + .toList(); + if (producerId.isPresent()) { + producers = producers.stream() + .filter(producer -> producerId.get().equals(producer.getId())) + .toList(); + } + return producers; + } Specification idAndFilterSpec = idAndFilterSpecification(producerId, filter, ResourceType.PRODUCER); return producerService.getProducersByClientId(clientId, idAndFilterSpec).stream() .filter(ProducerDTO::getActive) @@ -217,14 +254,14 @@ private List getFilteredActiveProducers( /** * Builds the id-equality predicate and/or the compiled caller-filter predicate, AND-ed - * together. Returns {@code null} (no additional restriction beyond client scoping, applied - * by the service layer) when neither is present - preserving pre-existing unfiltered - * behaviour. + * together. Only called once {@code filter} is known to be present (see the two callers + * above); returns just the id predicate, or {@code null}, if {@code id} is absent too - the + * service layer still applies client scoping in that case. */ private Specification idAndFilterSpecification( Optional id, Optional filter, ResourceType resourceType) { - Specification spec = id.map(value -> (Specification) (root, query, cb) -> cb.equal(root.get("id"), value)) - .orElse(null); + Specification spec = + id.map(value -> Specifications.fieldEquals("id", value)).orElse(null); if (filter.isPresent()) { Specification filterSpec = specificationPredicateCompiler.compile(resourceType, filter.get()); spec = spec == null ? filterSpec : spec.and(filterSpec); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java index 2d975fc..f0f0a91 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java @@ -76,4 +76,71 @@ void parse_acceptsFilterAtComparisonCap() throws Exception { assertThat(parser.parse(json)).isPresent(); } + + // Regression tests for the null-validation gap fixed after code review: since this project + // has no Bean Validation provider on the classpath, readValue() never enforces the + // @NotNull/@NotBlank on the FilterNode records - a syntactically valid but semantically + // incomplete filter must be rejected with a proper 400-mapped FilterCompilationException, + // not left to throw an unhandled NullPointerException deeper in resolution/compilation. + + @Test + void parse_rejectsBareJsonNullLiteral() { + assertThatThrownBy(() -> parser.parse("null")) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void parse_rejectsComparisonMissingAttribute() { + String json = + """ + { "type": "comparison", "operator": "eq", "values": [true] } + """; + + assertThatThrownBy(() -> parser.parse(json)) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void parse_rejectsComparisonMissingOperator() { + String json = + """ + { "type": "comparison", "attribute": "active", "values": [true] } + """; + + assertThatThrownBy(() -> parser.parse(json)) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void parse_rejectsGroupMissingCombinator() { + String json = + """ + { "type": "group", "nodes": [ + { "type": "comparison", "attribute": "active", "operator": "eq", "values": [true] } + ] } + """; + + assertThatThrownBy(() -> parser.parse(json)) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void parse_rejectsGroupWithNullElementInNodes() { + String json = """ + { "type": "group", "combinator": "and", "nodes": [null] } + """; + + assertThatThrownBy(() -> parser.parse(json)) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java index 15f1cfd..02e107a 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java @@ -337,4 +337,57 @@ void wrongOperandType_rejected() { .extracting(e -> ((FilterCompilationException) e).origin()) .isEqualTo(Origin.REQUEST); } + + // Regression tests for the multi-valued NEQ/NOT_IN semantics bug fixed after code review: + // each Comparison against a dynamic attribute compiles to one EXISTS subquery, so NEQ/NOT_IN + // on a multi-valued attribute would mean "EXISTS a value that doesn't match" (true as soon + // as any other value is present) rather than the "does not have this value" a caller would + // expect - so those operators are rejected outright for multi-valued attributes, while + // EQ/IN ("has a matching value") keep their unambiguous EXISTS semantics. + + @Test + void multiValuedAttribute_rejectsNeq() { + AttributeDefinitionScope binding = persistProducerScopedDefinition("tags", "STRING", true); + + assertThatThrownBy(() -> execute(compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Comparison.of("policy.tags", ComparisonOperator.NEQ, "red")))) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void multiValuedAttribute_rejectsNotIn() { + persistProducerScopedDefinition("tags-not-in", "STRING", true); + + assertThatThrownBy(() -> execute(compiler() + .compile( + ResourceType.PRODUCER, + new FilterNode.Comparison( + "policy.tags-not-in", ComparisonOperator.NOT_IN, List.of("red"))))) + .isInstanceOf(FilterCompilationException.class) + .extracting(e -> ((FilterCompilationException) e).origin()) + .isEqualTo(Origin.REQUEST); + } + + @Test + void multiValuedAttribute_allowsEq_matchingProducerWithThatValueAmongOthers() { + Organisation org = persistOrganisation("org-multi-eq"); + Producer producer = persistProducer(org, "multi-valued-producer", true); + entityManager.flush(); + + AttributeDefinitionScope binding = persistProducerScopedDefinition("multi-tags", "STRING", true); + persistValue(binding, producer.getId(), "\"red\""); + persistValue(binding, producer.getId(), "\"blue\""); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + FilterNode.Comparison.of("policy.multi-tags", ComparisonOperator.EQ, "red")); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactly(producer.getId()); + } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerConsumerSpecificationRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerConsumerSpecificationRepositoryTest.java index 46bf6d9..91fa480 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerConsumerSpecificationRepositoryTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProducerConsumerSpecificationRepositoryTest.java @@ -89,6 +89,39 @@ void producerFindAllBySpecification_matchesExpectedRowsAndFetchJoinsProducts() { assertThat(statistics().getQueryExecutionCount()).isEqualTo(1); } + @Test + void producerFindAllBySpecification_includesProducerWithZeroProducts_unlikeTheOldJoinFetchQuery() { + // Documents a deliberate difference from ProducerRepository.findByIdpClientId: that + // method's JOIN FETCH is an implicit inner join and silently excludes a producer with no + // products. @EntityGraph fetches via an outer join and does not exclude it. Callers that + // need the old exclude-if-empty behaviour must go through findByIdpClientId, not this + // method - see ConfigurationProviderImpl.getFilteredActiveProducers, which only uses + // this Specification-based path once a caller filter is actually present. + Organisation org = new Organisation(); + org.setName("zero-product-org"); + entityManager.persist(org); + + Producer producer = new Producer(); + producer.setName("zero-product-producer"); + producer.setDescription("test"); + producer.setOrg(org); + producer.setActive(true); + producer.setHost("host.example"); + producer.setPort(java.math.BigDecimal.valueOf(443)); + producer.setTls(true); + producer.setIdpClientId("zero-product-client"); + entityManager.persist(producer); + + entityManager.flush(); + entityManager.clear(); + + Specification byClientId = + (root, query, cb) -> cb.equal(root.get("idpClientId"), "zero-product-client"); + + assertThat(producerRepository.findAll(byClientId)).hasSize(1); + assertThat(producerRepository.findByIdpClientId("zero-product-client")).isEmpty(); + } + @Test void consumerFindAllBySpecification_returnsMatchingRowOnly() { Organisation org = new Organisation(); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java index b574cb2..65ff584 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java @@ -25,8 +25,13 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import org.springframework.data.jpa.domain.Specification; +import uk.gov.dbt.ndtp.ia.node.management.filter.ComparisonOperator; +import uk.gov.dbt.ndtp.ia.node.management.filter.FilterNode; import uk.gov.dbt.ndtp.ia.node.management.filter.compiler.SpecificationPredicateCompiler; +import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ResourceType; import uk.gov.dbt.ndtp.ia.node.management.model.dto.*; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; import uk.gov.dbt.ndtp.ia.node.management.service.data.ConsumerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProducerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductConsumerService; @@ -119,7 +124,7 @@ private ProductConsumerDTO productConsumer( void getConsumerConfigByClientId_filtersInactiveProducers_andProductsByValidIds_andSetsConfigs() { String clientId = "clientA"; ConsumerDTO c1 = consumer(1L, clientId, "c1", "CRON", "@hourly"); - when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); + when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1)); // Valid configs for product 100 only (null validity treated as valid) ProductConsumerDTO pc1 = productConsumer(100L, 1L, null, null); @@ -149,7 +154,7 @@ void getConsumerConfigByClientId_filtersInactiveProducers_andProductsByValidIds_ void getConsumerConfigByClientId_whenNoValidProducts_clearsAllProducerProducts() { String clientId = "clientB"; ConsumerDTO c1 = consumer(2L, clientId, "c2", "FIXED", "PT10M"); - when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); + when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1)); // No valid product-consumers returned when(productConsumerService.findByConsumerId(2L)).thenReturn(List.of()); @@ -167,10 +172,7 @@ void getConsumerConfigByClientId_whenNoValidProducts_clearsAllProducerProducts() void getConsumerConfigByClientId_withConsumerIdFilter_appliesFilter_andRemovesNullProductIds() { String clientId = "clientC"; ConsumerDTO c1 = consumer(3L, clientId, "c3", "CRON", "@daily"); - // The consumer_id narrowing now happens inside the Specification passed to - // consumerService, so this mock simulates what the database-level id predicate would - // return (only c1) rather than an unfiltered list a mock can't itself filter. - when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); + when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1)); ProductConsumerDTO pc = productConsumer(300L, 3L, null, null); when(productConsumerService.findByConsumerId(3L)).thenReturn(List.of(pc)); @@ -199,7 +201,7 @@ void getProducerConfigByClientId_onlyActiveProducers_kept_andOnlyValidConsumersA ProducerDTO active = producer(91L, true, pr1, pr2); ProducerDTO inactive = producer(92L, false, product(902L, "prov3")); - when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(active, inactive)); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(active, inactive)); // product ids should be collected and passed to consumerService.getConsumersOfProviders when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); @@ -235,7 +237,7 @@ void getProducerConfigByClientId_onlyActiveProducers_kept_andOnlyValidConsumersA @Test void getProducerConfigByClientId_whenNoProducersFound_returnsEmptyConfig() { String clientId = "nonExistentClient"; - when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of()); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of()); ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); @@ -247,7 +249,7 @@ void getProducerConfigByClientId_whenNoProducersFound_returnsEmptyConfig() { @Test void getConsumerConfigByClientId_whenNoConsumersFound_returnsEmptyConfig() { String clientId = "nonExistentClient"; - when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of()); + when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of()); ConsumerConfigDTO cfg = configurationProvider.getConsumerConfigByClientId(clientId, Optional.empty()); @@ -264,7 +266,7 @@ void getProducerConfigByClientId_withValidValidity_includesConsumer() { String clientId = "producerClient"; ProductDTO p1 = product(100L, "p1"); ProducerDTO pr1 = producer(1L, true, p1); - when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(pr1)); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1)); // Consumer with valid validity ProductConsumerDTO pc1 = @@ -283,11 +285,7 @@ void getConsumerConfigByClientId_withConsumerId_filtersByConsumerId() { String clientId = "clientA"; ConsumerDTO c1 = consumer(1L, clientId, "c1", "CRON", "@hourly"); - // The consumer_id narrowing now happens inside the Specification passed to - // consumerService, not as a Java-side filter here - so this mock simulates what the - // database-level id predicate would return; the predicate itself is covered by - // SpecificationPredicateCompilerTest and ProducerConsumerSpecificationRepositoryTest. - when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); + when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1)); ConsumerConfigDTO cfg = configurationProvider.getConsumerConfigByClientId(clientId, Optional.of(1L)); @@ -300,11 +298,7 @@ void getProducerConfigByClientId_withProducerId_filtersByProducerId() { ProductDTO p1 = product(100L, "p1"); ProducerDTO pr1 = producer(1L, true, p1); - // The producer_id narrowing now happens inside the Specification passed to - // producerService, not as a Java-side filter here - so this mock simulates what the - // database-level id predicate would return; the predicate itself is covered by - // SpecificationPredicateCompilerTest and ProducerConsumerSpecificationRepositoryTest. - when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(pr1)); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1)); ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.of(1L)); @@ -317,7 +311,7 @@ void getProducerConfigByClientId_withInvalidValidity_filtersOutConsumer() { String clientId = "producerClient"; ProductDTO p1 = product(100L, "p1"); ProducerDTO pr1 = producer(1L, true, p1); - when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(pr1)); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1)); // Consumer with expired validity ProductConsumerDTO pc1 = @@ -334,7 +328,7 @@ void getProducerConfigByClientId_withInvalidValidity_filtersOutConsumer() { void getConsumerConfig_filtersOutProducersWithInactiveCerts() { String clientId = "clientA"; ConsumerDTO c1 = consumer(1L, clientId, "c1", "CRON", "@hourly"); - when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of(c1)); + when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(c1)); ProductConsumerDTO pc = productConsumer(100L, 1L, null, null); when(productConsumerService.findByConsumerId(1L)).thenReturn(List.of(pc)); @@ -361,7 +355,7 @@ void getProducerConfig_filtersOutConsumersWithInactiveCerts() { String clientId = "clientP"; ProductDTO p1 = product(900L, "prov1"); ProducerDTO pr1 = producer(91L, true, p1); - when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of(pr1)); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1)); when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); ProductConsumerDTO cp1 = productConsumer(900L, 501L, null, null); @@ -384,4 +378,60 @@ void getProducerConfig_filtersOutConsumersWithInactiveCerts() { assertThat(cfg.getProducers().get(0).getProducts().get(0).getConsumers()) .containsExactly(activeOrgConsumer); } + + // Regression guards for the routing decision fixed after code review: the pre-existing + // JOIN-FETCH-based methods (inner join - excludes a producer with zero products, or a + // consumer path with no equivalent issue) must stay in use whenever no caller filter is + // supplied, even when producer_id/consumer_id is. Only an actual filter should route + // through the new Specification/@EntityGraph (outer join) path - see + // ConfigurationProviderImpl.getFilteredActiveProducers/getFilteredConsumers. + + @Test + void getProducerConfigByClientId_noFilterNoId_usesPreExistingUnfilteredMethod_notSpecification() { + String clientId = "routing-client-1"; + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of()); + + configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + verify(producerService).getProducersByClientId(clientId); + verify(producerService, never()).getProducersByClientId(eq(clientId), any()); + } + + @Test + void getProducerConfigByClientId_idOnlyNoFilter_stillUsesPreExistingUnfilteredMethod() { + String clientId = "routing-client-2"; + ProducerDTO pr1 = producer(1L, true); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(pr1)); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.of(1L)); + + verify(producerService).getProducersByClientId(clientId); + verify(producerService, never()).getProducersByClientId(eq(clientId), any()); + assertThat(cfg.getProducers()).extracting(ProducerDTO::getId).containsExactly(1L); + } + + @Test + void getProducerConfigByClientId_withFilter_usesSpecificationOverload_notPreExistingMethod() { + String clientId = "routing-client-3"; + when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of()); + FilterNode.Comparison filter = FilterNode.Comparison.of("active", ComparisonOperator.EQ, true); + when(specificationPredicateCompiler.compile(eq(ResourceType.PRODUCER), eq(filter))) + .thenReturn(mock(Specification.class)); + + configurationProvider.getProducerConfigByClientId(clientId, Optional.empty(), Optional.of(filter)); + + verify(producerService).getProducersByClientId(eq(clientId), any()); + verify(producerService, never()).getProducersByClientId(clientId); + } + + @Test + void getConsumerConfigByClientId_noFilterNoId_usesPreExistingUnfilteredMethod_notSpecification() { + String clientId = "routing-client-4"; + when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of()); + + configurationProvider.getConsumerConfigByClientId(clientId, Optional.empty()); + + verify(consumerService).findByIdpClientId(clientId); + verify(consumerService, never()).findByIdpClientId(eq(clientId), any()); + } } From 99c1e8a175f918e03acd50f19ff9caa2e7acbbb3 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 15:43:02 +0200 Subject: [PATCH 20/22] fix(config): use compareTo for BigDecimal zero-validity check 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. --- .../providers/configuration/ConfigurationProviderImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java index 2c8a4fe..cdf1f0b 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java @@ -336,7 +336,7 @@ private void populateConsumersForProducers(List producers) { */ private boolean isValidProvider(ProductConsumerDTO provider) { - if (provider.getValidity() == null || provider.getValidity().equals(BigDecimal.ZERO)) return true; + if (provider.getValidity() == null || provider.getValidity().compareTo(BigDecimal.ZERO) == 0) return true; return isValidGrantedTs(provider.getGrantedTs(), provider.getValidity()); } From f2365c6ce23f237bceb503c86e23bda16d02aeb0 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 15:55:16 +0200 Subject: [PATCH 21/22] fix(config): resolve 20 SonarCloud code smells on PR #69 - 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. --- .../filter/FilterRequestParser.java | 16 ++-- .../SpecificationPredicateCompiler.java | 6 +- .../ConfigurationResourceRegistry.java | 39 +++++----- .../filter/FilterRequestParserTest.java | 76 ++++++++----------- .../SpecificationPredicateCompilerTest.java | 52 +++++++------ .../DynamicAttributeResolverTest.java | 3 +- .../ConfigurationProviderImplTest.java | 5 +- 7 files changed, 96 insertions(+), 101 deletions(-) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java index 6f04155..9922ed6 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParser.java @@ -8,6 +8,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.List; import java.util.Optional; import org.springframework.stereotype.Component; import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; @@ -66,21 +67,20 @@ private static void validate(FilterNode node) { throw new FilterCompilationException(Origin.REQUEST, "Filter must not be null"); } switch (node) { - case FilterNode.Comparison comparison -> { - if (comparison.attribute() == null || comparison.attribute().isBlank()) { + case FilterNode.Comparison(String attribute, ComparisonOperator operator, List values) -> { + if (attribute == null || attribute.isBlank()) { throw new FilterCompilationException(Origin.REQUEST, "A comparison must name an attribute"); } - if (comparison.operator() == null) { + if (operator == null) { throw new FilterCompilationException( - Origin.REQUEST, - "Comparison on attribute '" + comparison.attribute() + "' must name an operator"); + Origin.REQUEST, "Comparison on attribute '" + attribute + "' must name an operator"); } } - case FilterNode.Group group -> { - if (group.combinator() == null) { + case FilterNode.Group(Combinator combinator, List nodes) -> { + if (combinator == null) { throw new FilterCompilationException(Origin.REQUEST, "A filter group must name a combinator"); } - group.nodes().forEach(FilterRequestParser::validate); + nodes.forEach(FilterRequestParser::validate); } } } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java index 6f6729d..745e1e9 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompiler.java @@ -200,14 +200,14 @@ private static JpaExpression castTo( // Shared comparison building // ----------------------------------------------------------------------------------- - @SuppressWarnings({"unchecked", "rawtypes"}) + @SuppressWarnings("unchecked") private static Predicate buildComparison( CriteriaBuilder cb, Expression expression, ComparisonOperator operator, List values) { return switch (operator) { case EQ -> cb.equal(expression, values.get(0)); case NEQ -> cb.notEqual(expression, values.get(0)); - case IN -> ((Expression) expression).in(values); - case NOT_IN -> cb.not(((Expression) expression).in(values)); + case IN -> expression.in(values); + case NOT_IN -> cb.not(expression.in(values)); case LT -> cb.lessThan((Expression) expression, (Comparable) values.get(0)); case LTE -> cb.lessThanOrEqualTo((Expression) expression, (Comparable) values.get(0)); case GT -> cb.greaterThan((Expression) expression, (Comparable) values.get(0)); diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistry.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistry.java index cad479d..912a285 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistry.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/ConfigurationResourceRegistry.java @@ -56,28 +56,31 @@ public ResourceAttribute resolve(ResourceType resourceType, String logicalName) private static ResourceDefinition producerDefinition() { return new ResourceDefinition( ResourceType.PRODUCER, - Map.of( - "id", new ResourceAttribute.Fixed("id", "id", AttributeType.LONG), - "name", new ResourceAttribute.Fixed("name", "name", AttributeType.STRING), - "description", new ResourceAttribute.Fixed("description", "description", AttributeType.STRING), - "active", new ResourceAttribute.Fixed("active", "active", AttributeType.BOOLEAN), - "host", new ResourceAttribute.Fixed("host", "host", AttributeType.STRING), - "port", new ResourceAttribute.Fixed("port", "port", AttributeType.DECIMAL), - "tls", new ResourceAttribute.Fixed("tls", "tls", AttributeType.BOOLEAN), - "orgId", new ResourceAttribute.Fixed("orgId", "org.id", AttributeType.LONG))); + Map.ofEntries( + fixed("id", "id", AttributeType.LONG), + fixed("name", "name", AttributeType.STRING), + fixed("description", "description", AttributeType.STRING), + fixed("active", "active", AttributeType.BOOLEAN), + fixed("host", "host", AttributeType.STRING), + fixed("port", "port", AttributeType.DECIMAL), + fixed("tls", "tls", AttributeType.BOOLEAN), + fixed("orgId", "org.id", AttributeType.LONG))); } private static ResourceDefinition consumerDefinition() { return new ResourceDefinition( ResourceType.CONSUMER, - Map.of( - "id", new ResourceAttribute.Fixed("id", "id", AttributeType.LONG), - "name", new ResourceAttribute.Fixed("name", "name", AttributeType.STRING), - "scheduleType", - new ResourceAttribute.Fixed("scheduleType", "scheduleType", AttributeType.STRING), - "scheduleExpression", - new ResourceAttribute.Fixed( - "scheduleExpression", "scheduleExpression", AttributeType.STRING), - "orgId", new ResourceAttribute.Fixed("orgId", "org.id", AttributeType.LONG))); + Map.ofEntries( + fixed("id", "id", AttributeType.LONG), + fixed("name", "name", AttributeType.STRING), + fixed("scheduleType", "scheduleType", AttributeType.STRING), + fixed("scheduleExpression", "scheduleExpression", AttributeType.STRING), + fixed("orgId", "org.id", AttributeType.LONG))); + } + + /** A fixed-column map entry, keyed by the same logical name the {@link ResourceAttribute.Fixed} carries. */ + private static Map.Entry fixed( + String logicalName, String jpaPath, AttributeType type) { + return Map.entry(logicalName, new ResourceAttribute.Fixed(logicalName, jpaPath, type)); } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java index f0f0a91..be67953 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/FilterRequestParserTest.java @@ -13,7 +13,11 @@ import java.util.ArrayList; import java.util.List; import java.util.Optional; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import uk.gov.dbt.ndtp.ia.node.management.filter.FilterCompilationException.Origin; class FilterRequestParserTest { @@ -91,53 +95,35 @@ void parse_rejectsBareJsonNullLiteral() { .isEqualTo(Origin.REQUEST); } - @Test - void parse_rejectsComparisonMissingAttribute() { - String json = - """ - { "type": "comparison", "operator": "eq", "values": [true] } - """; - - assertThatThrownBy(() -> parser.parse(json)) - .isInstanceOf(FilterCompilationException.class) - .extracting(e -> ((FilterCompilationException) e).origin()) - .isEqualTo(Origin.REQUEST); - } - - @Test - void parse_rejectsComparisonMissingOperator() { - String json = - """ - { "type": "comparison", "attribute": "active", "values": [true] } - """; - - assertThatThrownBy(() -> parser.parse(json)) - .isInstanceOf(FilterCompilationException.class) - .extracting(e -> ((FilterCompilationException) e).origin()) - .isEqualTo(Origin.REQUEST); + static Stream incompleteFilters() { + return Stream.of( + Arguments.of( + "comparison missing attribute", + """ + { "type": "comparison", "operator": "eq", "values": [true] } + """), + Arguments.of( + "comparison missing operator", + """ + { "type": "comparison", "attribute": "active", "values": [true] } + """), + Arguments.of( + "group missing combinator", + """ + { "type": "group", "nodes": [ + { "type": "comparison", "attribute": "active", "operator": "eq", "values": [true] } + ] } + """), + Arguments.of( + "group with null element in nodes", + """ + { "type": "group", "combinator": "and", "nodes": [null] } + """)); } - @Test - void parse_rejectsGroupMissingCombinator() { - String json = - """ - { "type": "group", "nodes": [ - { "type": "comparison", "attribute": "active", "operator": "eq", "values": [true] } - ] } - """; - - assertThatThrownBy(() -> parser.parse(json)) - .isInstanceOf(FilterCompilationException.class) - .extracting(e -> ((FilterCompilationException) e).origin()) - .isEqualTo(Origin.REQUEST); - } - - @Test - void parse_rejectsGroupWithNullElementInNodes() { - String json = """ - { "type": "group", "combinator": "and", "nodes": [null] } - """; - + @ParameterizedTest(name = "{0}") + @MethodSource("incompleteFilters") + void parse_rejectsSemanticallyIncompleteFilter(String description, String json) { assertThatThrownBy(() -> parser.parse(json)) .isInstanceOf(FilterCompilationException.class) .extracting(e -> ((FilterCompilationException) e).origin()) diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java index 02e107a..814e978 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java @@ -292,8 +292,10 @@ void groupOr_combinesFixedAndDynamicComparisons() { @Test void unknownAttribute_rejectedWithRequestOriginAndNoInternalLeak() { // compile() only returns a lazy Specification; resolution happens when the predicate is built. - assertThatThrownBy(() -> execute(compiler() - .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("nope", ComparisonOperator.EQ, "x")))) + Specification spec = + compiler().compile(ResourceType.PRODUCER, FilterNode.Comparison.of("nope", ComparisonOperator.EQ, "x")); + + assertThatThrownBy(() -> execute(spec)) .isInstanceOf(FilterCompilationException.class) .satisfies(e -> { FilterCompilationException fce = (FilterCompilationException) e; @@ -307,10 +309,10 @@ void unknownAttribute_rejectedWithRequestOriginAndNoInternalLeak() { @Test void operatorUnsupportedForType_rejected() { - assertThatThrownBy(() -> execute(compiler() - .compile( - ResourceType.PRODUCER, - FilterNode.Comparison.of("active", ComparisonOperator.CONTAINS, "x")))) + Specification spec = compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("active", ComparisonOperator.CONTAINS, "x")); + + assertThatThrownBy(() -> execute(spec)) .isInstanceOf(FilterCompilationException.class) .extracting(e -> ((FilterCompilationException) e).origin()) .isEqualTo(Origin.REQUEST); @@ -318,10 +320,12 @@ void operatorUnsupportedForType_rejected() { @Test void wrongArity_rejectedForSingleValueOperator() { - assertThatThrownBy(() -> execute(compiler() - .compile( - ResourceType.PRODUCER, - new FilterNode.Comparison("active", ComparisonOperator.EQ, List.of(true, false))))) + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + new FilterNode.Comparison("active", ComparisonOperator.EQ, List.of(true, false))); + + assertThatThrownBy(() -> execute(spec)) .isInstanceOf(FilterCompilationException.class) .extracting(e -> ((FilterCompilationException) e).origin()) .isEqualTo(Origin.REQUEST); @@ -329,10 +333,11 @@ void wrongArity_rejectedForSingleValueOperator() { @Test void wrongOperandType_rejected() { - assertThatThrownBy(() -> execute(compiler() - .compile( - ResourceType.PRODUCER, - FilterNode.Comparison.of("port", ComparisonOperator.GT, "not-a-number")))) + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, FilterNode.Comparison.of("port", ComparisonOperator.GT, "not-a-number")); + + assertThatThrownBy(() -> execute(spec)) .isInstanceOf(FilterCompilationException.class) .extracting(e -> ((FilterCompilationException) e).origin()) .isEqualTo(Origin.REQUEST); @@ -347,12 +352,11 @@ void wrongOperandType_rejected() { @Test void multiValuedAttribute_rejectsNeq() { - AttributeDefinitionScope binding = persistProducerScopedDefinition("tags", "STRING", true); + persistProducerScopedDefinition("tags", "STRING", true); + Specification spec = compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("policy.tags", ComparisonOperator.NEQ, "red")); - assertThatThrownBy(() -> execute(compiler() - .compile( - ResourceType.PRODUCER, - FilterNode.Comparison.of("policy.tags", ComparisonOperator.NEQ, "red")))) + assertThatThrownBy(() -> execute(spec)) .isInstanceOf(FilterCompilationException.class) .extracting(e -> ((FilterCompilationException) e).origin()) .isEqualTo(Origin.REQUEST); @@ -361,12 +365,12 @@ void multiValuedAttribute_rejectsNeq() { @Test void multiValuedAttribute_rejectsNotIn() { persistProducerScopedDefinition("tags-not-in", "STRING", true); + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + new FilterNode.Comparison("policy.tags-not-in", ComparisonOperator.NOT_IN, List.of("red"))); - assertThatThrownBy(() -> execute(compiler() - .compile( - ResourceType.PRODUCER, - new FilterNode.Comparison( - "policy.tags-not-in", ComparisonOperator.NOT_IN, List.of("red"))))) + assertThatThrownBy(() -> execute(spec)) .isInstanceOf(FilterCompilationException.class) .extracting(e -> ((FilterCompilationException) e).origin()) .isEqualTo(Origin.REQUEST); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java index 4a98dae..a5ed4b7 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java @@ -103,8 +103,9 @@ void resolve_isEmptyForMalformedLogicalName() { void resolve_throwsPolicyOriginForUnrecognisedDataType() { AttributeDefinition definition = persistDefinition("policy", "bad-type", "XML", false); bindToScope(definition, "PRODUCER"); + DynamicAttributeResolver resolver = resolver(); - assertThatThrownBy(() -> resolver().resolve(ResourceType.PRODUCER, "policy.bad-type")) + assertThatThrownBy(() -> resolver.resolve(ResourceType.PRODUCER, "policy.bad-type")) .isInstanceOf(FilterCompilationException.class) .extracting(e -> ((FilterCompilationException) e).origin()) .isEqualTo(Origin.POLICY); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java index 65ff584..e379a8b 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java @@ -415,8 +415,9 @@ void getProducerConfigByClientId_withFilter_usesSpecificationOverload_notPreExis String clientId = "routing-client-3"; when(producerService.getProducersByClientId(eq(clientId), any())).thenReturn(List.of()); FilterNode.Comparison filter = FilterNode.Comparison.of("active", ComparisonOperator.EQ, true); - when(specificationPredicateCompiler.compile(eq(ResourceType.PRODUCER), eq(filter))) - .thenReturn(mock(Specification.class)); + Specification compiledSpec = mock(Specification.class); + when(specificationPredicateCompiler.compile(ResourceType.PRODUCER, filter)) + .thenReturn(compiledSpec); configurationProvider.getProducerConfigByClientId(clientId, Optional.empty(), Optional.of(filter)); From d836ed7960eb831fa87e7983b0a6ffed7c61b2c7 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Fri, 4 Sep 2026 16:29:26 +0200 Subject: [PATCH 22/22] test(config): fix field-shadowing smell and close compiler coverage gaps - 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. --- .../SpecificationPredicateCompilerTest.java | 83 +++++++++++++++++++ .../DynamicAttributeResolverTest.java | 4 +- .../ConfigurationProviderImplTest.java | 16 ++++ 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java index 814e978..38d864e 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/compiler/SpecificationPredicateCompilerTest.java @@ -136,6 +136,89 @@ void fixedAttributeEquality_matchesOnlyExpectedRows() { assertThat(execute(spec)).extracting(Producer::getId).containsExactly(active.getId()); } + @Test + void fixedAttributeNeq_matchesOnlyNonMatchingRow() { + Organisation org = persistOrganisation("org-neq"); + Producer active = persistProducer(org, "active-producer-neq", true); + persistProducer(org, "inactive-producer-neq", false); + entityManager.flush(); + + Specification spec = compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("active", ComparisonOperator.NEQ, false)); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactly(active.getId()); + } + + @Test + void fixedAttributeIn_matchesAnyListedId() { + Organisation org = persistOrganisation("org-in"); + Producer first = persistProducer(org, "in-producer-1", true); + Producer second = persistProducer(org, "in-producer-2", true); + persistProducer(org, "in-producer-3", true); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + new FilterNode.Comparison("id", ComparisonOperator.IN, List.of(first.getId(), second.getId()))); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactlyInAnyOrder(first.getId(), second.getId()); + } + + @Test + void fixedAttributeNotIn_excludesListedIds() { + Organisation org = persistOrganisation("org-not-in"); + Producer excluded = persistProducer(org, "not-in-producer-1", true); + Producer kept = persistProducer(org, "not-in-producer-2", true); + entityManager.flush(); + + Specification spec = compiler() + .compile( + ResourceType.PRODUCER, + new FilterNode.Comparison("id", ComparisonOperator.NOT_IN, List.of(excluded.getId()))); + + assertThat(execute(spec)) + .extracting(Producer::getId) + .contains(kept.getId()) + .doesNotContain(excluded.getId()); + } + + @Test + void fixedAttributeRangeOperators_compareDecimalColumn() { + Organisation org = persistOrganisation("org-range"); + Producer low = persistProducer(org, "range-producer-low", true); + low.setPort(BigDecimal.valueOf(100)); + Producer high = persistProducer(org, "range-producer-high", true); + high.setPort(BigDecimal.valueOf(900)); + entityManager.flush(); + + assertThat(execute(compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("port", ComparisonOperator.LT, 500)))) + .extracting(Producer::getId) + .containsExactly(low.getId()); + assertThat(execute(compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("port", ComparisonOperator.LTE, 100)))) + .extracting(Producer::getId) + .containsExactly(low.getId()); + assertThat(execute(compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("port", ComparisonOperator.GTE, 900)))) + .extracting(Producer::getId) + .containsExactly(high.getId()); + } + + @Test + void fixedAttributeContains_matchesCaseInsensitiveSubstring() { + Organisation org = persistOrganisation("org-contains"); + Producer matching = persistProducer(org, "alpha-producer", true); + persistProducer(org, "beta-producer", true); + entityManager.flush(); + + Specification spec = compiler() + .compile(ResourceType.PRODUCER, FilterNode.Comparison.of("name", ComparisonOperator.CONTAINS, "ALPHA")); + + assertThat(execute(spec)).extracting(Producer::getId).containsExactly(matching.getId()); + } + // 3.2 dynamic attribute EXISTS subquery @Test diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java index a5ed4b7..3faa379 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/filter/registry/DynamicAttributeResolverTest.java @@ -103,9 +103,9 @@ void resolve_isEmptyForMalformedLogicalName() { void resolve_throwsPolicyOriginForUnrecognisedDataType() { AttributeDefinition definition = persistDefinition("policy", "bad-type", "XML", false); bindToScope(definition, "PRODUCER"); - DynamicAttributeResolver resolver = resolver(); + DynamicAttributeResolver underTest = resolver(); - assertThatThrownBy(() -> resolver.resolve(ResourceType.PRODUCER, "policy.bad-type")) + assertThatThrownBy(() -> underTest.resolve(ResourceType.PRODUCER, "policy.bad-type")) .isInstanceOf(FilterCompilationException.class) .extracting(e -> ((FilterCompilationException) e).origin()) .isEqualTo(Origin.POLICY); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java index e379a8b..638e5ae 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java @@ -31,6 +31,7 @@ import uk.gov.dbt.ndtp.ia.node.management.filter.compiler.SpecificationPredicateCompiler; import uk.gov.dbt.ndtp.ia.node.management.filter.registry.ResourceType; import uk.gov.dbt.ndtp.ia.node.management.model.dto.*; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Consumer; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; import uk.gov.dbt.ndtp.ia.node.management.service.data.ConsumerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProducerService; @@ -435,4 +436,19 @@ void getConsumerConfigByClientId_noFilterNoId_usesPreExistingUnfilteredMethod_no verify(consumerService).findByIdpClientId(clientId); verify(consumerService, never()).findByIdpClientId(eq(clientId), any()); } + + @Test + void getConsumerConfigByClientId_withFilter_usesSpecificationOverload_notPreExistingMethod() { + String clientId = "routing-client-5"; + when(consumerService.findByIdpClientId(eq(clientId), any())).thenReturn(List.of()); + FilterNode.Comparison filter = FilterNode.Comparison.of("name", ComparisonOperator.EQ, "c1"); + Specification compiledSpec = mock(Specification.class); + when(specificationPredicateCompiler.compile(ResourceType.CONSUMER, filter)) + .thenReturn(compiledSpec); + + configurationProvider.getConsumerConfigByClientId(clientId, Optional.empty(), Optional.of(filter)); + + verify(consumerService).findByIdpClientId(eq(clientId), any()); + verify(consumerService, never()).findByIdpClientId(clientId); + } }