hk2166 commented on code in PR #16781:
URL: https://github.com/apache/grails-core/pull/16781#discussion_r4393326219
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -163,6 +167,56 @@ private static Class bindingType(final Object object) {
return object.getClass();
}
+ private static Class persistentClass(final Object object, final
MappingContext mappingContext) {
+ final ProxyHandler proxyHandler = mappingContext.getProxyHandler();
+ if (proxyHandler != null && proxyHandler.isProxy(object)) {
+ final Class proxiedClass = proxyHandler.getProxiedClass(object);
+ if (proxiedClass != null) {
+ return proxiedClass;
+ }
+ }
+ return object.getClass();
+ }
+
+ /**
+ * The constraints of the persistent entity an object is an instance of,
or a proxy of. The mapping context
+ * evaluates them once, for the validator of the entity, and keeps them
for as long as the entity is mapped,
+ * so binding reads them there rather than evaluating the {@code
constraints} closure of the class again.
+ *
+ * @return the constraints, or {@code null} when the object is not a
mapped entity or its validator does not
+ * expose its constraints
+ */
+ private static Map getEntityConstrainedProperties(final Object object,
final GrailsApplication application) {
+ if (application == null) {
+ return null;
+ }
+ try {
+ final MappingContext mappingContext =
application.getMappingContext();
+ if (mappingContext == null) {
+ return null;
+ }
+ final Class type = persistentClass(object, mappingContext);
+ final PersistentEntity entity =
mappingContext.getPersistentEntity(type.getName());
+ if (entity == null || entity.getJavaClass() != type) {
Review Comment:
Added both in a new `EntityConstraintsBindingSpec` in
grails-test-suite-persistence. The discovery from your other comment needs
grails-validation on the classpath, which grails-web-databinding's own tests
don't have.
- `an entity without a validator is bound with the constraints its class
declares`: a `KeyValueMappingContext` with `addPersistentEntity` and no
`addEntityValidator`. Through discovery, a missing validator comes back as no
constraints rather than as a `null`. So the guard that matters now is the
fallback when discovery finds none, and removing it fails this test.
- `a class whose name resolves to the entity of another class is bound with
the constraints it declares`: `ValidatedRecord_$$_Probe` constrains `name` with
`bindable: false`, while the validator of the `ValidatedRecord` entity
constrains `secret`. Removing the `getJavaClass() != type` check fails this
test.
I checked both by removing each guard and running the spec.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -163,6 +167,56 @@ private static Class bindingType(final Object object) {
return object.getClass();
}
+ private static Class persistentClass(final Object object, final
MappingContext mappingContext) {
+ final ProxyHandler proxyHandler = mappingContext.getProxyHandler();
+ if (proxyHandler != null && proxyHandler.isProxy(object)) {
+ final Class proxiedClass = proxyHandler.getProxiedClass(object);
+ if (proxiedClass != null) {
+ return proxiedClass;
+ }
+ }
+ return object.getClass();
+ }
+
+ /**
+ * The constraints of the persistent entity an object is an instance of,
or a proxy of. The mapping context
+ * evaluates them once, for the validator of the entity, and keeps them
for as long as the entity is mapped,
+ * so binding reads them there rather than evaluating the {@code
constraints} closure of the class again.
+ *
+ * @return the constraints, or {@code null} when the object is not a
mapped entity or its validator does not
+ * expose its constraints
+ */
+ private static Map getEntityConstrainedProperties(final Object object,
final GrailsApplication application) {
+ if (application == null) {
+ return null;
+ }
+ try {
+ final MappingContext mappingContext =
application.getMappingContext();
+ if (mappingContext == null) {
+ return null;
+ }
+ final Class type = persistentClass(object, mappingContext);
+ final PersistentEntity entity =
mappingContext.getPersistentEntity(type.getName());
+ if (entity == null || entity.getJavaClass() != type) {
+ return null;
+ }
+ final Validator validator =
mappingContext.getEntityValidator(entity);
+ if (validator == null) {
+ return null;
+ }
+ final MetaClass metaClass =
GroovySystem.getMetaClassRegistry().getMetaClass(validator.getClass());
+ final MetaProperty constrainedProperties =
metaClass.hasProperty(validator, CONSTRAINED_PROPERTIES);
Review Comment:
Binding now goes through `ConstrainedDiscovery`, which a holder class loads
the first time binding needs it. I measured it with the same benchmark as the
description: `DataTest`, 24 constraints, 4 properties bound. These are µs per
call, the median of the last 3 of 5 rounds:
| | 8.0.x | Reading the validator's map | Through `ConstrainedDiscovery` |
|---|---:|---:|---:|
| `new Book(map)` | 76 | 12 | 18 |
| `bindObjectToInstance` | 83 | 13 | 17 |
Discovery costs about 4 to 5 µs a bind. That covers the new
`ConstrainedDelegate` for each property, plus the extra lookups
`bindableConstraintValue` makes to unwrap each one. I kept discovery for the
contract, and left a comment with these numbers at the call. If you'd rather
have the direct read, I'll switch back and say there why it's deliberate.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -163,6 +167,56 @@ private static Class bindingType(final Object object) {
return object.getClass();
}
+ private static Class persistentClass(final Object object, final
MappingContext mappingContext) {
+ final ProxyHandler proxyHandler = mappingContext.getProxyHandler();
+ if (proxyHandler != null && proxyHandler.isProxy(object)) {
+ final Class proxiedClass = proxyHandler.getProxiedClass(object);
+ if (proxiedClass != null) {
+ return proxiedClass;
+ }
+ }
+ return object.getClass();
+ }
+
+ /**
+ * The constraints of the persistent entity an object is an instance of,
or a proxy of. The mapping context
+ * evaluates them once, for the validator of the entity, and keeps them
for as long as the entity is mapped,
+ * so binding reads them there rather than evaluating the {@code
constraints} closure of the class again.
+ *
+ * @return the constraints, or {@code null} when the object is not a
mapped entity or its validator does not
+ * expose its constraints
+ */
+ private static Map getEntityConstrainedProperties(final Object object,
final GrailsApplication application) {
+ if (application == null) {
+ return null;
+ }
+ try {
+ final MappingContext mappingContext =
application.getMappingContext();
Review Comment:
Yes, I'd leave them for a follow-up. Both still bind correctly:
- A MongoDB entity in a Hibernate app isn't in
`grailsApplication.mappingContext`, so it falls back to evaluating its closure,
as on 8.0.x.
- Through discovery, the `GormValidatorAdapter` of an `@Validated` entity
comes back as no constraints, so it falls back the same way.
Reaching the datastore an entity belongs to would need something like
`GormEnhancer.findEntity`. That lives in grails-datamapping-core, which
grails-web-databinding doesn't depend on today. I can open the follow-up issue
once this PR is settled.
--
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]
