HV-1552 Adding new MinAge Constraint

[HillmerCh]

This PR has the changes suggested for you

[marko-bekhta]

Hi @Hilmerc ! Sorry for the late reply. It looks we are on the right track with this one. I've noticed that the build failed : http://ci.hibernate.org/job/hibernate-validator-PR/1042/org.hibernate.validator$hibernate-validator/console and it looks like you haven't included a programatic definition file:

[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /var/lib/jenkins/workspace/hibernate-validator-PR/engine/src/test/java/org/hibernate/validator/test/internal/constraintvalidators/hv/AgeValidatorTest.java:[26,40] cannot find symbol
  symbol:   class AgeMinDef
  location: package org.hibernate.validator.cfg.defs
[INFO] 1 error

Could you push it as well (and any other changes if there are such 😉 )?

Also it's OK to keep the PRs opened and add new stuff to them. This way it's easier to track the changes and have discussions in one place.

[HillmerCh]

Hi @marko-bekhta , you right it was my mistake, I have pulled AgeMinDef, Could you check please?

[marko-bekhta]

@Hilmerc so about the plan that I've mentioned... It looks we are somewhere between first and second steps of adding a new constraint. Here's a more detailed version of what I think we should do next:

1. Before we go further we need to get the constraints and their parameters right. This way it'll be easier and will require less changes in the next steps. 1.1. We need to add unit attribute to @AgeMin (ChronoUnit unit() default ChronoUnit.YEARS;) 1.2. Update existing validator's logic to use this new attribute in both initialization and validation. 1.3. Pull out initialization and common validation logic from existing validator into abstract/base one, which will be used as a base to implement validators for other date/time types. (it'll be somehow similar to what we already have for @Past/@Future validators). 1.4. Add implementation for data types we would like this constraint to support. I guess we could have the same list of types that we have for Past/Future constraints (JSR-310 types, java.util Calendar/Date ...). 1.5. Having this in place it should be easy to add @AgeMax constraint following the same steps. 1.6. We would need to add a few more tests for other supported data types. 1.7. Also as mentioned in the other comments, we will also need to add a test extending form ConstrainedTest for both new constraints.
2. Based on the above work we could update programmatic constraint definition (AgeMinDef) and add the corresponding AgeMaxDef for @AgeMax.
3. After we are done with the constraints, and are confident that all looks good, we can move forward and add Annotation Processor support. It's really easy to do. Just need to add two lines, one for a constraint, to TypeNames.HibernateValidatorTypes and register them in ConstraintHelper.
4. Then we should also mention new constraints in documentation. In our case it should be easy to do. Just need to add something like:

`@AgeMin`:: Checks that the annotated date/time property is at least `value` `unit`s old. `unit` determines the type of `ChronoUnit` to be used to compare age. The default is ChronoUnit.YEAR.
    Supported data types::: `CharSequence`
    Hibernate metadata impact::: None

We can come up with a better wording for documentation when we reach this step.

What do you think of such plan? Please let us know if you need more details or any help with any of these steps.

[HillmerCh]

@marko-bekhta I am working with the plan, for the moment no doubts

[HillmerCh]

@marko-bekhta I have finished the step number 1, abstract classes, and implementation for LocalDate and Calendar, before I follow next step, Could you look over the classes? Especially I want to hear your thoughts about AgeMinValidatorForCalendar.

Thanks

[HillmerCh]

@marko-bekhta I think I have finished with AgeMin constraints. Could you look over one more time please, before I start with MaxAge.

I have an doubt about which date types support: As this constraint works with a dates I think that support for time types as LocalTime is not necessary.

[HillmerCh]

Hi @marko-bekhta , I made some changes 'cause I think the idea to define the units is a good idea. I created an Enum AgeMin#Unit this Enum support only YEAR, MONTHS and DAYS using only these 3 units some of the problems that we got before can be fixed. I think unit as seconds, minutes are not necessary

enum Unit {
	YEARS( ChronoUnit.YEARS ), MONTHS( ChronoUnit.MONTHS ), DAYS( ChronoUnit.DAYS );

I haven't unified the specific validator class as you told me, I think it is much better compares the same data type: LocalDate.compareTo(otherLocalDate) , using specific validator we can support time types as Year. Please look over AgeMinValidatorForYear to see what I did.

[marko-bekhta]

@Hilmerc that's great. Thanks for experimenting more with this! I'll try to have a look later today.

[marko-bekhta]

Hi @HillmerCh ! Finally had a chance to spent some time on this one. I really liked the idea of limiting the units to a specified set that we can control. I've noticed that there were quite a few formatting changes with the last commit. The checkstyle seems to be happy with those, so just in case you haven't seen there's https://github.com/hibernate/hibernate-ide-codestyles repository that could make formatting code in the IDE easier. Now as for the changes, the idea of own enum Unit is great. If I remember correctly, the trouble with the truncateTo attribute was that we could only use units up to ChronoUnit.DAYS included. So couldn't we apply the same approach with our own enum to that attribute and limit the options to ChronoUnit.NANOS - ChronoUnit.DAYS ? I'd say having two of such enums could be useful. First one would limit the options of which units can be used to define the age (I assume that we could ignore MILLENNIA, ERAS and FOREVER). Second one as mentioned earlier would limit the units used for truncateTo attribute. As for the unified validator - it just seemed more practical to have the logic of adding units in one place. In case of a Year wouldn't year.adjustInto( LocalDateTime.now(clockReference) ) do the trick of getting the LocalDateTime and then working with it in the abstract validator ?

[HillmerCh]

Hi @marko-bekhta I was a little busy the last days, I'm gonna work with this PR and try to finish next week. Thanks for your help and patience.

[HillmerCh]

@marko-bekhta I'm not sure if I understand well you said. About UNITs: You suggest to use only NANOS and DAYS, to use the unit between them? SECOND, MINUTES. If you just mean it is use only NANOS and DAYS, it can be difficult to the developer to express the values, for example, if we want to say that the de age must be 18 years, Do I have to set the value 18*365? But some years have 366 days (February 29th). About: year.adjustInto For this example: the age must be maximum 1 year old. The user input the value 14/04/2017 (From today it is one year ago). The method. year.adjustInto returns 2018-04-14T09:50:19.487032 as a LocalDateTime. Comparing the two LocalDateTime, the second is greater than the first, but someone who was born on 14/04/2017 is one year old until 13/04/2019. To avoid that kind of problem, I think it is much better compares same types. But, As I said I don't know if I understand your recommendations. Please, Can you say me if I am right o give more information. Thanks,

Hillmer

[marko-bekhta]

Hi @HillmerCh no worries 😃. IIRC I was thinking that as we discarded the idea of truncateTo attribute because it was causing the very same issues with the date/time classes, where we couldn't be sure if the exception will or will not be thrown, we might want to re-consider it once more and limit the units of it. So basically have something like:

public @interface AgeMin {
    // ....
    int value();
    
    AgeUnit ageUnit() default AgeUnit.YEARS;
    
    TruncateUnit trunkateTo() default TruncateUnit.NANOS;
    
    boolean inclusive() default true;
}

enum AgeUnit {
    YEARS( ChronoUnit.YEARS ), MONTHS( ChronoUnit.MONTHS ), DAYS( ChronoUnit.DAYS ) ....;
}

enum TruncateUnit {
    DAYS( ChronoUnit.DAYS ), HOURS( ChronoUnit.HOURS ), MINUTES( ChronoUnit.MINUTES ), ..., NANOS( ChronoUnit.NANOS ),;
}

As these units will be the same between AgeMin and AgeMax we should probably have them outside the constraint.

About UNITs: You suggest to use only NANOS and DAYS, to use the unit between them? SECOND, MINUTES.

I was thinking about the TruncateUnit where we could have all the units between days and nanos included.

I hope this clarify the part about the units. And as for the other part of the question - I'll get back to you once I remind myself what I was thinking back then 😃

[Hibernate-CI]

Can one of the admins add this person to the trusted builders? (reply with: "add to whitelist" or "ok to test")