HV-1363 support for non-standard Java beans

[marko-bekhta]

• https://hibernate.atlassian.net/browse/HV-1363

I think with an opened PR it'll be easier to discuss this. What I've done so far is next:

• First I wanted to remove the dependency on reflection in ConstraintLocation and that resulted in the bigger part of the changes in this PR.
• After that change the fail fast tests started to behave randomly in case there are two constraints on the same parameter. That's why the last commit was needed.
• And probably the most relevant part - I've moved out all the logic related to detecting getters to a separate class - PropertyFilter. And that actually uncovered a couple of new challenges.

Having simple filtering for property methods seems to be not enough. I've extracted all the things that we do with the getters to this interface:

public interface PropertyFilter {
    boolean isProperty(Executable executable);
    String getPropertyName(Method method);
    Set<String> getPossibleMethodNamesForProperty(String propertyName);
}

Why such methods?

• we need to determine if something is a property or not
• if it is a property we need to extract the name part from it
• and in case of XML or programmatic configuration we receive the name of the property and we need to construct back the full name to find a corresponding executable.

Next step that I was planning to do was to continue with the approach suggested by @gunnarmorling in the previous PR for this improvement, i.e. having a PropertySelector and refactor AnnotationMetaDataProvider to use it.

public interface PropertySelector<T extends ConstrainableType> {
    Stream<Property> getProperties(T type);
}

But actually extracting the getter logic into PropertyFilter uncovered the other question. In case of XML or programmatic configuration we need to have such filter right away, when we read xml or build programmatic constraints. And it would mean that we cannot expose the PropertyFilter interface as a configuration option on HibernateValidatorContext. As exposing it there would allow users to change the filter after the XML config was read. (the similar problem to what we had when we tried to cache the parameter name in the constraint location).
So the question is - do we want to expose this configuration through the context or not ? Seems like exposing it would require to delay the metadata creation from XML and programmatic configuration to the creation of Validator ?

[gsmet]

So the question is - do we want to expose this configuration through the context or not ?

Quick answer for this one: I don't think it's a good idea to do so. It makes sense for it to be a ValidatorFactory only setting.

[marko-bekhta]

@gsmet even though we expose this "property selection functionality" only through the HibernateValidatorConfiguration we still would have a "problem", as it's at the same level where we add constraint mappings so if user adds mappings and then change the propertyFilter it most likely would not end good. I've added a few warnings here, as I don't see a nice solution for now. Unless we want to just collect the info here and delay all the work until later time, when building the factory.
Other than that I think you can give it a first look, when you have time for it 😃.

[gsmet]

Hmmm, it's the second time we hit this wall (we already had this issue when we discussed resolving the parameter names once and for all - it's a bit different here though as we don't have the HibernateValidatorContext issue). Taking a step back, I think it was a pretty bad decision to allow to define the mappings during the configuration phase. It should have been a separate step.

Wondering if we should revisit this idea of postponing the mappings creation to the factory building phase.

But it's something we can do after getting this patch in, if we think it makes sense.

[gsmet]

Woot, that is some massive work :). Thanks for taking care of it.

I did review commit per commit so sorry if some comments gets fixed afterwards, I think most of them aren't and are still valid.

I have 2 major questions here:

• We had this issue reported in the forum recently: https://discourse.hibernate.org/t/primefaces-chips-web-component-sizevalidatorforcharsequence/374 . I would like us to fix it at some point. I.e. be able to say that the getter type is different from the property type. The OP's case is rather convoluted but think about Optional on the getter and you can see the point. I know it's a new requirement but as we are mostly rewriting everything here, I would like to make sure we take this into consideration and it works as expected.
• I think we should try to prototype a JSON validator to see if we can get something working with what you designed. We don't need a full blown solution (having it is a totally different issue and will need much more work) but just making sure the new elements are generic enough to cover this case.

[gsmet]

It's missing the formatter.

[gsmet]

It's missing the formatter.

[gsmet]

Let's reformat this when we change the parameters. It's really annoying when reviewing.

[gsmet]

I would name the package javabean instead of java.beans.

[gsmet]

s/executable/callable/

I think this should be moved inside the Callable implementations.

[gsmet]

Let's test a real use case with the getters being generated in a fluid way (i.e. name() for the name property). This is the most common use case people ask about.

[gsmet]

Why don't you use the DefaultPropertyFilter here instead of having another class?

[gsmet]

Commit message should be fixed: s/re/are/

[gsmet]

Maybe drop the With part? You're returning a MethodHandle.

[gsmet]

@marko-bekhta looks like you forgot this one.

[gsmet]

That being said, this commit should probably be squashed with the one it fixes.

[gsmet]

Is it normal that there's still a TODO here?

[marko-bekhta]

it can be removed now, will clean it up.

[gsmet]

@marko-bekhta looks like we have some issues with Karaf. Do you reproduce the failures on your laptop? If so, taking a look at the karaf log file might help.

[marko-bekhta]

@gsmet I have the same strange behavior as in the stax PR. I did mvn clean install before pushing the code and the build passed for all modules. But then there are these failures on CI. I made a couple more small changes after that but haven't pushed them yet as wanted to figure out what's happening with the osgi tests...

[marko-bekhta]

forgot to post all the other comments, and as it looks a simple rebase solved the problem 😃

[marko-bekhta]

yes :)

[marko-bekhta]

yes, it's a leftover prior to Type->ConstrainableType change

[marko-bekhta]

ahh, I think I've removed it completely in next commit...

[marko-bekhta]

The idea was to reduce the amount of possible cases that must be covered later. Field property and Getter property would still both get a "property compatible" (something implements Property) parameter

[marko-bekhta]

It was just to make the project compile, at this point it is used in the CDI module, but it is replaced in later commits and this thing removed.

[marko-bekhta]

thanks for spotting this. I've reused this array of methoda to run stream once for getters and then for other methods, but then they got combined but I've forgot about the array.

[marko-bekhta]

We could have a getter in JavaBeanExecutable but I thought that it's better to make it "harder" to get to that field so that it won't get called somewhere else. What I also thought about was to actually move the work with annotations to the JavaBean* classes so that we don't expose any reflection outside our wrappers. But that looked like much work and I thought it would be better to work on it later if we decide it is a good way to go.

[marko-bekhta]

it can be removed now, will clean it up.

[marko-bekhta]

As for the first question:

We had this issue reported in the forum recently: https://discourse.hibernate.org/t/primefaces-chips-web-component-sizevalidatorforcharsequence/374 . I would like us to fix it at some point. I.e. be able to say that the getter type is different from the property type. ....

With the current implementation the next test pass:

@Test
public void testGetterPropertyMatcher() {
    Validator validator = Validation.byProvider( HibernateValidator.class )
            .configure()
            .getterPropertyMatcher( new BarGetterPropertyMatcher() )
            .buildValidatorFactory()
            .getValidator();
    Set<ConstraintViolation<Bar>> violations = validator.validate( new Bar() );

    ConstraintViolationAssert.assertThat( violations ).containsOnlyViolations(
            violationOf( AssertTrue.class ).withProperty( "bar" ),
            violationOf( NotBlank.class ).withProperty( "bar" ),
            violationOf( NotNull.class ).withProperty( "bar" ),
            violationOf( Min.class ).withProperty( "bar" ),

            violationOf( NotEmpty.class ).withProperty( "tags" ),
            violationOf( Size.class ).withProperty( "tags" )
    );
}

private static class Bar {

    @Min( 10 )
    private final int bar = 1;

    @Size(min = 10, max = 2147483647)
    private final String tags = "";

    @NotEmpty
    public List<String> getTags() {
        return Arrays.stream( tags.split( "," ) ).filter( tag -> !tag.trim().isEmpty() )
                .collect( Collectors.toList() );
    }

    @AssertTrue
    boolean isBar() {
        return false;
    }

    @NotBlank
    String getBar(){
        return "";
    }

    @NotNull
    Set<String> bar(){
        return null;
    }
}

public static class BarGetterPropertyMatcher implements GetterPropertyMatcher {

    @Override
    public boolean isProperty(ConstrainableExecutable executable) {
        return executable.getParameterTypes().length == 0;
    }

    @Override
    public String getPropertyName(ConstrainableExecutable method) {
        String name = method.getName();
        char[] chars = name.substring( name.startsWith( "is" ) ? 2 : name.startsWith( "get" ) ? 3 : 0 ).toCharArray();
        chars[0] = Character.toLowerCase( chars[0] );
        return new String( chars );
    }
}

we have a GetterPropertyMatcher that would take any method without parameters as getter and extract the name without a prefix.
So if I understood the problem correctly - it is already solved, right?

[gsmet]

@marko-bekhta yes looks good. It would be nice if we could add a test (not as convoluted as your example, just with the String/List and the default GetterPropertyMatcher).

[marko-bekhta]

@gsmet done :) - https://github.com/marko-bekhta/hibernate-validator/blob/9a8c7a4b68392c3132edd18f4b34f6ca4ca42a78/engine/src/test/java/org/hibernate/validator/test/properties/GetterPropertyMatcherTest.java#L100

and about the second question ... Should it be something like javax.json.JsonObject or just a string, or something like Map<String, Object> ?

[gsmet]

I think we could try to experiment with the new javax.json and see how it goes. But yeah, it's the same with a Map. Our ultimate goal is to be able to validate free form objects.

The idea would be to see if we could validate something without adding new stuff to the HV engine proper.

[marko-bekhta]

Hi @gsmet I've tried a simple example with javax.json and here's a commit with my experiment - marko-bekhta@020e49c
I didn't want to push it here to keep this PR cleaner. What I've done there is:

• Added a "mocked" version of JsonMetadataProvider which returns a couple of constraints for properties like name and email.
• Added JsonProperty and JsonConstrainableType which extend from the property API that we have. Note that I've hardcoded the list of possible properties into JsonConstrainableType but I real case I think we wouldn't need that at all and we would just programmatically define some constraints (probably) without any validation if the property is present or not ...
• And there's this simple test to check how it would work:

@Test
public void testValidatePropertyWithRedefinedDefaultGroupOnMainEntity() {
    Validator validator = getValidator();
    JsonObject jsonObject = Json.createObjectBuilder()
            .add( "firstName", "name" )
            .add( "lastName", "" )
            .add( "age", BigDecimal.valueOf( 20 ) )
            .add( "email", "not-an-email-string" )
            .build();
    Set<ConstraintViolation<JsonObject>> constraintViolations = validator.validate( jsonObject );
    assertThat( constraintViolations ).containsOnlyViolations(
            violationOf( Email.class ).withProperty( "email" ),
            violationOf( NotBlank.class ).withProperty( "lastName" )
    );
}

But here are some questions I didn't find the answers for right away and I'm not sure how we would like to do it:

• How a user would define constraints for Json objects ? I thought of a possibility to do it with something similar to our programmatic API. But there's the next question:
• Would it mean that we could define only one set of global constraints for json properties ?

[gsmet]

Agreed to not push this here. We should probably even move this discussion to another PR.

Yes, I was thinking about having some sort of programmatic API. TBH, what would have been nice would be to be able to use the existing API but I don't think it will work.

The type could be some sort of key to define a JSON schema (e.g. that would answer your question about having only a global set of constraints, you would define different types and it would be a parameter of your validate() call).

What I'm not very clear about is how you would define the metadata for nested properties. You either need a way to define the path of the property or you would use some sort of cascading mechanism and you would need an API to set the type of a nested object so that it properly cascades.

Another thing is how we would define the metadata, I suppose we would have to build them for every validation call as the JSON information might be different (types for instance) and I'm not sure we would like to force the user to define the whole schema of his JSON content.

@yrodiere have you already thought about it as part of Search 6?

[marko-bekhta]

@gsmet I've created another PR for json discussion. #949

[yrodiere]

@yrodiere have you already thought about it as part of Search 6?

@gsmet If you're talking about non-standard Java beans, I did. Essentially I added an abstraction layer so that the core of the mapper implementation does not care about how properties are accessed.

• PojoTypeModel allows to retrieve properties by name or as a Stream
• PojoPropertyModel gives access to the property metadata and to the property handle
• PropertyHandle allows to get the property value at runtime

One thing that is missing is the ability to explicitly select the access type: getter or field. For now in the Hibernate ORM mapper, I comply with the JPA Access Type, and in the JavaBeans mapper, I use getters (because that's the rule with standard JavaBeans). But we should probably offer some way of selecting the access type explicitly.

Chains of properties are not modeled as part of this SPI, though, because they can be implemented independently from how properties are accessed. I added such model as BoundPojoModelPath and its various subtypes (BoundPojoModelPathOriginalTypeNode, BoundPojoModelPathPropertyNode and BoundPojoModelPathValueNode in particular) in my most recent PR.

If you were talking about JSON... Well, I took great care to allow the introduction of mappers that are not targeting POJOs, but target something else such as JSON or Maps. I didn't do much more. But I suspect we won't be able to expose the same APIs/SPIs, because the requirements are different:

• Java annotations make no sense
• the access type (getter/field) makes no sense
• even random access to properties might not make sense: we might want to handle JSON as a stream of data, keeping the memory footprint minimal.

I think the best way to solve all this is to try to implement such a mapper, and see what we can re-use from the POJO mapper. We might be able to move a subset of it to a common project that both the POJO mapper and the JSON mapper would depend on.

[gsmet]

@marko-bekhta could you rebase this one? I plan to make a last pass on it and discuss with Gunnar about the API names when he's back.

Thanks!

[marko-bekhta]

rebased and ready.

[gsmet]

So... my idea for this PR is to try to commit the first part with the abstraction really soon and then iterate on the rest after that.

I think the abstraction will be helpful for your JSON work so we'd better get it in soon and as far as possible right :).

• I squashed 2 commits that shouldn't have been separated and also pushed an additional fixup commit with
  • a few fixes
  • a few things you committed afterwards fixed the build of this commit so I made the tests work for this commit by backporting things from later commits
• as for the rest, I added quite a lot of comments/questions about the abstraction and what it should be. Maybe you have also some more insights from your work on the JSON support.

I would say let's not rush into fixing things and take a step back to get this first commit right.

Note that most of my questions are really questions so there might be good reasons for doing it the way you did or not going over the top.

Maybe we should have a chat once you have digested all my comments.

PS: I added a name suggestion about the matcher to get it out of my head. Let's not change anything on this front for now, I want us to focus on the very first commit.

[gsmet]

I'm a bit surprised we don't also have an abstraction on ElementType. I think it's going to be in the way.

It's pretty obvious from the way we have to special case things whereas the ElementType could be returned by the Property object or the Callable in the case of a method/constructor.

Maybe we could call it ConstraintLocationType.

AFAICS, it's only exposed in BV in the TraversableResolver (and considering it only applies to Java Bean properties (either fields or getters), we could check that the ConstraintLocationType is valid and translate it then call a getElementType() method which would be present on JavaBeanField and JavaBeanGetter) and in ConstraintFinder (we probably could have a translation layer there too).

Did you think about it?

[gsmet]

Note: I don't have your vision about the full patch/future developments so these are real questions.

Is this really what we want to do or are we talking about JavaBeanExecutable?

I mean do we expect these things to be compatible with anything else than JavaBeanExecutable or is it specific to it?

Considering we are introducing the difference between Method and Constructor which is not present in Callable, I'm thinking we are talking about Java beans here.

To be honest, I'm wondering if we should introduce Method and Constructor as subinterfaces of Callable too.

Also wondering if we should create different objects for JavaBeanMethod and JavaBeanConstructor (which could inherit from an AbstractJavaBeanExecutable). They seem to be different beasts so I would have different objects for them (same as Getter and Field).

[marko-bekhta]

I couldn't think of any other kind of executables/callables at this point. But the idea was to not limit ourselves with something specific if the more generic thing could be used.

I'll get back to the Method/Constructor part after I'll go through all the other comments and remind myself of what's happening :)

[gsmet]

Why do you have property instanceof JavaBeanField ? ElementType.FIELD : ElementType.METHOD in ConfiguredConstraint.forProperty() whereas you don't call it here. I would expect us to call ConfiguredConstraint.forProperty() for a getter given how you separated everything. At least, there's something inconsistent here.

[marko-bekhta]

I don't like this part either. But it's a part of a bigger problem that I've initially hit. The thing is that getter-property should also be treated as an executable. These getter-executables are later converted to both executable and properties in https://github.com/marko-bekhta/hibernate-validator/blob/9b26462ecf0c890add80bee8dcc50f6ccb4ae983/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/BeanMetaDataImpl.java#L723

So that check property instanceof JavaBeanField ? ElementType.FIELD : ElementType.METHOD in ConfiguredConstraint#forProperty() could be removed completely.

[gsmet]

Don't you have a ConstrainedProperty.forGetter()?

[marko-bekhta]

I had it initially but dropped it later. See the comment about getter-treated-as-executable

[gsmet]

Still thinking that it would make sense to separate JavaBeanConstructor and JavaBeanMethod and not downcast them so fast.

[gsmet]

I think you could avoid passing the ElementType here. This to reduce our exposition to it.

[marko-bekhta]

this one is later used in buildConstraintDescriptor() so cannot do much about it. But maybe what you were thinking about the ConstraintLocationType would help here.

[gsmet]

So we don't propagate the abstraction to TypeArgumentFieldLocation? I can see how it could be tied to a Java bean but if it were possible, I would appreciate us to push the abstraction as far as possible.

Note that it might not be a good idea, feel free to explain why it's not a good idea.

[marko-bekhta]

I thought about it, but as all this TypeArgumentLocation hierarchy is private to AnnotationMetaDataProvider and we later just build locations from the instances I think it is ok as it is. This way we will not create additional objects.

[gsmet]

You haven't answered this one. Did you change the code or is my concern misplaced?

[gsmet]

You should break; from here. Otherwise if you have an isgethas(), you will have some issues (agreed it's not recommended but let's be thorough).

[gsmet]

I know I already made you change the name but I'm wondering if GetterPropertySelectionStrategy would be better?

[marko-bekhta]

I think I replied to the comments and I have a different branch with a few additional things applied to "abstraction". I wasn't sure where to put those changes, probably best to open a new PR?

[marko-bekhta]

I don't like this part either. But it's a part of a bigger problem that I've initially hit. The thing is that getter-property should also be treated as an executable. These getter-executables are later converted to both executable and properties in https://github.com/marko-bekhta/hibernate-validator/blob/9b26462ecf0c890add80bee8dcc50f6ccb4ae983/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/BeanMetaDataImpl.java#L723

So that check property instanceof JavaBeanField ? ElementType.FIELD : ElementType.METHOD in ConfiguredConstraint#forProperty() could be removed completely.

[marko-bekhta]

I couldn't think of any other kind of executables/callables at this point. But the idea was to not limit ourselves with something specific if the more generic thing could be used.

I'll get back to the Method/Constructor part after I'll go through all the other comments and remind myself of what's happening :)

[marko-bekhta]

I had it initially but dropped it later. See the comment about getter-treated-as-executable

[marko-bekhta]

Even though Constrainable is enough Property would better show with what are we dealing with. Changed.

[marko-bekhta]

hmm... I'll think about this. But with the current state of things, you need to somehow get to the property from ConstrainedElemet first, to be able to get its name.

[marko-bekhta]

It does feel strange. But the thing is that we need property to make the annotationProcessingOptions.areMemberConstraintsIgnoredFor(property) check just a few lines above in a method calling this one, and I didn't want to create properties multiple times. And we need to pass a field as we need to get annotation information from it later.

I was thinking about moving this part into Property itself (i.e. the annotations retrieval) . But we cannot expose annotations from a property, and adding abstraction over them as well didn't make sense as it wouldn't apply to Map or JSON.

[marko-bekhta]

this one is later used in buildConstraintDescriptor() so cannot do much about it. But maybe what you were thinking about the ConstraintLocationType would help here.

[marko-bekhta]

I thought about it, but as all this TypeArgumentLocation hierarchy is private to AnnotationMetaDataProvider and we later just build locations from the instances I think it is ok as it is. This way we will not create additional objects.

[marko-bekhta]

Thanks 👍 will keep my eyes opened for these :)

[marko-bekhta]

+1 on this. Will be done!

[gsmet]

@marko-bekhta let's close this one as it's barely readable now with all those comments, most of them related to the abstraction.

We can rebase and open a new (smaller) one.

Two comments are still valid though:

• HV-1363 support for non-standard Java beans #938 (comment)
• HV-1363 support for non-standard Java beans #938 (comment)