Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrapping of a class that defines the constraint validation rules #994

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

marko-bekhta
Copy link
Member

This PR is to address

IIRC, Gunnar had a good idea about requiring a wrapping here. It goes with the idea of wrapping the mapping name or the class.

from the #989

The wrapping class is called HibernateConstrainedType. Prior to actually wrapping the class as a "thing" that defines validation rules a few more changes were applied. I tried to make all of them as small as I could.

In this PR you would see the similar change for cascading metadata and how it is retrieved. And to answer the question from the other PR - I was thinking about the case when we would have something like

class A {
    @ValidPropertyHolder("someMapping")
    private PropHolder holder;
}

where the properties of a regular bean are property holders and I would like us not to "special case" them, but rather treat them as a regular properties marked for cascading.

If the changes in general are OK, we can also try to reduce the amount of created HibernateConstrainedTypes especially the ones in constraint location classes. Didn't want to go for it right away as it looks like it might require some more change, and the PR is getting big already 😃

I'm also still not 100% convinced that we should keep the metadata in ValueContext...

Any feedback is welcomed, especially on the naming of classes 😄

Copy link
Member

@gunnarmorling gunnarmorling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @marko-bekhta, this one caught my curiosity as I saw my name :)

Two small technical comments inline, but one more general question on the overall approach: Is HibernateConstrainedType meant to represent non-bean based types, down the road, too? If so, how would getActualType() behave in that case?

I'm very far away by now from HV and probably shouldn't bother you with my comments anyways, but then I was just curious how the free-form work goes :) Thanks a lot!

/**
* @return a class of an object that will be validated.
*/
Class<T> getActuallClass();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getActualClass()

*/
Class<T> getActuallClass();

List<HibernateConstrainedType<? super T>> getHierarchy(Filter... filters);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filter is a type from an internal package, whereas this interface isn't. That's a bit inconsistent (and there's nightly build in place which should spot this), I reckon this type should go under internal, too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, I missed this part (about Filter being an internal class).

I reckon this type should go under internal, too?

that's a good question... And the answer I suppose would be driven by the answer to the next question. How do we want the users to pass the mapping-name for free form validation? I was thinking that we would actually ask them to pass this HibernateConstrainedType as one of the parameters. Something like

<T> Set<ConstraintViolation<T>> validate(T object, HibernateConstrainedType<T> mapping, Class<?>... groups);

then we would need this class to be in the public API. But if we decide that we would just ask users for a string

<T> Set<ConstraintViolation<T>> validate(T object, String mapping, Class<?>... groups);

And later inside the method just wrap it with our own impl ... we could then move it to internal package.

HibernateConstrainedType being public interface gives us interesting opportunities. For example using this hierarchy method from it we could, in theory, build a constraints hierarchy for free form validation.

@marko-bekhta
Copy link
Member Author

Hi @gunnarmorling, thanks for the questions :)

Yes, the idea is to use this same type for the property holders (or free form validation) as well. And in that case I was thinking that we would return the class of an object that really holds the data - map, json etc.
Now that you've asked it - I looked at the final version of the code and I think we can move this method to JavaBeanConstrainedType which is an impl of this interface. Right now it is used to log some messages (we can just update the methods in LOG to use this new interface instead and add nice toString), and inside javabean specific code like in annotation based provider (and in this case we don't want to pass anything else but java beans to it). It was useful initially as there were a lot of places that would fail to compile 😃. So I'll probably apply the change that I've just described above and then we can continue looking at this PR.

@gunnarmorling
Copy link
Member

I looked at the final version of the code and I think we can move this method to JavaBeanConstrainedType which is an impl of this interface.

Yes, that'd seem reasonable.

@@ -761,14 +761,14 @@ private void validateCascadedContainerElementsInContext(Object value, BaseBeanVa
}
}

private BeanValueContext<?, Object> buildNewLocalExecutionContext(ValueContext<?, ?> valueContext, Object value) {
private BeanValueContext<?, Object> buildNewLocalExecutionContext(CascadingMetaData cascadingMetaData, ValueContext<?, ?> valueContext, Object value) {
BeanValueContext<?, Object> newValueContext;
Contracts.assertNotNull( value, "value cannot be null" );
BeanMetaData<?> beanMetaData = beanMetaDataManager.getBeanMetaData( value.getClass() );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the line

…gMetaData

As in case of property holders we would need to produce metadata not based
on a class of a current object at runtime but based on a string representing
the constraint mapping, we need to delegate this to cascading metadata as
that's the place where this information will be stored.
…s class

`HibernateConstrainedType` wraps a class and is used as a key for metadata
manager. It can also wrap strings or any other objects/data for metadata
of property holders and other kinds of objects.
Copy link

sonarcloud bot commented Oct 17, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants