From 957337c3404bc4e3e307cd78ee34331cbc6d777d Mon Sep 17 00:00:00 2001 From: Leon van Zantvoort Date: Mon, 10 Aug 2026 14:51:52 +0200 Subject: [PATCH 1/2] fix!: derive KeyDelegate nullability from the underlying field Metamodel.key() only wraps metamodels that do not carry the Key marker, so KeyDelegate.isNullable()'s instanceof check on the delegate could never match: every factory-built key reported non-nullable, and keyset pagination accepted nullable cursor keys that silently skip NULL rows. KeyDelegate now derives nullability from the record field the metamodel designates, through a new MetamodelFactory.isNullable bridged via MetamodelHelper: a Key answers for itself, a unique field applies its nullsDistinct setting, an inline record derives from its constituent fields, and a plain field reports its own nullability. The sealed-entity field-resolution block in MetamodelFactory is extracted into a shared helper. BREAKING CHANGE: scroll now rejects a Metamodel.key()-wrapped nullable field with a descriptive PersistenceException instead of silently skipping NULL rows; a wrapped metamodel whose path does not resolve to a record field fails the same way at validation time. Fixes #403 --- .../core/template/impl/MetamodelFactory.java | 84 ++++++++++++++----- ...itoryPreparedStatementIntegrationTest.java | 24 +++++- .../st/orm/core/template/MetamodelTest.java | 42 ++++++++++ .../src/main/java/st/orm/Metamodel.java | 14 +++- .../src/main/java/st/orm/MetamodelHelper.java | 18 ++++ .../src/test/java/st/orm/KeyDelegateTest.java | 8 +- 6 files changed, 165 insertions(+), 25 deletions(-) diff --git a/storm-core/src/main/java/st/orm/core/template/impl/MetamodelFactory.java b/storm-core/src/main/java/st/orm/core/template/impl/MetamodelFactory.java index cd015212c..2718b7358 100644 --- a/storm-core/src/main/java/st/orm/core/template/impl/MetamodelFactory.java +++ b/storm-core/src/main/java/st/orm/core/template/impl/MetamodelFactory.java @@ -144,14 +144,7 @@ public static Metamodel of(@Nonnull Class rootTable public static Metamodel canonical(@Nonnull Metamodel metamodel) { try { Class rootTable = metamodel.root(); - Class fieldResolutionClass = rootTable; - if (rootTable.isSealed() && isSealedEntity(rootTable)) { - Class[] permitted = rootTable.getPermittedSubclasses(); - if (permitted != null && permitted.length > 0) { - fieldResolutionClass = (Class) permitted[0]; - } - } - String foreignKeyPath = primaryKeyThroughForeignKeyPath(fieldResolutionClass, metamodel.fieldPath()); + String foreignKeyPath = primaryKeyThroughForeignKeyPath(fieldResolutionClass(rootTable), metamodel.fieldPath()); if (foreignKeyPath == null) { return metamodel; } @@ -185,6 +178,69 @@ public static Metamodel of(@Nonnull Class rootTable return List.copyOf(result); } + /** + * Returns whether the field designated by the given metamodel allows NULL values, following the semantics of + * {@link Metamodel.Key#isNullable()}. A metamodel that carries the {@link Metamodel.Key} marker answers for + * itself. For any other metamodel the record field at the metamodel's path decides: a unique field applies its + * {@code nullsDistinct} setting, an inline record derives its nullability from its constituent fields, and a + * plain field is as nullable as the field itself, because no unique constraint restricts its NULL values. + * + *

This backs {@link Metamodel#key(Metamodel)} delegates, which wrap metamodels that do not carry the key + * marker themselves.

+ */ + public static boolean isNullable(@Nonnull Metamodel metamodel) { + if (metamodel instanceof Metamodel.Key key) { + return key.isNullable(); + } + String path = metamodel.fieldPath(); + if (path.isEmpty()) { + // The root metamodel designates the row itself rather than a column. + return false; + } + try { + RecordField field = getRecordField(fieldResolutionClass(metamodel.root()), path); + boolean nullsDistinct = getNullsDistinct(field); + boolean inline = !Ref.class.isAssignableFrom(field.type()) + && getORMConverter(field).isEmpty() + && isRecord(field.type()) + && !Data.class.isAssignableFrom(field.type()); + if (inline) { + // Mirrors SimpleKeyMetamodel: an inline record is nullable when its unique constraint treats NULLs + // as distinct and any of its constituent fields is nullable. + boolean fieldIsUnique = field.isAnnotationPresent(UK.class) || field.isAnnotationPresent(PK.class); + if (!fieldIsUnique || !nullsDistinct) { + return false; + } + for (var leaf : metamodel.flatten()) { + if (leaf instanceof Metamodel.Key leafKey && leafKey.isNullable()) { + return true; + } + } + return false; + } + return field.nullable() && nullsDistinct; + } catch (SqlTemplateException e) { + throw new PersistenceException("Failed to resolve nullability for metamodel field at path '%s' on type %s.".formatted(path, metamodel.root().getName()), e); + } + } + + /** + * Returns the class that field resolution should inspect for the given root table. For sealed entity interfaces, + * resolution is delegated to the first permitted subclass: the sealed interface itself declares accessor methods + * but is not a record, so {@code getRecordField()} cannot inspect it directly. This mirrors the delegation + * pattern used by {@code findPkField()}. + */ + @SuppressWarnings("unchecked") + private static Class fieldResolutionClass(@Nonnull Class rootTable) { + if (rootTable.isSealed() && isSealedEntity(rootTable)) { + Class[] permitted = rootTable.getPermittedSubclasses(); + if (permitted != null && permitted.length > 0) { + return (Class) permitted[0]; + } + } + return rootTable; + } + /** * Returns the path of the foreign key field when {@code path} names the primary key of a table reached through * a foreign key, or {@code null} otherwise. Mirrors the analysis in {@link #getModel(Class, String)}: inline @@ -242,17 +298,7 @@ private static Metamodel getModel(@Nonnull Class ro if (path.isEmpty()) { return (Metamodel) root(rootTable); } - // For sealed entity interfaces, delegate field resolution to the first permitted subclass. - // The sealed interface itself declares accessor methods but is not a record, so getRecordField() - // cannot inspect it directly. This mirrors the delegation pattern used by findPkField(). - Class fieldResolutionClass = rootTable; - if (rootTable.isSealed() && isSealedEntity(rootTable)) { - Class[] permitted = rootTable.getPermittedSubclasses(); - if (permitted != null && permitted.length > 0) { - //noinspection unchecked - fieldResolutionClass = (Class) permitted[0]; - } - } + Class fieldResolutionClass = fieldResolutionClass(rootTable); Class fieldType; String effectivePath; StringBuilder effectiveField; diff --git a/storm-core/src/test/java/st/orm/core/RepositoryPreparedStatementIntegrationTest.java b/storm-core/src/test/java/st/orm/core/RepositoryPreparedStatementIntegrationTest.java index f65536c3f..f8f7e2416 100644 --- a/storm-core/src/test/java/st/orm/core/RepositoryPreparedStatementIntegrationTest.java +++ b/storm-core/src/test/java/st/orm/core/RepositoryPreparedStatementIntegrationTest.java @@ -2591,6 +2591,27 @@ public void testScrollRejectsNullableCompoundKey() { .scroll(Scrollable.of(key, 5))); } + @Test + public void testScrollRejectsWrappedNullableKey() { + // Owner.telephone is @Nullable and not unique; its key() view keeps the field's nullability, + // so scroll rejects it as a cursor key. + var key = Metamodel.key(Owner_.telephone); + assertTrue(key.isNullable()); + assertThrows(PersistenceException.class, () -> + ORMTemplate.of(dataSource) + .selectFrom(Owner.class) + .scroll(Scrollable.of(key, 5))); + } + + @Test + public void testScrollWithWrappedNonNullableKey() { + // Owner.lastName is @Nonnull; its key() view passes the nullability validation. + var window = ORMTemplate.of(dataSource) + .selectFrom(Owner.class) + .scroll(Scrollable.of(Metamodel.key(Owner_.lastName), 5)); + assertEquals(5, window.content().size()); + } + @Test public void testScrollAcceptsNullsNotDistinctCompoundKey() { // EntityWithNullsNotDistinctUK has @UK(nullsDistinct = false) NullableCompoundUK. @@ -2607,10 +2628,11 @@ public void testScrollAcceptsNullsNotDistinctCompoundKey() { public void testMetamodelKeyFactory() { // Owner_.id is already a Key (via @PK -> @UK), so key() should return the same instance. assertSame(Owner_.id, Metamodel.key(Owner_.id)); - // Owner_.address is not unique, so key() should wrap it in a KeyDelegate. + // Owner_.address is an inline record, which carries the Key marker; key() returns it as-is. var addressKey = Metamodel.key(Owner_.address); assertNotNull(addressKey); assertInstanceOf(Metamodel.Key.class, addressKey); + assertSame(Owner_.address, addressKey); // The delegate should be equal to the original metamodel. assertEquals(Owner_.address, addressKey); assertEquals(addressKey, Owner_.address); diff --git a/storm-core/src/test/java/st/orm/core/template/MetamodelTest.java b/storm-core/src/test/java/st/orm/core/template/MetamodelTest.java index 60a78f133..66bf1a155 100644 --- a/storm-core/src/test/java/st/orm/core/template/MetamodelTest.java +++ b/storm-core/src/test/java/st/orm/core/template/MetamodelTest.java @@ -16,6 +16,7 @@ import st.orm.core.model.Owner; import st.orm.core.model.Owner_; import st.orm.core.model.Pet; +import st.orm.core.model.Pet_; import st.orm.core.model.VetSpecialty; import st.orm.core.model.VetSpecialtyPK; import st.orm.core.model.VetSpecialty_; @@ -276,6 +277,47 @@ public void testNonUniqueInlineRecordIsNotNullable() { assertFalse(key.isNullable()); } + // Metamodel.key() factory nullability tests. + + @Test + public void testWrappedNullableFieldIsNullable() { + // Owner.telephone is @Nullable and not unique, so its generated metamodel carries no Key marker; + // the key() view derives nullability from the underlying field. + Metamodel.Key key = Metamodel.key(Owner_.telephone); + assertInstanceOf(Metamodel.KeyDelegate.class, key); + assertTrue(key.isNullable()); + } + + @Test + public void testWrappedNonNullableFieldIsNotNullable() { + // Owner.firstName is @Nonnull, so its key() view is non-nullable. + Metamodel.Key key = Metamodel.key(Owner_.firstName); + assertInstanceOf(Metamodel.KeyDelegate.class, key); + assertFalse(key.isNullable()); + } + + @Test + public void testWrappedFactoryMetamodelWithNullableFieldIsNullable() { + // The factory route for manually constructed metamodels keeps the field's nullability. + Metamodel telephone = Metamodel.of(Owner.class, "telephone"); + assertTrue(Metamodel.key(telephone).isNullable()); + } + + @Test + public void testWrappedNestedFieldResolvesNullabilityAtLeaf() { + // A nested path resolves nullability at the leaf field. + Metamodel telephone = Metamodel.of(Pet.class, "owner.telephone"); + assertTrue(Metamodel.key(telephone).isNullable()); + Metamodel firstName = Metamodel.of(Pet.class, "owner.firstName"); + assertFalse(Metamodel.key(firstName).isNullable()); + } + + @Test + public void testWrappedNullableForeignKeyIsNullable() { + // Pet.owner is a @Nullable @FK, so its foreign key column allows NULLs. + assertTrue(Metamodel.key(Pet_.owner).isNullable()); + } + @Test @SuppressWarnings("unchecked") public void testNullableCompoundKeyLeafReturnsNull() { diff --git a/storm-foundation/src/main/java/st/orm/Metamodel.java b/storm-foundation/src/main/java/st/orm/Metamodel.java index c7b7b1872..066c9f403 100644 --- a/storm-foundation/src/main/java/st/orm/Metamodel.java +++ b/storm-foundation/src/main/java/st/orm/Metamodel.java @@ -222,6 +222,9 @@ interface Key extends Metamodel { * context where the key is used. Using a non-unique column as a keyset pagination key will silently skip rows when * duplicate values span page boundaries.

* + *

The returned key reports {@link Key#isNullable()} from the underlying field: a nullable field stays nullable + * when viewed as a key, so keyset pagination keeps rejecting it.

+ * * @param metamodel the metamodel to view as a key. * @return a {@code Key} instance backed by the given metamodel. * @param the root table type. @@ -263,7 +266,16 @@ record KeyDelegate(@Nonnull Metamodel delegate) impleme @Override public boolean isIdentical(@Nonnull T a, @Nonnull T b) { return delegate.isIdentical(a, b); } @Override public boolean isSame(@Nonnull T a, @Nonnull T b) { return delegate.isSame(a, b); } @Override public List> flatten() { return delegate.flatten(); } - @Override public boolean isNullable() { return delegate instanceof Key key && key.isNullable(); } + + /** + * Returns the nullability of the underlying field. A delegate around a {@link Key} answers through that key; + * any other delegate derives the answer from the record field the metamodel designates, so a nullable field + * wrapped by {@link #key(Metamodel)} is still rejected as a keyset pagination cursor. + */ + @Override + public boolean isNullable() { + return delegate instanceof Key key ? key.isNullable() : MetamodelHelper.isNullable(delegate); + } @Override public boolean equals(Object o) { diff --git a/storm-foundation/src/main/java/st/orm/MetamodelHelper.java b/storm-foundation/src/main/java/st/orm/MetamodelHelper.java index c7552f010..0c380718d 100644 --- a/storm-foundation/src/main/java/st/orm/MetamodelHelper.java +++ b/storm-foundation/src/main/java/st/orm/MetamodelHelper.java @@ -10,6 +10,7 @@ class MetamodelHelper { private static final Method ROOT_METHOD; private static final Method OF_METHOD; private static final Method FLATTEN_METHOD; + private static final Method IS_NULLABLE_METHOD; static { try { @@ -17,6 +18,7 @@ class MetamodelHelper { ROOT_METHOD = factoryClass.getMethod("root", Class.class); OF_METHOD = factoryClass.getMethod("of", Class.class, String.class); FLATTEN_METHOD = factoryClass.getMethod("flatten", Navigable.class); + IS_NULLABLE_METHOD = factoryClass.getMethod("isNullable", Metamodel.class); } catch (ReflectiveOperationException e) { var ex = new ExceptionInInitializerError("Failed to initialize Metamodel. Please ensure that storm-core is present in the classpath."); ex.initCause(e); @@ -78,4 +80,20 @@ static Metamodel of(Class rootTable, String path) { throw new PersistenceException(t); } } + + static boolean isNullable(@Nonnull Metamodel metamodel) { + try { + try { + return (Boolean) IS_NULLABLE_METHOD.invoke(null, metamodel); + } catch (InvocationTargetException e) { + throw e.getTargetException(); + } catch (ReflectiveOperationException e) { + throw new RuntimeException("Reflection invocation failed for MetamodelFactory.isNullable", e); + } + } catch (RuntimeException e) { + throw e; + } catch (Throwable t) { + throw new PersistenceException(t); + } + } } diff --git a/storm-foundation/src/test/java/st/orm/KeyDelegateTest.java b/storm-foundation/src/test/java/st/orm/KeyDelegateTest.java index fa17207dc..d68ede184 100644 --- a/storm-foundation/src/test/java/st/orm/KeyDelegateTest.java +++ b/storm-foundation/src/test/java/st/orm/KeyDelegateTest.java @@ -92,10 +92,10 @@ void keyDelegateDelegatesAllMethods() { } @Test - void keyDelegateIsNotNullableForNonKeyMetamodel() { - TestMetamodel metamodel = new TestMetamodel(); - Metamodel.Key key = Metamodel.key(metamodel); - assertFalse(key.isNullable()); + void keyDelegateIsNotNullableWhenDelegateIsNonNullableKey() { + TestKeyMetamodel nonNullableKey = new TestKeyMetamodel(false); + Metamodel.KeyDelegate delegate = new Metamodel.KeyDelegate<>(nonNullableKey); + assertFalse(delegate.isNullable()); } @Test From e4dbf0dc6c55b3c2e00d27b71a53c7d3b81e151d Mon Sep 17 00:00:00 2001 From: Leon van Zantvoort Date: Mon, 10 Aug 2026 15:15:32 +0200 Subject: [PATCH 2/2] refactor: narrow reflective invoke catches to IllegalAccessException Method.invoke's only remaining checked reflective failure once InvocationTargetException is handled is IllegalAccessException; the broader ReflectiveOperationException catch is flagged as masked by CodeQL. The static initializer keeps the broad catch, where Class.forName and getMethod throw other reflective exceptions. Also names MetamodelFactory.root in root()'s failure message. --- .../src/main/java/st/orm/MetamodelHelper.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/storm-foundation/src/main/java/st/orm/MetamodelHelper.java b/storm-foundation/src/main/java/st/orm/MetamodelHelper.java index 0c380718d..185c554c8 100644 --- a/storm-foundation/src/main/java/st/orm/MetamodelHelper.java +++ b/storm-foundation/src/main/java/st/orm/MetamodelHelper.java @@ -37,8 +37,8 @@ static Metamodel root(@Nonnull Class rootTable) { return (Metamodel) ROOT_METHOD.invoke(null, rootTable); } catch (InvocationTargetException e) { throw e.getTargetException(); - } catch (ReflectiveOperationException e) { - throw new RuntimeException("Reflection invocation failed for MetamodelFactory.of", e); + } catch (IllegalAccessException e) { + throw new RuntimeException("Reflection invocation failed for MetamodelFactory.root", e); } } catch (RuntimeException e) { throw e; @@ -54,7 +54,7 @@ static Metamodel of(Class rootTable, String path) { return (Metamodel) OF_METHOD.invoke(null, rootTable, path); } catch (InvocationTargetException e) { throw e.getTargetException(); - } catch (ReflectiveOperationException e) { + } catch (IllegalAccessException e) { throw new RuntimeException("Reflection invocation failed for MetamodelFactory.of", e); } } catch (RuntimeException e) { @@ -71,7 +71,7 @@ static Metamodel of(Class rootTable, String path) { return (List>) FLATTEN_METHOD.invoke(null, metamodel); } catch (InvocationTargetException e) { throw e.getTargetException(); - } catch (ReflectiveOperationException e) { + } catch (IllegalAccessException e) { throw new RuntimeException("Reflection invocation failed for MetamodelFactory.flatten", e); } } catch (RuntimeException e) { @@ -87,7 +87,7 @@ static boolean isNullable(@Nonnull Metamodel metamodel) { return (Boolean) IS_NULLABLE_METHOD.invoke(null, metamodel); } catch (InvocationTargetException e) { throw e.getTargetException(); - } catch (ReflectiveOperationException e) { + } catch (IllegalAccessException e) { throw new RuntimeException("Reflection invocation failed for MetamodelFactory.isNullable", e); } } catch (RuntimeException e) {