From c1074db3a4ba7befab5ce12e0fd0edceee0cb47c Mon Sep 17 00:00:00 2001 From: Clemens Portele Date: Mon, 7 Sep 2026 11:10:44 +0200 Subject: [PATCH] fix a create reporting the identifier of the request body For a feature type whose identifier the database assigns on insert, a create reported the value of the id column in the decoded request body instead of the identifier the insert returned: the `Location` header of the response named a feature that does not exist, and the per-role column overrides of a transaction updated no row, because they are keyed on the reported identifier. The value of the id column in a written feature is the identifier of the feature only where the client assigns it, for example a `gml:id` decoded into an `objid` column, where the surrogate primary key is not the identifier. Where the database generates the identifier, a value in the request body is not inserted at all, so the identifier is the one the insert returns. `SqlQueryMapping.hasGeneratedId` is the rule that decides this, lifted out of `FeatureProviderSql.hasGeneratedId`, which used it for the collection metadata already, and applied where the mutation session determines the identifier of a written feature. `CreatedFeatureIdSpec` locks which id a write reports, for all four variants of the documentation: a generated id, a client-assigned id in another column, a primary key with `{generated=false}`, and an id the caller states. The rows and the insert statements are stubbed, so the rule is exercised without the stream that drains the request body; the entry point that the spec drives derives the id from the mapping, which the two callers used to do with duplicated code. --- .../features/sql/app/SqlMutationSession.java | 60 +++--- .../sql/domain/FeatureProviderSql.java | 21 +- .../features/sql/domain/SqlQueryMapping.java | 20 ++ .../sql/app/CreatedFeatureIdSpec.groovy | 185 ++++++++++++++++++ .../sql/domain/GeneratedFeatureIdSpec.groovy | 111 +++++++++++ 5 files changed, 354 insertions(+), 43 deletions(-) create mode 100644 xtraplatform-features-sql/src/test/groovy/de/ii/xtraplatform/features/sql/app/CreatedFeatureIdSpec.groovy create mode 100644 xtraplatform-features-sql/src/test/groovy/de/ii/xtraplatform/features/sql/domain/GeneratedFeatureIdSpec.groovy diff --git a/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/app/SqlMutationSession.java b/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/app/SqlMutationSession.java index 6b897fa78..7571140e4 100644 --- a/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/app/SqlMutationSession.java +++ b/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/app/SqlMutationSession.java @@ -153,15 +153,9 @@ public FeatureTransactions.MutationResult createFeatures( } RowCursor rowCursor = new RowCursor(mapping.getMainTable().getFullPath()); - Optional> - roleIdColumn = mapping.getColumnForId(); - String roleIdColumnName = roleIdColumn.map(t -> t.second().getName()).orElse(null); - SqlQuerySchema roleIdTable = - roleIdColumn.map(de.ii.xtraplatform.base.domain.util.Tuple::first).orElse(null); try { - writeFeaturesBatched( - collected, rowCursor, Optional.empty(), crs, roleIdTable, roleIdColumnName, builder); + writeCollectedFeatures(mapping, collected, rowCursor, Optional.empty(), crs, false, builder); } catch (RuntimeException e) { builder.error(e); } @@ -1581,6 +1575,41 @@ public void close() { sqlSession.close(); } + /** + * Writes the features that were drained from the request and reports their ids. + * + *

The value of the role-id column in a written feature is the externally visible id of the + * feature, but only where the client assigns it (e.g. an ALKIS {@code gml:id} decoded into an + * {@code objid} column, where the surrogate primary key is not the id). Where the database + * generates the id on insert, an id in the request body is not inserted at all — the id of the + * feature is the one the insert returns, so the column is not consulted. + * + *

Package-private so a spec can exercise which id a write reports without the stream that + * drains the request body. + */ + void writeCollectedFeatures( + SqlQueryMapping mapping, + List collected, + RowCursor rowCursor, + Optional featureId, + EpsgCrs crs, + boolean deleteFirst, + ImmutableMutationResult.Builder builder) { + Optional> + roleIdColumn = mapping.hasGeneratedId() ? Optional.empty() : mapping.getColumnForId(); + String roleIdColumnName = roleIdColumn.map(t -> t.second().getName()).orElse(null); + SqlQuerySchema roleIdTable = + roleIdColumn.map(de.ii.xtraplatform.base.domain.util.Tuple::first).orElse(null); + + if (deleteFirst) { + writeFeaturesPerFeature( + collected, rowCursor, featureId, crs, true, roleIdTable, roleIdColumnName, builder); + } else { + writeFeaturesBatched( + collected, rowCursor, featureId, crs, roleIdTable, roleIdColumnName, builder); + } + } + private FeatureTransactions.MutationResult writeFeatures( FeatureTransactions.MutationResult.Type type, String featureType, @@ -1605,23 +1634,8 @@ private FeatureTransactions.MutationResult writeFeatures( type == FeatureTransactions.MutationResult.Type.UPDATE || type == FeatureTransactions.MutationResult.Type.REPLACE; - // Role-id column on the main table — its value in the inserted feature is the externally - // visible feature id (e.g. ALKIS gml:id stored in 'objid'); fall back to the surrogate PK only - // when no role-id column / no value is present. - Optional> - roleIdColumn = mapping.getColumnForId(); - String roleIdColumnName = roleIdColumn.map(t -> t.second().getName()).orElse(null); - SqlQuerySchema roleIdTable = - roleIdColumn.map(de.ii.xtraplatform.base.domain.util.Tuple::first).orElse(null); - try { - if (deleteFirst) { - writeFeaturesPerFeature( - collected, rowCursor, featureId, crs, true, roleIdTable, roleIdColumnName, builder); - } else { - writeFeaturesBatched( - collected, rowCursor, featureId, crs, roleIdTable, roleIdColumnName, builder); - } + writeCollectedFeatures(mapping, collected, rowCursor, featureId, crs, deleteFirst, builder); } catch (RuntimeException e) { builder.error(e); } diff --git a/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/domain/FeatureProviderSql.java b/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/domain/FeatureProviderSql.java index dd1a549ee..36808a643 100644 --- a/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/domain/FeatureProviderSql.java +++ b/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/domain/FeatureProviderSql.java @@ -98,7 +98,6 @@ import de.ii.xtraplatform.features.sql.app.SqlQueryTemplates; import de.ii.xtraplatform.features.sql.app.SqlQueryTemplatesDeriver; import de.ii.xtraplatform.features.sql.domain.FeatureProviderSqlData.QueryGeneratorSettings; -import de.ii.xtraplatform.features.sql.domain.SqlQueryColumn.Operation; import de.ii.xtraplatform.features.sql.infra.db.SourceSchemaValidatorSql; import de.ii.xtraplatform.geometries.domain.transcode.wktwkb.WkbDialect; import de.ii.xtraplatform.services.domain.AuditLog; @@ -1300,25 +1299,7 @@ public boolean hasGeneratedId(String featureType) { Optional.ofNullable(queryMappings.get(featureType)); if (queryMapping.isPresent()) { - return queryMapping.get().stream() - .allMatch( - mapping -> { - if (mapping.getColumnForId().isPresent() && mapping.getSchemaForId().isPresent()) { - String primaryKey = mapping.getColumnForId().get().first().getPrimaryKey(); - String idColumn = mapping.getColumnForId().get().second().getName(); - - if (!Objects.equals(primaryKey, idColumn)) { - return false; - } - - return !mapping - .getColumnForId() - .get() - .second() - .hasOperation(Operation.DO_NOT_GENERATE); - } - return true; - }); + return queryMapping.get().stream().allMatch(SqlQueryMapping::hasGeneratedId); } return true; diff --git a/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/domain/SqlQueryMapping.java b/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/domain/SqlQueryMapping.java index bef49b975..55cde094f 100644 --- a/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/domain/SqlQueryMapping.java +++ b/xtraplatform-features-sql/src/main/java/de/ii/xtraplatform/features/sql/domain/SqlQueryMapping.java @@ -93,6 +93,26 @@ default Optional> getColumnForId() { return getColumnForRole(Role.ID); } + /** + * Whether the database assigns the id of a new feature on insert: the column of the property with + * the role {@code ID} is the primary key of its table and is not excluded from generation with + * {@code {generated=false}}. Where the id is generated, a value in the request body is not + * inserted, so the id of a new feature is the one that the insert returns. + */ + default boolean hasGeneratedId() { + Optional> column = getColumnForId(); + + if (column.isEmpty() || getSchemaForId().isEmpty()) { + return true; + } + + if (!Objects.equals(column.get().first().getPrimaryKey(), column.get().second().getName())) { + return false; + } + + return !column.get().second().hasOperation(SqlQueryColumn.Operation.DO_NOT_GENERATE); + } + default Optional> getColumnForFilterGeometry() { return getColumnForRole(Role.FILTER_GEOMETRY).or(() -> getColumnForRole(Role.PRIMARY_GEOMETRY)); } diff --git a/xtraplatform-features-sql/src/test/groovy/de/ii/xtraplatform/features/sql/app/CreatedFeatureIdSpec.groovy b/xtraplatform-features-sql/src/test/groovy/de/ii/xtraplatform/features/sql/app/CreatedFeatureIdSpec.groovy new file mode 100644 index 000000000..42086026a --- /dev/null +++ b/xtraplatform-features-sql/src/test/groovy/de/ii/xtraplatform/features/sql/app/CreatedFeatureIdSpec.groovy @@ -0,0 +1,185 @@ +/* + * Copyright 2026 interactive instruments GmbH + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ +package de.ii.xtraplatform.features.sql.app + +import de.ii.xtraplatform.crs.domain.OgcCrs +import de.ii.xtraplatform.features.domain.FeatureTransactions +import de.ii.xtraplatform.features.domain.ImmutableFeatureSchema +import de.ii.xtraplatform.features.domain.ImmutableMutationResult +import de.ii.xtraplatform.features.domain.SchemaBase +// the rows of a feature and the insert statements use different Tuple types +import de.ii.xtraplatform.base.domain.util.Tuple as RowTuple +import de.ii.xtraplatform.features.domain.Tuple as StatementTuple +import de.ii.xtraplatform.features.sql.domain.ImmutableSqlQueryColumn +import de.ii.xtraplatform.features.sql.domain.ImmutableSqlQueryMapping +import de.ii.xtraplatform.features.sql.domain.ImmutableSqlQuerySchema +import de.ii.xtraplatform.features.sql.domain.SqlQueryColumn +import de.ii.xtraplatform.features.sql.domain.SqlQueryMapping +import de.ii.xtraplatform.features.sql.domain.SqlQuerySchema +import de.ii.xtraplatform.features.sql.domain.SqlSession +import spock.lang.Specification + +import java.util.function.Consumer +import java.util.function.Supplier + +/** + * Which identifier a create reports. The identifier of a new feature is + * + *

+ * + * The second rule must not apply where the database generates the identifier: a value in the + * request body is not inserted then, so reporting it names a feature that does not exist. That + * was the defect of the {@code Location} header of a create (see the ATS test + * {@code /conf/features/gml-srsname} of OGC API - Features - Part 4, whose GML request body + * carries the mandatory {@code gml:id}). + * + *

The rows and the insert statements are stubbed, so the rule is exercised without the + * encoder and the stream runner. + */ +class CreatedFeatureIdSpec extends Specification { + + static final String RETURNED_ID = 'B.100' + static final String ID_IN_REQUEST_BODY = 'B.n1' + + SqlSession sqlSession + FeatureMutationsSql featureMutationsSql + + def setup() { + sqlSession = Stub(SqlSession) + sqlSession.runReturning(_ as String) >> [RETURNED_ID] + featureMutationsSql = Stub(FeatureMutationsSql) + } + + def 'a generated identifier is the one the insert returns, not the one in the request body'() { + given: 'a type whose id column is the primary key, so the database generates the identifier' + SqlQueryMapping mapping = mapping('id', false) + + when: 'a create with an identifier in the request body' + List ids = reportedIds(mapping, Optional.empty()) + + then: 'the value of the request body is not inserted, so it is not the identifier' + ids == [RETURNED_ID] + } + + def 'a client-assigned identifier is the value of the id column in the request body'() { + given: 'a type whose id column is not the primary key (e.g. an ALKIS objid)' + SqlQueryMapping mapping = mapping('objid', false) + + when: + List ids = reportedIds(mapping, Optional.empty()) + + then: 'the surrogate primary key that the insert returns is not the identifier' + ids == [ID_IN_REQUEST_BODY] + } + + def 'an identifier the client assigns with {generated=false} is taken from the request body'() { + given: 'the id column is the primary key, but it is not generated on insert' + SqlQueryMapping mapping = mapping('id', true) + + when: + List ids = reportedIds(mapping, Optional.empty()) + + then: + ids == [ID_IN_REQUEST_BODY] + } + + def 'the identifier the caller states wins over both'() { + given: 'a PUT that creates the feature at the URI of the request' + SqlQueryMapping mapping = mapping('id', false) + + when: + List ids = reportedIds(mapping, Optional.of('B.42')) + + then: + ids == ['B.42'] + } + + /** Runs the create path of the mutation session over one stubbed feature row. */ + List reportedIds(SqlQueryMapping mapping, Optional featureId) { + SqlQuerySchema table = mapping.getMainTable() + ModifiableSqlRowData row = ModifiableSqlRowData.create() + // the encoder stores SQL literals, so a string value is quoted + row.putValues(mapping.getColumnForId().get().second().getName(), "'$ID_IN_REQUEST_BODY'") + FeatureDataSql feature = ModifiableFeatureDataSql.create().setMapping(mapping) + feature.addRows(RowTuple.of(table, row)) + + featureMutationsSql.createInstanceInserts(_, _, _, _, _) >> [ + ({ -> + StatementTuple.of("INSERT INTO buildings (name) VALUES ('Old Mill') RETURNING id;", + ({ String id -> } as Consumer)) + } as Supplier) + ] + + SqlMutationSession session = new SqlMutationSession( + sqlSession, [buildings: [mapping]], featureMutationsSql, null, null, + Optional.empty(), null, Optional.empty()) + ImmutableMutationResult.Builder builder = ImmutableMutationResult.builder() + .type(FeatureTransactions.MutationResult.Type.CREATE) + .hasFeatures(false) + + // the entry point that derives the id from the mapping and the written row, i.e. the rule + // itself — not a value the spec computed for it + session.writeCollectedFeatures( + mapping, + [feature], + new RowCursor(table.getFullPath()), + featureId, + OgcCrs.CRS84, + false, + builder) + + return builder.build().getIds() + } + + /** + * A mapping with one table and one id column. + * + * @param idColumn the name of the column of the property with the role ID; 'id' is the + * primary key of the table, any other name is a column of its own + * @param doNotGenerate the flag {@code {generated=false}} on that column + */ + static SqlQueryMapping mapping(String idColumn, boolean doNotGenerate) { + SqlQueryColumn column = new ImmutableSqlQueryColumn.Builder() + .name(idColumn) + .pathSegment(idColumn) + .type(SchemaBase.Type.STRING) + .role(SchemaBase.Role.ID) + .operations(doNotGenerate + ? Map.of(SqlQueryColumn.Operation.DO_NOT_GENERATE, [] as String[]) + : Map.of()) + .schemaIndex(0) + .build() + SqlQuerySchema table = new ImmutableSqlQuerySchema.Builder() + .name('buildings') + .pathSegment('buildings') + .primaryKey('id') + .addColumns(column) + .build() + + return new ImmutableSqlQueryMapping.Builder() + .addTables(table) + .mainSchema(new ImmutableFeatureSchema.Builder() + .name('buildings') + .type(SchemaBase.Type.OBJECT) + .sourcePath('/buildings') + .putProperties2('id', new ImmutableFeatureSchema.Builder() + .type(SchemaBase.Type.STRING) + .sourcePath(idColumn) + .role(SchemaBase.Role.ID)) + .build()) + .putValueTables('id', table) + .putValueColumns('id', column) + .build() + } +} diff --git a/xtraplatform-features-sql/src/test/groovy/de/ii/xtraplatform/features/sql/domain/GeneratedFeatureIdSpec.groovy b/xtraplatform-features-sql/src/test/groovy/de/ii/xtraplatform/features/sql/domain/GeneratedFeatureIdSpec.groovy new file mode 100644 index 000000000..992c2b544 --- /dev/null +++ b/xtraplatform-features-sql/src/test/groovy/de/ii/xtraplatform/features/sql/domain/GeneratedFeatureIdSpec.groovy @@ -0,0 +1,111 @@ +/* + * Copyright 2026 interactive instruments GmbH + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ +package de.ii.xtraplatform.features.sql.domain + +import de.ii.xtraplatform.features.domain.ImmutableFeatureSchema +import de.ii.xtraplatform.features.domain.SchemaBase +import spock.lang.Specification +import spock.lang.Unroll + +/** + * Whether the database assigns the id of a new feature on insert. The four variants of the + * documentation of the SQL feature provider: + * + *

+ * + * The decision drives the id that a create reports: the id that the insert returns where the + * database assigns it, the value in the written row where the client does. + */ +class GeneratedFeatureIdSpec extends Specification { + + @Unroll + def 'the id is generated on insert: #expected — #variant'() { + expect: + mapping(primaryKey, idColumn, doNotGenerate).hasGeneratedId() == expected + + where: + variant | primaryKey | idColumn | doNotGenerate || expected + 'primary key, auto-generated' | 'id' | 'id' | false || true + 'primary key with {generated=false}' | 'id' | 'id' | true || false + 'another column (e.g. an ALKIS objid)' | 'id' | 'objid' | false || false + 'another column, {generated=false}' | 'id' | 'objid' | true || false + } + + def 'a type without a property with the role ID reports a generated id'() { + given: 'a mapping whose only column is a geometry' + SqlQueryColumn geometry = new ImmutableSqlQueryColumn.Builder() + .name('geom') + .pathSegment('geom') + .type(SchemaBase.Type.GEOMETRY) + .role(SchemaBase.Role.PRIMARY_GEOMETRY) + .schemaIndex(0) + .build() + SqlQuerySchema table = new ImmutableSqlQuerySchema.Builder() + .name('buildings') + .pathSegment('buildings') + .addColumns(geometry) + .build() + SqlQueryMapping mapping = new ImmutableSqlQueryMapping.Builder() + .addTables(table) + .mainSchema(new ImmutableFeatureSchema.Builder() + .name('buildings') + .type(SchemaBase.Type.OBJECT) + .sourcePath('/buildings') + .putProperties2('geom', new ImmutableFeatureSchema.Builder() + .type(SchemaBase.Type.GEOMETRY) + .sourcePath('geom') + .role(SchemaBase.Role.PRIMARY_GEOMETRY)) + .build()) + .putValueTables('geom', table) + .putValueColumns('geom', geometry) + .build() + + expect: 'nothing in the request body can be the id of the new feature' + mapping.hasGeneratedId() + } + + static SqlQueryMapping mapping(String primaryKey, String idColumn, boolean doNotGenerate) { + SqlQueryColumn column = new ImmutableSqlQueryColumn.Builder() + .name(idColumn) + .pathSegment(idColumn) + .type(SchemaBase.Type.STRING) + .role(SchemaBase.Role.ID) + .operations(doNotGenerate + ? Map.of(SqlQueryColumn.Operation.DO_NOT_GENERATE, [] as String[]) + : Map.of()) + .schemaIndex(0) + .build() + SqlQuerySchema table = new ImmutableSqlQuerySchema.Builder() + .name('buildings') + .pathSegment('buildings') + .primaryKey(primaryKey) + .addColumns(column) + .build() + + return new ImmutableSqlQueryMapping.Builder() + .addTables(table) + .mainSchema(new ImmutableFeatureSchema.Builder() + .name('buildings') + .type(SchemaBase.Type.OBJECT) + .sourcePath('/buildings') + .putProperties2('id', new ImmutableFeatureSchema.Builder() + .type(SchemaBase.Type.STRING) + .sourcePath(idColumn) + .role(SchemaBase.Role.ID)) + .build()) + .putValueTables('id', table) + .putValueColumns('id', column) + .build() + } +}