jdaugherty commented on code in PR #16583: URL: https://github.com/apache/grails-core/pull/16583#discussion_r4363457645
########## grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/dialect/UniqueNullsPerDialect.java: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.grails.orm.hibernate.dialect; + +import java.util.Arrays; + +import org.hibernate.dialect.Dialect; +import org.hibernate.dialect.Oracle8iDialect; + +import org.grails.datastore.mapping.config.UniqueNulls; + +/** + * How the unique index Hibernate creates for a {@code unique} constraint with a group treats a {@code null} in the key, + * on each database, so that the validation can read {@code null}s the way the index will. + * <ul> + * <li>{@link #ORACLE}: Oracle indexes a row that has a {@code null} in some of the key columns and compares its + * other columns, so {@code ('en', null)} twice is a violation, and only a key that is {@code null} in every column is + * left out. A {@code null} is a value: {@link UniqueNulls#VALUE}.</li> + * <li>{@link #DEFAULT}: H2, PostgreSQL, MySQL and MariaDB treat {@code null}s as distinct, so such a row never + * conflicts. SQL Server and DB2 compare them, but Hibernate renders the index for a nullable key as a filtered unique + * index that leaves the rows with a {@code null} out, which comes to the same. {@link UniqueNulls#SKIP}.</li> + * </ul> + * + * @since 9.0 + */ +public enum UniqueNullsPerDialect { + + ORACLE(UniqueNulls.VALUE, Oracle8iDialect.class), + + DEFAULT(UniqueNulls.SKIP, Dialect.class); Review Comment: Not under Hibernate 5. `hibernate-core` 5.6 has no filtered-index delegate at all (its `org.hibernate.dialect.unique` package holds `DefaultUniqueDelegate`, `DB2UniqueDelegate` and `InformixUniqueDelegate`): `SQLServerDialect`, its 2005 to 2016 subclasses and `AbstractTransactSQLDialect` do not override `getUniqueDelegate()`, so they get `Dialect`'s `DefaultUniqueDelegate`, which renders `alter table ... add constraint ... unique (...)`. SQL Server compares nulls under a plain unique constraint (Hibernate 7's `AlterTableUniqueIndexDelegate` javadoc says that is why it moved SQL Server to the filtered index), so `('en', null)` twice is rejected by the index while this table has the validator read `skip`: the row passes validation and fails at flush. Sybase is the same case on 5.6: no override, plain constraint, and Hibernate's own `SkipNullableUniqueDelegate` javadoc says Sybase "always consider[s] null values to be non-unique". Please add `SQL_SERVER(UniqueNulls.VALUE, SQLServerDialect.class)` and `SYBASE(UniqueNulls.VALUE, SybaseDialect.class)` here and drop the filtered-index sentence from this javadoc; DB2 is right (`DB2Dialect` overrides with `DB2UniqueDelegate`, `exclude null keys`). While here: `Oracle8iDialect` misses the deprecated `OracleDialect`/`Oracle9Dialect` pair, which extend `Dialect` directly; `Oracle9Dialect.class` as a second Oracle match covers them. ########## grails-doc/src/en/ref/Constraints/unique.adoc: ########## @@ -64,6 +64,60 @@ username(unique: ['group', 'department']) In this example `username` must be unique in `group` and `department`. There might be same logins in different groups or different departments. +==== Null values in a composite key + +A property of the key can be `null`: a `country` that is optional in `language unique: 'country'`, a `parent` that is `null` for the top level nodes of a tree, or the constrained property itself when it is `nullable`. Whether such a row can be a duplicate of another is a question databases answer in two ways for their unique index, so the constraint has four readings of a `null`, selected per constraint or for the whole application: + +[cols="1,4"] +|=== +|Reading |A `null` in the key + +|`database` (default) +|Is read the way the unique index of the database does: `skip` where the index treats `null` values as distinct, `value` where it compares them. The validation and the index then agree. + +|`skip` +|Is never a duplicate, as in a SQL unique predicate: `('en', null)` twice is valid, and the lookup is not made. + +|`value` +|Is a value that matches only another `null`: `('en', null)` conflicts with another `('en', null)` and with no row that has a country. This holds for the constrained property as well. + +|`wildcard` +|In the group, leaves its property out of the lookup, so it matches every row: `('en', null)` conflicts with `('en', 'US')`. A `null` constrained property is not checked. +|=== + +With rows `('en', 'US')` and `('en', null)`, a new `('en', null)` is a duplicate under `value` and `wildcard` and is valid under `skip`; a new `(null, 'US')` is a duplicate only under `value`. + +The unique index Hibernate creates for the group treats `null` values as follows, which is what `database` reads: + +[cols="2,1,3"] +|=== +|Database |Reading |Why + +|H2, PostgreSQL, MySQL, MariaDB +|`skip` +|`null` values are distinct in a unique index. + +|SQL Server, DB2 Review Comment: True for Hibernate 7, where `SQLServerDialect` and `DB2Dialect` both use `AlterTableUniqueIndexDelegate`. Under Hibernate 5 only DB2 renders the index that way; SQL Server and Sybase get a plain unique constraint and compare nulls, so they read `value` (see the comment on the Hibernate 5 `UniqueNullsPerDialect`). 9.0.x ships both, so please say which Hibernate each row describes and add the Hibernate 5 rows. ########## grails-doc/src/en/guide/upgrading/upgrading90x.adoc: ########## @@ -1151,3 +1151,34 @@ When upgrading an affected application: ---- See xref:theWebLayer#commandObjects[Command Objects] for a trait-based alternative to sharing a non-domain superclass with a domain class. + +==== 17. A Null in a Unique Constraint's Composite Key Follows the Database + +A `unique` constraint that names other properties, such as `url unique: 'httpMethod'`, validates the constrained property together with the properties of its group. +Before Grails 9, a group property whose value was `null` was left out of the query that looks for a conflicting row. +The constraint then checked the constrained property alone, so `new Language(language: 'en')` was rejected when a `Language` with `language: 'en'` and `country: 'US'` existed, although the two rows differ in `country`. +That was stricter than any reading of `null`: in SQL a `null` never conflicts, and where `null` is a value it conflicts only with another `null`. + +In Grails 9 the constraint reads a `null` in its key, in the group and in the constrained property, as the unique index of the database does, so that the validation and the index agree: + +* `skip` on H2, PostgreSQL, MySQL, MariaDB, SQL Server and DB2, whose index leaves a row with a `null` out: `('en', null)` is never a duplicate. Review Comment: Per the inline note on the Hibernate 5 table: on Hibernate 5, SQL Server and Sybase read `value`. Either qualify this bullet with Hibernate 7 and add the Hibernate 5 line, or point at the table in the constraints reference once it carries both. ########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/constraints/builtin/UniqueConstraint.groovy: ########## @@ -48,27 +55,72 @@ class UniqueConstraint extends AbstractConstraint { public static final String NAME = 'unique' + public static final String GROUP = 'group' + + public static final String NULLS = 'nulls' + protected List group = [] + /** The reading the constraint asks for, or {@code null} to follow the datastore. */ + protected UniqueNulls nulls + UniqueConstraint(Class<?> constraintOwningClass, String constraintPropertyName, Object constraintParameter, MessageSource messageSource) { super(constraintOwningClass, constraintPropertyName, constraintParameter, messageSource) - if (constraintParameter instanceof Iterable) { - for (property in ((Iterable) constraintParameter)) { + Object groupParameter = constraintParameter + if (constraintParameter instanceof Map) { + groupParameter = ((Map) constraintParameter).get(GROUP) + nulls = UniqueNulls.of(((Map) constraintParameter).get(NULLS)) + } + if (groupParameter instanceof Iterable) { + for (property in ((Iterable) groupParameter)) { group.add(property.toString()) } } - else if (constraintParameter instanceof CharSequence) { - group.add(constraintParameter.toString()) + else if (groupParameter instanceof CharSequence) { + group.add(groupParameter.toString()) } } @Override protected Object validateParameter(Object constraintParameter) { if (constraintParameter instanceof Boolean) { return constraintParameter - } else { - return constraintParameter instanceof Iterable || constraintParameter instanceof CharSequence } + if (constraintParameter instanceof Map) { + Map options = (Map) constraintParameter + Object groupOption = options.get(GROUP) + boolean knownKeys = options.keySet().every { it == GROUP || it == NULLS } + boolean validGroup = groupOption == null || groupOption instanceof Iterable || groupOption instanceof CharSequence + if (!knownKeys || !validGroup) { + throw new IllegalArgumentException("The unique constraint of [${constraintOwningClass.name}.${constraintPropertyName}] takes a map with a '${GROUP}' (a property name or a list of them) and a '${NULLS}' option, not ${options}") + } + UniqueNulls.of(options.get(NULLS)) + return options Review Comment: Nit: `unique: [:]` gets through here (no unknown key, no group) and then `if (constraintParameter)` at line 190 is false for an empty map, so the constraint validates nothing without a word. Reject an empty map here, or return `true` for it. ########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/constraints/builtin/UniqueConstraint.groovy: ########## @@ -157,6 +157,9 @@ class UniqueConstraint extends AbstractConstraint { } } eq(propName, value) + } else { + // null is compared as a value: only a row whose value is also null conflicts + isNull(propName) Review Comment: Verified at 70a0b0220a: the map form next to the `Iterable` and `CharSequence` forms, the three readings plus `database`, and `grails.gorm.unique.nulls` for the rest. ########## grails-doc/src/en/guide/upgrading/upgrading90x.adoc: ########## @@ -305,3 +305,22 @@ It applies nothing else. Null properties are written whatever `@JsonInclude` or `spring.jackson.default-property-inclusion` say, properties are written in the converter's order rather than by `@JsonPropertyOrder` or sorted alphabetically, and `@JsonView`, `@JsonFilter`, `@JsonAnyGetter`, `@JsonIgnoreType`, `@JsonFormat` and `@JsonSerialize` on a property, and mix-ins, other than the property names they give, do not apply. + +==== 5. A Null Value in the Group of a Unique Constraint Is Compared as a Value Review Comment: 9.0.x has 19 sections now (18 RxGORM on RxJava 3, 19 RxJava 1 and 2 deprecated), so this is 20 after the next merge; the file conflicts until then. ########## grails-doc/src/en/guide/upgrading/upgrading90x.adoc: ########## @@ -305,3 +305,22 @@ It applies nothing else. Null properties are written whatever `@JsonInclude` or `spring.jackson.default-property-inclusion` say, properties are written in the converter's order rather than by `@JsonPropertyOrder` or sorted alphabetically, and `@JsonView`, `@JsonFilter`, `@JsonAnyGetter`, `@JsonIgnoreType`, `@JsonFormat` and `@JsonSerialize` on a property, and mix-ins, other than the property names they give, do not apply. + +==== 5. A Null Value in the Group of a Unique Constraint Is Compared as a Value + +A `unique` constraint that names other properties, such as `url unique: 'httpMethod'`, validates the constrained property together with the properties of its group. +Before Grails 9, a group property whose value was `null` was left out of the query that looks for a conflicting row. +The constraint then checked the constrained property alone, so `new Language(language: 'en')` was rejected when a `Language` with `language: 'en'` and `country: 'US'` existed, although the two rows differ in `country`. + +In Grails 9 a `null` group value is compared as a value: only a row whose value of that group property is also `null` conflicts. +With `language unique: 'country'`, `('en', null)` no longer conflicts with `('en', 'US')`, and a second `('en', null)` still does. +The same holds for a nullable association in the group, so a folder with `name unique: 'parent'` may share its name with a folder under a parent, while two top level folders with the same name still conflict. + +A group with several properties compares every one of them, so `('A', null, 'web')` conflicts only with a row that has code `A`, no region and channel `web`. + +Applications that relied on the old behaviour are affected: a save that was rejected because a `null` group value matched every row of the constrained property now succeeds when the rows differ in that group property. +The old behaviour was a defect and has no replacement. +To keep a rule such as "the name is unique whatever the value of `parent`", declare the constraint on the one property, `name unique: true`. +If the database must also reject the second `('en', null)` row, remember that most databases do not treat `null` values of a unique index as equal; the validation does, the index does not. Review Comment: Verified for Hibernate 7 against `hibernate-core` 7.4: `SQLServerDialect` and `DB2Dialect` both use `AlterTableUniqueIndexDelegate`, so `skip` is right there and my list was wrong for 7. On Hibernate 5.6 it is the other way round: there is no filtered-index delegate, `SQLServerDialect` gets `DefaultUniqueDelegate` and a plain unique constraint, so SQL Server (and Sybase) read `value` while this table says `skip`. Inline on the Hibernate 5 `UniqueNullsPerDialect`. ########## grails-doc/src/en/ref/Constraints/unique.adoc: ########## @@ -64,6 +64,60 @@ username(unique: ['group', 'department']) In this example `username` must be unique in `group` and `department`. There might be same logins in different groups or different departments. +==== Null values in a composite key + +A property of the key can be `null`: a `country` that is optional in `language unique: 'country'`, a `parent` that is `null` for the top level nodes of a tree, or the constrained property itself when it is `nullable`. Whether such a row can be a duplicate of another is a question databases answer in two ways for their unique index, so the constraint has four readings of a `null`, selected per constraint or for the whole application: + +[cols="1,4"] +|=== +|Reading |A `null` in the key + +|`database` (default) +|Is read the way the unique index of the database does: `skip` where the index treats `null` values as distinct, `value` where it compares them. The validation and the index then agree. + +|`skip` +|Is never a duplicate, as in a SQL unique predicate: `('en', null)` twice is valid, and the lookup is not made. + +|`value` +|Is a value that matches only another `null`: `('en', null)` conflicts with another `('en', null)` and with no row that has a country. This holds for the constrained property as well. + +|`wildcard` +|In the group, leaves its property out of the lookup, so it matches every row: `('en', null)` conflicts with `('en', 'US')`. A `null` constrained property is not checked. +|=== + +With rows `('en', 'US')` and `('en', null)`, a new `('en', null)` is a duplicate under `value` and `wildcard` and is valid under `skip`; a new `(null, 'US')` is a duplicate only under `value`. Review Comment: With only those two rows, `(null, 'US')` matches nothing under `value` either: `isNull(language)` finds no row, so it is valid under every reading. The sentence holds once the rows include `(null, 'US')`, which is the four-row set `UniqueNullsSpec` and `UniqueConstraintNullsSpec` use; please list that row, or drop the clause. ########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/constraints/builtin/UniqueConstraint.groovy: ########## @@ -157,6 +157,9 @@ class UniqueConstraint extends AbstractConstraint { } } eq(propName, value) + } else { Review Comment: Verified: the early return and the `isNull` on the constrained property follow the same reading as the group, and the `(null, 'US')` and `(null, null)` rows pin it in both specs. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/Settings.java: ########## @@ -54,6 +54,11 @@ public interface Settings { * The default constraints */ String SETTING_DEFAULT_CONSTRAINTS = PREFIX + '.' + "default.constraints"; + /** + * How the {@code unique} constraint treats a {@code null} in a composite key: {@code wildcard}, {@code skip}, + * {@code value} or {@code database}, which is the default. See {@link UniqueNulls}. + */ + String SETTING_UNIQUE_NULLS = PREFIX + '.' + "unique.nulls"; Review Comment: `grails.gorm.unique.nulls` needs an entry in `grails-core/src/main/resources/META-INF/additional-spring-configuration-metadata.json`, next to `grails.gorm.default.nullable` and the other `grails.gorm.*` keys, so IDE hints and the configuration report know it: type `java.lang.String`, `defaultValue` `database`, and a description naming the four values. ########## grails-doc/src/en/guide/upgrading/upgrading90x.adoc: ########## @@ -305,3 +305,22 @@ It applies nothing else. Null properties are written whatever `@JsonInclude` or `spring.jackson.default-property-inclusion` say, properties are written in the converter's order rather than by `@JsonPropertyOrder` or sorted alphabetically, and `@JsonView`, `@JsonFilter`, `@JsonAnyGetter`, `@JsonIgnoreType`, `@JsonFormat` and `@JsonSerialize` on a property, and mix-ins, other than the property names they give, do not apply. + +==== 5. A Null Value in the Group of a Unique Constraint Is Compared as a Value + +A `unique` constraint that names other properties, such as `url unique: 'httpMethod'`, validates the constrained property together with the properties of its group. +Before Grails 9, a group property whose value was `null` was left out of the query that looks for a conflicting row. +The constraint then checked the constrained property alone, so `new Language(language: 'en')` was rejected when a `Language` with `language: 'en'` and `country: 'US'` existed, although the two rows differ in `country`. + +In Grails 9 a `null` group value is compared as a value: only a row whose value of that group property is also `null` conflicts. +With `language unique: 'country'`, `('en', null)` no longer conflicts with `('en', 'US')`, and a second `('en', null)` still does. +The same holds for a nullable association in the group, so a folder with `name unique: 'parent'` may share its name with a folder under a parent, while two top level folders with the same name still conflict. + +A group with several properties compares every one of them, so `('A', null, 'web')` conflicts only with a row that has code `A`, no region and channel `web`. + +Applications that relied on the old behaviour are affected: a save that was rejected because a `null` group value matched every row of the constrained property now succeeds when the rows differ in that group property. +The old behaviour was a defect and has no replacement. Review Comment: Verified: the note names the readings, the default and the `wildcard` line, and says why `unique: true` is not a replacement. ########## grails-doc/src/en/ref/Constraints/unique.adoc: ########## @@ -64,6 +64,8 @@ username(unique: ['group', 'department']) In this example `username` must be unique in `group` and `department`. There might be same logins in different groups or different departments. +A `null` value of a property in the group is compared as a value: it conflicts only with a row whose value of that property is also `null`. With `group unique: 'department'`, a group without a `department` may have the same name as a group of a department, but two groups with that name and no `department` conflict. This is the same for a group that has several properties and for an association in the group, such as a `parent` that is `null` for the top level nodes of a tree. Review Comment: Verified, with two corrections inline: the worked example's rows, and the SQL Server/DB2 row holding for Hibernate 7 only. ########## grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/UniqueConstraintNullGroupValueSpec.groovy: ########## @@ -0,0 +1,197 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.grails.data.testing.tck.tests + +import spock.lang.IgnoreIf +import spock.lang.Issue +import spock.lang.PendingFeatureIf + +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import org.apache.grails.data.testing.tck.domains.UniqueNullFolder +import org.apache.grails.data.testing.tck.domains.UniqueNullLanguage +import org.apache.grails.data.testing.tck.domains.UniqueNullMulti +import org.apache.grails.data.testing.tck.domains.UniqueNullRoute + +/** + * A null value of a property in the group of a {@code unique} constraint is compared as a value: + * it only conflicts with the rows whose value of that property is also null. + * + * The simple in-memory datastore of the TCK does not register the {@code unique} constraint, so this specification + * does not run there; {@code UniqueConstraintSpec} of {@code grails-datamapping-core-test} covers it. + */ +@IgnoreIf({ Boolean.getBoolean('simple.gorm.suite') }) +@Issue('https://github.com/apache/grails-core/issues/14503') +class UniqueConstraintNullGroupValueSpec extends GrailsDataTckSpec { Review Comment: Verified. Ran at this head on the in-memory datastore, Hibernate 7 (H2 and the PostgreSQL, MySQL, MariaDB and Oracle containers) and Hibernate 5: all green. MongoDB and Neo4j still need a CI run, which `no ci` is holding back. ########## grails-doc/src/en/guide/upgrading/upgrading90x.adoc: ########## @@ -1151,3 +1151,34 @@ When upgrading an affected application: ---- See xref:theWebLayer#commandObjects[Command Objects] for a trait-based alternative to sharing a non-domain superclass with a domain class. + +==== 17. A Null in a Unique Constraint's Composite Key Follows the Database Review Comment: 9.0.x grew again since: 17 is now "Generated GORM Identity and Version Properties Stay Within Domain Classes", followed by 18 (RxGORM on RxJava 3) and 19 (RxJava 1 and 2 modules deprecated), which is why the branch conflicts on this file. After merging 9.0.x this is 20; the PR body's "section 17" too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
