jdaugherty commented on code in PR #16583:
URL: https://github.com/apache/grails-core/pull/16583#discussion_r4348899041
##########
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:
@c162.txt
##########
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:
Hibernate generates the composite unique index from this same constraint, so
under the value reading the validator disagrees with the index it produced on
PostgreSQL, MySQL, MariaDB and H2, and agrees with it on Oracle and SQL Server.
The skip reading is the reverse. That is the main reason both modes need to
exist. Rather than "most databases", say which databases match which mode so a
reader can pick the one that agrees with their index.
##########
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:
With the toggle this paragraph should describe the modes and the default,
and say how a `null` value of the constrained property itself is treated
(skipped today), so the whole rule for a composite key is in one place.
##########
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:
Once the modes exist, run these shapes under each one. The `Folder` and
`Translation` shapes from the issue thread are where the modes disagree, so
they make the clearest table. Please also add the case where the constrained
property itself is null, so that column's rule is pinned deliberately rather
than inherited from `skipNullValues()`.
##########
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:
This sentence settles the question #14503 left open. With the toggle, the
note should instead list the modes, name the default and show how to keep the
previous behaviour. The replacement suggested on the next line is not
equivalent either: `name unique: true` makes the name unique across every
parent, while today's behaviour only does that for rows without a parent.
##########
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 already has sections 1 to 16 here (XML web support, Spring JSON
conversion and the rest landed after this branch was cut), so this becomes a
second `5.` when merged. Renumber after merging 9.0.x in; as of today it is 17.
##########
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:
Whichever default we pick, the constrained property itself still follows the
skip rule: `AbstractConstraint.validate` returns before `processValidate` runs
when the value is null, since `skipNullValues()` is true. After this change
`(null, 'US')` twice passes validation while `('en', null)` twice fails, so one
composite key has two different null rules depending on which column is null.
The toggle should govern both columns the same way, or the docs have to explain
why they differ.
--
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]
