jdaugherty commented on code in PR #16781:
URL: https://github.com/apache/grails-core/pull/16781#discussion_r4393103167
##########
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:
This only finds entities of the primary datastore.
`grailsApplication.mappingContext` is the `grailsDomainClassMappingContext`
bean, and `MongoDbDataStoreSpringInitializer` registers that alias only when
MongoDB is not a secondary datastore. A MongoDB entity in a Hibernate app isn't
found here, so it binds correctly but still evaluates its constraints on every
bind. The same is true of an entity annotated `@Validated`, whose validator is
a `GormValidatorAdapter`.
Is it OK to leave these cases for a follow-up issue?
##########
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:
No test covers either fallback guard. I removed both `entity.getJavaClass()
!= type` and the `validator == null` check below, and `DataBindingUtilsSpec`,
`GrailsWebDataBinderSpec` and `DomainProxyBindingSpec` all still pass.
Could you add a test for each?
- A mapped entity that has no validator, for example a
`KeyValueMappingContext` with `addPersistentEntity` and no
`addEntityValidator`, should bind with the constraints its class declares.
Without the guard, `validator.getClass()` throws a `NullPointerException`,
which nothing here catches, so the bind fails.
- An object whose class name resolves to an entity that isn't its class.
This happens after a class is reloaded. It also happens for any class whose
name contains `_$$_`, because `getPersistentEntity(String)` cuts the name at
that marker. A class named something like `MappedEntity_$$_Probe`, which the
proxy handler doesn't recognise and whose own constraints differ from the
validator's, shows which set was used.
##########
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:
There's already an SPI for reading an entity's constraints:
`org.grails.validation.discovery.ConstrainedDiscovery` in grails-core. Its
default implementation, `DefaultConstrainedDiscovery` in grails-validation,
reads the same validator but checks `instanceof ConstrainedEntity`. Looking up
a property named `constrainedProperties` finds `PersistentEntityValidator`
today, but that name isn't a contract. Any validator with a `Map` property of
that name would be read as constraints.
Can binding go through `ConstrainedDiscovery`, loaded once rather than on
every bind? It wraps each property in a new `ConstrainedDelegate` on every
call, so it would need measuring against the numbers in the description. If
that costs too much, please add a comment here saying so, so the next reader
knows this copy of the lookup is deliberate.
--
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]
