jdaugherty commented on code in PR #16583:
URL: https://github.com/apache/grails-core/pull/16583#discussion_r4392716761
##########
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:
Verified: it is section 20 at cd413415c8.
##########
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:
The reference table now has the per-database list. The only piece left is
the upgrade note's Db2 bullet (new comment).
##########
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, including the corrections from round 2.
##########
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:
Verified: the entry sits next to `grails.gorm.default.nullable`, typed
`java.lang.String` with default `database`.
##########
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:
Verified: `[:]` is rejected, and `UniqueNullsSpec` has the row.
--
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]
