-
Notifications
You must be signed in to change notification settings - Fork 570
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
HV-1552 Adding new MinAge Constraint #913
base: main
Are you sure you want to change the base?
Conversation
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
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. |
Hi @marko-bekhta , you right it was my mistake, I have pulled AgeMinDef, Could you check please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Hilmerc thanks for updated PR! I've added a few small comments inline. Would be great if you could try to add ChronoUnit
attribute. If that works out fine, then I'd say we could add support for other date/time classes for which this constraint would make sense. Having that in place we could move forward and add the @AgeMax
constraint. And also look into some other things like documentation and Annotation Processor support. If you have any questions - please ask.
@@ -0,0 +1,21 @@ | |||
package org.hibernate.validator.cfg.defs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a license header comment as in the other classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @marko-bekhta I have done all your suggestions except the one for ChronoUnit attribute. I'm not sure what you mean. Is it something like this: add to @interface AgeMin
a attribute like ChronoUnit unit();
, so users can define the value when use the annotation like this:
@AgeMin( value = MINIMUM_AGE , inclusive = true, unit= ChronoUnit.YEARS)
or @AgeMin( value = MINIMUM_AGE , inclusive = true, unit= ChronoUnit.MONTHS)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Hilmerc yes, that's exactly it! :) This will make the constraint more versatile.
I've also prepared a short plan, with items that are still needed to finish this work. I'll post it in the separate comment. I hope it'll be helpful.
/** | ||
* @return value the age in years from a given date must be greater or equal to | ||
*/ | ||
int value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the previous discussion, there was an idea to try out to support other units (ChronoUnit
) with the ChronoUnit#YEARS
by default. Could you please try to experiment with that ? Should be simple to add something like ChronoUnit unit() default ChronoUnit.YEARS;
to the constraint. If it works the programmatic definition (AgeMinDef
) would need this attribute to be added as well.
@TestForIssue(jiraKey = "HV-1552" ) | ||
public class AgeValidatorTest { | ||
|
||
private int value = 18; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make a constant of it and use in other occurrences of this number as well. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/** | ||
* @return an initialized {@link ConstraintValidator} using {@code DUMMY_CONSTRAINT_VALIDATOR_INITIALIZATION_CONTEXT} | ||
*/ | ||
private ConstraintValidator<AgeMin, LocalDate> getInitializedValidator(int value, boolean inclusive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we place private methods at the end of the test class and start with the tests. It's easier to see what is tested this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* @since 6.0.8 | ||
*/ | ||
@TestForIssue(jiraKey = "HV-1552" ) | ||
public class AgeValidatorTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These set of tests would check if AgeMinValidator
works as expected as well as the programmatic definition. Could you also add another test class similar for example to this one - https://github.com/hibernate/hibernate-validator/blob/master/engine/src/test/java/org/hibernate/validator/test/constraints/annotations/hv/ISBNConstrainedTest.java
We add such tests in that package to make sure that the constraint and corresponding validators are correctly registered and also that the default message is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@Hilmerc so about the plan that I've mentioned...
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. |
@marko-bekhta I am working with the plan, for the moment no doubts |
@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? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Hilmerc good progress! I've added a couple of comments here and there, those license headers are easy to miss :). Looks good in general. I think we can try to improve those validator classes a bit more and pull the data manipulation in the abstract ones to prevent repetition. Let me know what you think of that.
Class<? extends Payload>[] payload() default {}; | ||
|
||
/** | ||
* @return value the referenceAge according to unit from a given date must be greater or equal to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value and referenceAge seems to be a duplication. Maybe something like:
the amount of date period units, defined by {@link AgeMin#unit()}, have passed since the annotated elementdate till now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
import static java.lang.annotation.RetentionPolicy.RUNTIME; | ||
|
||
/** | ||
* The annotated element must be a date where the number of Years, Days, Months, etc. according |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it more similar to other constraint descriptions.
* The annotated element must be an instant, date or time for which at least
* the specified amount ({@link AgeMin#value()}) of Years/Days/Months/etc. defined
* by {@link AgeMin#unit()} have passed till now.
* <p>
* Supported types are:
* <ul>
* <li>{@code java.util.Calendar}</li>
* <li>{@code java.time.LocalDate}</li>
* <li> {@code add any other types that will be supported} </li>
* </ul>
* <p>
* {@code null} elements are considered valid.
how about something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. Done.
* By default, it is inclusive. | ||
* | ||
* @return {@code true} if the number of years from a given date must be higher or equal to the specified value, | ||
* {@code false} if the number of years from a given date must be higher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
years -> date period units ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, Done
* with the reference value. | ||
* By default, it is YEARS. | ||
* | ||
* @return unit the date period unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the date period unit
should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
protected boolean inclusive; | ||
|
||
protected ChronoUnit unit; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add
public void initialize(int referenceAge, ChronoUnit unit, boolean inclusive, HibernateConstraintValidatorInitializationContext initializationContext) {
try {
this.referenceClock = Clock.offset(
initializationContext.getClockProvider().getClock(),
getEffectiveTemporalValidationTolerance( initializationContext.getTemporalValidationTolerance() )
);
}
catch (Exception e) {
throw LOG.getUnableToGetCurrentTimeFromClockProvider( e );
}
this.referenceAge = referenceAge;
this.unit = unit;
this.inclusive = inclusive;
}
here. This way we will not need to repeat same logic for referenceClock
in all other validators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do something like you say, but no good ideas came to me, thanks. Done.
@Override | ||
public void initialize(ConstraintDescriptor<AgeMin> constraintDescriptor, HibernateConstraintValidatorInitializationContext initializationContext) { | ||
try { | ||
super.referenceClock = Clock.offset( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same could be done here with the parent initialize
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return true; | ||
} | ||
|
||
long result = this.getCurrentAge( value ) - this.referenceAge; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's slightly change this isValid
method so we wouldn't need to write the similar code that's right now in Calendar
validator for the Date
one. As instants does not support add/subtract operations on units > days we need to do something similar to what is already in AgeMinValidatorForCalendar
. Maybe something like:
public boolean isValid(T value, ConstraintValidatorContext context) {
// null values are valid
if ( value == null ) {
return true;
}
// As Instant does not support plus operation on ChronoUnits greater than DAYS we need to convert it to LocalDateTime
// first, which supports such operations.
long result = LocalDateTime.ofInstant( getInstant( value ), ZoneOffset.ofHours( 0 ) ).plus( referenceAge, unit ).compareTo(
LocalDateTime.now( referenceClock )
);
return isValid( result );
}
with such change you'd need to change the corresponding isValid method to:
@Override
protected boolean isValid(long result) {
return this.inclusive ? result <= 0 : result < 0;
}
Then for Date
you would only need to convert it to instant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marko-bekhta the method LocalDate.ofInstant( ... ) comes on Java 9, I'm no sure about to use Java 9 features. I use next line than is compatible with Java 8
getInstant( value ).atZone( ZoneOffset.ofHours( 0 ) ).toLocalDate()
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... We don't want Java 9 stuff :). I was suggesting to use LocalDateTime
which should allow to use any of ChronoUnit
s . Otherwise we would have the ability to do something like:
class MyClass {
@AgeMin( value = 24 * 60, unit = ChronoUnit.MINUTES )
private LocalDate date;
//.....
}
but the add
operation would fail for such unit on LocalDate
. Also I guess it will answers your other question about supported types. I think this constraint can have a more wider meaning. We could think of it as a constraint on how much time units have passed till now. And would allow having something like:
class Exam {
@AgeMin(value = 5, unit = ChronoUnit.MINUTES, message = "You should at least spent 5 minutes on the exam, before you can submit it")
@AgeMax(value = 60, unit = ChronoUnit.MINUTES, message = "You cannot submit your exam as the allowed time limit have been exceeded ")
private LocalTime examTime;
// ....
}
or something like:
class Token {
@AgeMax(value = 10, unit = ChronoUnit.MINUTES, message = "Token expired")
private LocalDateTime issuedAt;
// ....
}
@@ -0,0 +1,32 @@ | |||
package org.hibernate.validator.internal.constraintvalidators.hv.age.min; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license header is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,25 @@ | |||
package org.hibernate.validator.internal.constraintvalidators.hv.age.min; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license header is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
@Override | ||
protected long getCurrentAge(LocalDate value) { | ||
return super.unit.between( value, getReferenceValue( super.referenceClock ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be good to pull this logic into abstract validator, so we wouldn't need to repeat it for other date/time types. Maybe something similar to what was mentioned for Instant
based validator ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @Hilmerc ! I think we are almost there. Mainly there's only one thing that bugs me - those date-time plus/minus operations which potentially could throw exceptions if the unit is "wrong" for the constrained type. Could you try using LocalDateTime
as mentioned in the other comment, and see how it would work ?
import static org.hibernate.validator.testutil.ConstraintViolationAssert.violationOf; | ||
|
||
import java.time.LocalDate; | ||
import java.time.temporal.ChronoUnit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be unused import - http://ci.hibernate.org/job/hibernate-validator-PR/1062/console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// As Instant does not support plus operation on ChronoUnits greater than DAYS we need to convert it to LocalDate | ||
// first, which supports such operations. | ||
|
||
long result = getInstant( value ).atZone( ZoneOffset.ofHours( 0 ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mentioned in another comment, I think we should stick to LocalDateTime
so there will be no errors for different units. In case we use LocalDate
here UnsupportedTemporalTypeException
will be thrown for minutes, hours and other time units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marko-bekhta for InstanteBase I don't see any problem using either LocalDate or LocalDateTime, but it is necessary convert to LocalDate before comparing Calendar, the reason is that Calendar has time, even though if you were born for example at 10:00 pm your new age starts from 00:00 AM.
long result = getInstant( value ).atZone( ZoneOffset.ofHours( 0 ) )
.toLocalDateTime().plus( referenceAge, unit ).toLocalDate()
.compareTo( LocalDateTime.now( referenceClock).toLocalDate() );
For TimeBased, I got an UnsupportedTemporalTypeException
yesterday with Year, as you say with LocalDate.
A possible solution can be to create a method LocalDateTime getReferenceValue(T value, Clock reference), and for Year return the year that comes on value plus January 1st 00:00 AM, For LocalDate the date that comes on value plus 00:00 AM. I mean they only return a LocalDateTime, adding the values (date and/or time) to make the T value consistent . But I'm not sure about it. Can you see other solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hilmerc that's a very good point on the time part of birthday and how we calculate the age. Thinking more about it, it might be that we wanted to do to much with one constraint and we might slightly moved away from your original intention for it. Seems that there are next two ways how it can move forward from this point:
- As we already have the
unit
in place and it is possible to specify age for example in minutes it wouldn't make sense to truncate all that data up to days which is suitable for calculating the age of a person. Hence if we want to keepunit
attribute in place and also have a correct logic for calculating an age of a person we would need to add another attribute to this constraint. Something likeChronoUnit truncateTo() default ChronoUnit.NANOS;
a time unit to which we want to truncate the date-time. In such case if we want an age of a person we would usetruncateTo=ChronoUnit.DAYS
. And if we would like to keep the time part we would leave it as default. - Ignore the idea of having the ability to specify units for an age and keep this particular constraint as simple as an age of a person. But as it looks to be an interesting and useful case to be able to specify constraints on how much time have passed think of a different constraint for it.
Personally, I'm leaning more towards (1), as it will be a more versatile constraint. If you like this approach and would want to give it a try we'd need to add this additional attribute to a constraint and then use LocalDateTime#truncatedTo(TemporalUnit)
in validators. Also as we would only operate with LocalDateTime
instances in the validator to prevent exceptions on plus/minus operations maybe it will actually make sense to have a single base validator for all types? We could then modify the current instant one to something like:
class AbstractAgeValidator {
....
public void initialize(
int referenceAge,
ChronoUnit unit,
boolean inclusive,
HibernateConstraintValidatorInitializationContext initializationContext) {
....
}
@Override
public boolean isValid(T value, ConstraintValidatorContext context) {
....
}
protected abstract LocalDateTime getLocalDateTime(T value);
....
}
class AbstractAgeInstantBasedValidator extends AbstractAgeValidator {
protected abstract Instant getInstant(T value);
@Override
protected LocalDateTime getLocalDateTime(T value){
return LocalDateTime.ofInstant(getInstant(value));
}
}
and for types like LocalDate
and others we could just use methods on them to get to LocalDateTime
and return directly from getLocalDateTime
method.
Also, just as a note, in any of these cases, we would need to write up a better explanation of what the constraint is checking and how it will behave. Just to set the users expectations right :) But don't think about this part yet. That's something for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marko-bekhta after some work and tests I figured out that the attribute ChronoUnit#truncateTo() can generate some problems like ChronoUnit#Unit.
jshell> LocalDateTime.now( ZoneOffset.UTC ).truncatedTo(ChronoUnit.YEARS);
| java.time.temporal.UnsupportedTemporalTypeException thrown: Unit is too large to be used for truncation
jshell> LocalDateTime.now( ZoneOffset.UTC ).truncatedTo(ChronoUnit.WEEKS);
| java.time.temporal.UnsupportedTemporalTypeException thrown: Unit is too large to be used for truncation
I think we should eliminate the ability to specify a unit, make validators with years and, after use and feedback, we can explore the possibility of returning to our idea of using Units.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hilmerc thanks a lot for experimenting with this approach. So it seems, we wanted to much from one constraint. Let's remove the units then and clean things up to keep this constraint just for age in years. I still think that the above idea with one abstract validator for all types would work (Just use LocalDate
instead of LocalDateTime
).
return true; | ||
} | ||
|
||
int result = value.compareTo( getReferenceValue( referenceClock, referenceAge, unit ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to follow the same pattern as in the instant validatior ? I mean do the operation once here in the validator rather than do the minus in each type's impl. Also I've noticed that you've used the minus for these TemporalAccessor
types and for Instant
there's add
. Would be better to use the same one in both, both are fine just pick one :)
* {@link ClockProvider} increased or decreased with the specified referenceAge of Years/Days/Months/etc. | ||
* defined by {@link ChronoUnit}. | ||
*/ | ||
protected abstract T getReferenceValue(Clock reference, int referenceAge, ChronoUnit unit ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking more about this method, it looks like we will have the same problem with add/minus operations as mentioned earlier. Let's assume we have a LocalDate
as below:
class Foo {
@AgeMin(value = 60, unit = ChronoUnit.HOURS)
private LocalDate date;
}
it will result in UnsupportedTemporalTypeException
when we try to perform either plus
or minus
operations in the validator. So I guess we need to work with LocalDateTime
all the time?
|
||
protected boolean inclusive; | ||
|
||
protected ChronoUnit unit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICS, at least part of these properties can be private now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
@Override | ||
protected boolean isValid(long result) { | ||
return super.inclusive ? result <= 0 : result < 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're using this
and super
quite randomly. See the class just above. Let's provide a protected isInclusive()
getter in the parent class, it's going to be cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Hi @marko-bekhta , I made some changes 'cause I think the idea to define the units is a good idea. I created an Enum
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 |
@Hilmerc that's great. Thanks for experimenting more with this! I'll try to have a look later today. |
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. |
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. |
@marko-bekhta I'm not sure if I understand well you said. Hillmer |
Hi @HillmerCh no worries 😃. IIRC I was thinking that as we discarded the idea of 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
I was thinking about the 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 😃 |
Can one of the admins add this person to the trusted builders? (reply with: "add to whitelist" or "ok to test") |
This PR has the changes suggested for you