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

HV-1552 Adding new MinAge Constraint #913

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Hibernate Validator, declare and validate application constraints
*
* License: Apache License, Version 2.0
* See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
*/
package org.hibernate.validator.cfg.defs;
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

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) ?

Copy link
Member

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.


import org.hibernate.validator.cfg.ConstraintDef;
import org.hibernate.validator.constraints.AgeMin;

import java.time.temporal.ChronoUnit;

/**
* @author Hillmer Chona
* @since 6.0.8
*/
public class AgeMinDef extends ConstraintDef<AgeMinDef, AgeMin> {

public AgeMinDef() {
super( AgeMin.class );
}

public AgeMinDef value(int value) {
addParameter( "value", value );
return this;
}

public AgeMinDef inclusive(boolean inclusive) {
addParameter( "inclusive", inclusive );
return this;
}

public AgeMinDef unit(ChronoUnit unit) {
addParameter( "unit", unit );
return this;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* Hibernate Validator, declare and validate application constraints
*
* License: Apache License, Version 2.0
* See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
*/
package org.hibernate.validator.constraints;

import javax.validation.Constraint;
import javax.validation.Payload;
import java.lang.annotation.Documented;
import java.lang.annotation.Repeatable;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
import java.time.temporal.ChronoUnit;


import static java.lang.annotation.ElementType.ANNOTATION_TYPE;
import static java.lang.annotation.ElementType.CONSTRUCTOR;
import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.ElementType.TYPE_USE;
import static java.lang.annotation.RetentionPolicy.RUNTIME;

/**
* The annotated element must be a date where the number of Years, Days, Months, etc. according
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect. Done.

* to an unit {@code java.time.temporal.ChronoUnit} go by to today must be
* greater or equal to the specified value if inclusive is true
* or is greater when inclusive is false.
* <p>
* <p>
* The supported type is {@code LocalDate}. {@code null} is considered valid.
* The supported type is {@code Calendar}. {@code null} is considered valid.
* <p>
*
* @author Hillmer Chona
* @since 6.0.8
*/
@Target({METHOD, FIELD, ANNOTATION_TYPE, CONSTRUCTOR, PARAMETER, TYPE_USE})
@Retention(RUNTIME)
@Repeatable(AgeMin.List.class)
@Documented
@Constraint(validatedBy = {})
public @interface AgeMin {

String message() default "{org.hibernate.validator.constraints.AgeMin.message}";

Class<?>[] groups() default {};

Class<? extends Payload>[] payload() default {};

/**
* @return value the referenceAge according to unit from a given date must be greater or equal to
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
int value();
Copy link
Member

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.


/**
* Specifies whether the specified value is inclusive or exclusive.
* 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
Copy link
Member

Choose a reason for hiding this comment

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

years -> date period units ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, Done

*/
boolean inclusive() default true;


/**
* Specifies the date period unit ( years, months, days, etc.) that will be used to compare the given date
* with the reference value.
* By default, it is YEARS.
*
* @return unit the date period unit
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
ChronoUnit unit() default ChronoUnit.YEARS;
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably switch places of unit and inclusive. It would be nicer to read having value "of" units inclusive. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Done


/**
* Defines several {@link AgeMin} annotations on the same element.
*
* @see AgeMin
*/
@Target({METHOD, FIELD, ANNOTATION_TYPE, CONSTRUCTOR, PARAMETER, TYPE_USE})
@Retention(RUNTIME)
@Documented
@interface List {
AgeMin[] value();
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Hibernate Validator, declare and validate application constraints
*
* License: Apache License, Version 2.0
* See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
*/
package org.hibernate.validator.internal.constraintvalidators.hv.age;

import java.lang.annotation.Annotation;
import java.time.Clock;
import java.time.Duration;
import java.time.Instant;
import java.time.temporal.ChronoUnit;

import javax.validation.ConstraintValidatorContext;

import org.hibernate.validator.constraintvalidation.HibernateConstraintValidator;

/**
* Base class for all age validators that use an {@link Instant} to be compared to the age reference.
*
* @author Hillmer Chona
* @since 6.0.8
*/
public abstract class AbstractAgeInstantBasedValidator<C extends Annotation, T> implements HibernateConstraintValidator<C, T> {

protected Clock referenceClock;

protected int referenceAge;

protected boolean inclusive;

protected ChronoUnit unit;
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


Copy link
Member

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.

Copy link
Contributor Author

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 boolean isValid(T value, ConstraintValidatorContext context) {
// null values are valid
if ( value == null ) {
return true;
}

long result = this.getCurrentAge( value ) - this.referenceAge;
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

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?

Copy link
Member

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 ChronoUnits . 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;

    // ....
}


return isValid( result );
}

/**
* Returns the temporal validation tolerance to apply.
*/
protected abstract Duration getEffectiveTemporalValidationTolerance(Duration absoluteTemporalValidationTolerance);

/**
* Returns the number of Years, Days, Months, etc. according to an unit {@code java.time.temporal.ChronoUnit}
* from a given {@code java.util.Calendar} to current day
*/
protected abstract long getCurrentAge(T value);

/**
* Returns whether the result of the comparison between the validated value and the age reference is considered
* valid.
*/
protected abstract boolean isValid(long result);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Hibernate Validator, declare and validate application constraints
*
* License: Apache License, Version 2.0
* See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
*/
package org.hibernate.validator.internal.constraintvalidators.hv.age;

import java.lang.annotation.Annotation;
import java.time.Clock;
import java.time.Duration;
import java.time.temporal.ChronoUnit;
import java.time.temporal.TemporalAccessor;

import javax.validation.ConstraintValidatorContext;

import org.hibernate.validator.constraintvalidation.HibernateConstraintValidator;

/**
* Base class for all age validators that are based on the {@code java.time} package.
*
* @author Hillmer Chona
* @since 6.0.8
*/
public abstract class AbstractAgeTimeBasedValidator<C extends Annotation, T extends TemporalAccessor & Comparable<? super T>>
implements HibernateConstraintValidator<C, T> {

protected Clock referenceClock;

protected int referenceAge;

protected boolean inclusive;

protected ChronoUnit unit;

@Override
public boolean isValid(T value, ConstraintValidatorContext context) {
// null values are valid
if ( value == null ) {
return true;
}

long result = this.getCurrentAge( value ) - this.referenceAge;

return isValid( result );
}

/**
* Returns the temporal validation tolerance to apply.
*/
protected abstract Duration getEffectiveTemporalValidationTolerance(Duration absoluteTemporalValidationTolerance);

/**
* Returns the number of Years, Days, Months, etc. according to an unit {@code java.time.temporal.ChronoUnit}
* from a given {@code java.util.Calendar} to current day
*/
protected abstract long getCurrentAge(T value);

/**
* Returns whether the result of the comparison between the validated value and the age reference is considered
* valid.
*/
protected abstract boolean isValid(long result);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Hibernate Validator, declare and validate application constraints
*
* License: Apache License, Version 2.0
* See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
*/
package org.hibernate.validator.internal.constraintvalidators.hv.age.min;

import java.lang.invoke.MethodHandles;
import java.time.Clock;
import java.time.Duration;
import java.time.Instant;

import javax.validation.metadata.ConstraintDescriptor;

import org.hibernate.validator.constraints.AgeMin;
import org.hibernate.validator.constraintvalidation.HibernateConstraintValidatorInitializationContext;
import org.hibernate.validator.internal.constraintvalidators.hv.age.AbstractAgeInstantBasedValidator;
import org.hibernate.validator.internal.util.logging.Log;
import org.hibernate.validator.internal.util.logging.LoggerFactory;

/**
* Base class for all {@code @AgeMin} validators that use an {@link Instant} to be compared to the age reference.
*
* @author Hillmer Chona
* @since 6.0.8
*/
public abstract class AbstractAgeMinInstantBasedValidator<T> extends AbstractAgeInstantBasedValidator<AgeMin, T> {

private static final Log LOG = LoggerFactory.make( MethodHandles.lookup() );

@Override
public void initialize(ConstraintDescriptor<AgeMin> constraintDescriptor, HibernateConstraintValidatorInitializationContext initializationContext) {
try {
super.referenceClock = Clock.offset(
Copy link
Member

Choose a reason for hiding this comment

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

having that initialize method in parent class this can be replaced with something like

AgeMin annotation = constraintDescriptor.getAnnotation();
initialize( annotation.value(), annotation.unit(), annotation.inclusive(), initializationContext );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like to say super.initialize, super.SOMETHING ..., to make the code easier to read, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

yeah it does make sense, I've also looked at other examples and that's the pattern used in them as well. So yep, make a change :)
just as an example of such usage - https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/constraintvalidators/hv/Mod10CheckValidator.java#L45

initializationContext.getClockProvider().getClock(),
getEffectiveTemporalValidationTolerance( initializationContext.getTemporalValidationTolerance() )
);
super.referenceAge = constraintDescriptor.getAnnotation().value();
super.inclusive = constraintDescriptor.getAnnotation().inclusive();
super.unit = constraintDescriptor.getAnnotation().unit();

}
catch (Exception e) {
throw LOG.getUnableToGetCurrentTimeFromClockProvider( e );
}
}

@Override
protected Duration getEffectiveTemporalValidationTolerance(Duration absoluteTemporalValidationTolerance) {
return absoluteTemporalValidationTolerance;
}

@Override
protected boolean isValid(long result) {
return super.inclusive ? result >= 0 : result > 0;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Hibernate Validator, declare and validate application constraints
*
* License: Apache License, Version 2.0
* See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
*/
package org.hibernate.validator.internal.constraintvalidators.hv.age.min;

import java.lang.invoke.MethodHandles;
import java.time.Clock;
import java.time.Duration;
import java.time.temporal.Temporal;
import java.time.temporal.TemporalAccessor;

import javax.validation.metadata.ConstraintDescriptor;

import org.hibernate.validator.constraints.AgeMin;
import org.hibernate.validator.constraintvalidation.HibernateConstraintValidatorInitializationContext;
import org.hibernate.validator.internal.constraintvalidators.hv.age.AbstractAgeTimeBasedValidator;
import org.hibernate.validator.internal.util.logging.Log;
import org.hibernate.validator.internal.util.logging.LoggerFactory;

/**
*
* Base class for all {@code @AgeMin} validators that are based on the {@code java.time} package.
*
* @author Hillmer Chona
* @since 6.0.8
*/
public abstract class AbstractAgeMinTimeBasedValidator<T extends Temporal & TemporalAccessor & Comparable<? super T>>
extends AbstractAgeTimeBasedValidator<AgeMin, T> {

private static final Log LOG = LoggerFactory.make( MethodHandles.lookup() );

@Override
public void initialize(ConstraintDescriptor<AgeMin> constraintDescriptor, HibernateConstraintValidatorInitializationContext initializationContext) {
try {
super.referenceClock = Clock.offset(
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

initializationContext.getClockProvider().getClock(),
getEffectiveTemporalValidationTolerance( initializationContext.getTemporalValidationTolerance() )
);
super.referenceAge = constraintDescriptor.getAnnotation().value();
super.inclusive = constraintDescriptor.getAnnotation().inclusive();
super.unit = constraintDescriptor.getAnnotation().unit();

}
catch (Exception e) {
throw LOG.getUnableToGetCurrentTimeFromClockProvider( e );
}
}

@Override
protected Duration getEffectiveTemporalValidationTolerance(Duration absoluteTemporalValidationTolerance) {
return absoluteTemporalValidationTolerance.negated();
}

@Override
protected boolean isValid(long result) {
return super.inclusive ? result >= 0 : result > 0;
}
}
Loading