jdaugherty commented on PR #16474:
URL: https://github.com/apache/grails-core/pull/16474#issuecomment-5953403785
1. No test covers the path from the issue. In a running app, a command
object gets its evaluator from DefaultConstraintEvaluatorFactoryBean, which
reads the setting from the application config. The PR's command-object test in
ImportFromSpec calls DefaultValidatorRegistry.evaluate(...) instead, which
reads the setting from the datastore settings. The fix itself is still tested,
but not the config wiring a user actually hits. A small integration spec in
grails-test-examples/gorm would cover it, since that app already sets nullable:
false and has a ValidateableSpec. My probe there needed @Rollback.
2. Doc wording is imprecise. This is in all three doc changes
(sharingConstraints.adoc, Constraints.adoc, upgrading80x.adoc):
- "leaves it unconstrained" is narrower than what happens. The default
applies to any property that has no nullable constraint. In my probe, a
property with only maxSize: 5 was also made required. Suggested wording: "a
property for which the source declares no nullable constraint".
- "unless grails.gorm.default.nullable is set to false" leaves out the
wildcard form '*'(nullable: false), which also makes these properties required
(and already did before this PR). In the upgrade guide, "unless you restore the
required-by-default behaviour (below)" would cover both.
--
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]
