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

Add exclusive mode to MinDuration and MaxDuration annotations #2167

Merged
merged 5 commits into from Nov 14, 2017

Conversation

Projects
None yet
5 participants
@FredDeschenes
Contributor

FredDeschenes commented Oct 3, 2017

A lot of the configurations with a @MinDuration seem to actually need to validate that a duration has been set and is greater than zero, but were stuck to use some kind of arbitrary minimum value (some of them used 1 millisecond, which should be good enough for most cases, but some used 1 second which is pretty high). I don't think Dropwizard should be forcing minimum durations on its configurations, just as no @MaxDuration is used anywhere.

I've changed the ones in DataSourceFactory as mentioned in the issue, but 6 more remain throughout the project. I wasn't comfortable enough with these configurations to change them myself, but I can update the PR if you feel like they can also be changed to a @PositiveDuration.

I'm also not totally sold on the name 'PositiveDuration', but I feel like it's clearer than the 'NonZeroDuration' I proposed in #2130 (which would imply that "-1 nanoseconds" is valid).

@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2017

Coverage Status

Coverage increased (+0.006%) to 85.427% when pulling a9be4f4 on FredDeschenes:positive-durations into e8f7f6c on dropwizard:master.

@evnm

This comment has been minimized.

Member

evnm commented Oct 29, 2017

Thanks for responding to #2130, @FredDeschenes.

Instead of introducing a new annotation, what if we added an boolean exclusive element to MinDuration (defaulting to false) which would control whether the value was treated inclusively or exclusively? This element could feasibly be added to the rest of the min/max validation annotations in Dropwizard as well.

In this case, the usage in DataSourceFactory would become

@MinDuration(value = 1, unit = TimeUnit.SECONDS, exclusive=true)
private Duration maxWaitForConnection = Duration.seconds(30);
@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Oct 30, 2017

Good idea, but that seems like a different issue.

#2130 is about some @MinDuration values being set to pretty high values when all we want to validate is that a duration was actually set. We could of course not add the @PositiveDuration and just make sure the values in the @MinDurations are set to low enough values.

@evnm

This comment has been minimized.

Member

evnm commented Oct 31, 2017

As I understand it, the issue raised in #2130 is that the defined minimum duration is too large. The solution is to lower the default such that any positive real number is valid.

I believe our two solutions be to equivalent—the set of positive real numbers is equal to the set of real numbers with an exclusive minimum of zero.

Does that make sense?

@FredDeschenes FredDeschenes force-pushed the FredDeschenes:positive-durations branch from a9be4f4 to c485085 Oct 31, 2017

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Oct 31, 2017

Yeah I guess it does make sense, just might be a little harder to get what's going on when reading the annotation.

Anyway I've added the exclusive mode to MinDuration and MaxDuration, should I modify MinDuration usages to be something like @MinDuration(value = 0, unit = TimeUnit.NANOSECONDS, exclusive = true)?

@FredDeschenes FredDeschenes changed the title from Add @PositiveDuration annotation and associated validator to Add exclusive mode to MinDuration and MaxDuration annotations Oct 31, 2017

@coveralls

This comment has been minimized.

coveralls commented Oct 31, 2017

Coverage Status

Coverage increased (+0.0004%) to 85.498% when pulling c485085 on FredDeschenes:positive-durations into 2f8ba7e on dropwizard:master.

@evnm

This comment has been minimized.

Member

evnm commented Oct 31, 2017

I think 1 ms is a reasonable default. Do you agree?

I don't want to unilaterally drive the bus here. @nickbabcock, what do you think?

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 31, 2017

Oh man, if I turn my nitpickiness up to 11, I prefer an inclusive argument over exclusive to avoid the explicit exclusive=false double negative. I believe "inclusive" also has fewer definitions than "exclusive", which might ease the impact on readability (as @FredDeschenes pointed out)

The error message becomes shorter

must be less than or equal (or less than if in 'exclusive' mode) to {value} {unit}

becomes

must be less than (or equal to in 'inclusive' mode) to {value} {unit}

Ideally the correct operator (>= / >) would be present in the error message, so the user would know if upper bound is valid instead of the user checking the source to see if inclusive is enabled 🤔

I'd prefer if this PR went through without touching all the duration annotations to keep potential bikeshedding down. I'm not sure how many annotations are affected, but if it's anymore than a handful then another PR is best.

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Oct 31, 2017

I switched 'exclusive' to 'inclusive'.

As for displaying the operator, I can either add them to the current text or replace words by them (ex: "greater" -> ">"), but since the message is in an annotation it needs to be constant so we can't change the message depending on the mode.

As for the original issue of the @MinDuration annotations, there's around 30 of them but #2130 was specifically for the ones in DataSourceFactory so 7. I'll create another PR once this is merged and you decide on what the minimum value should be.

@coveralls

This comment has been minimized.

coveralls commented Oct 31, 2017

Coverage Status

Coverage increased (+0.0004%) to 85.498% when pulling c3f38e4 on FredDeschenes:positive-durations into 2f8ba7e on dropwizard:master.

@evnm

Looks good. Just a wording nit for the inclusive javadocs.

@@ -26,7 +26,7 @@
@Documented
@Constraint(validatedBy = MaxDurationValidator.class)
public @interface MaxDuration {
String message() default "must be less than or equal to {value} {unit}";
String message() default "must be less than (or equal if in 'inclusive' mode) to {value} {unit}";

This comment has been minimized.

@evnm

evnm Nov 1, 2017

Member

"less than (or equal to, if in 'inclusive' mode) {value}"

@@ -41,4 +41,9 @@
* @return unit of the value the element must be higher or equal to
*/
TimeUnit unit() default TimeUnit.SECONDS;
/**
* @return if the validation should include the set value (default is true).

This comment has been minimized.

@evnm

evnm Nov 1, 2017

Member

Perhaps something worded like the following, for this and the other equivalent javadocs.

"True if the validation is to allow values equal to {@code value}. False if the validation is to be exclusive. Defaults to {@code true}."

@coveralls

This comment has been minimized.

coveralls commented Nov 2, 2017

Coverage Status

Coverage increased (+0.0004%) to 85.498% when pulling f08aae5 on FredDeschenes:positive-durations into 2f8ba7e on dropwizard:master.

@evnm

Pardon the delay. I went on vacation right around the time you posted that last diff. Thanks for bearing with me here, @FredDeschenes.

@@ -26,7 +26,7 @@
@Documented
@Constraint(validatedBy = MaxDurationValidator.class)
public @interface MaxDuration {
String message() default "must be less than or equal to {value} {unit}";
String message() default "must be less than (or equal to, if in 'inclusive' mode) to {value} {unit}";

This comment has been minimized.

@evnm

evnm Nov 14, 2017

Member

Rm that second "to"

@@ -26,7 +26,7 @@
@Documented
@Constraint(validatedBy = MinDurationValidator.class)
public @interface MinDuration {
String message() default "must be greater than or equal to {value} {unit}";
String message() default "must be greater than (or equal to, if in 'inclusive' mode) to {value} {unit}";

This comment has been minimized.

@evnm

evnm Nov 14, 2017

Member

ditto

"tooSmall must be greater than or equal to 30 SECONDS",
"maxDurs[0].<collection element> must be less than or equal to 30 SECONDS",
"minDurs[0].<collection element> must be greater than or equal to 30 SECONDS",
"tooBig must be less than (or equal to, if in 'inclusive' mode) to 30 SECONDS",

This comment has been minimized.

@evnm

evnm Nov 14, 2017

Member

ditto, for all of these

@coveralls

This comment has been minimized.

coveralls commented Nov 14, 2017

Coverage Status

Coverage increased (+0.0007%) to 87.174% when pulling 3b8413d on FredDeschenes:positive-durations into 8bee1f5 on dropwizard:master.

@evnm

evnm approved these changes Nov 14, 2017

Thanks!

@evnm evnm added the improvement label Nov 14, 2017

@evnm evnm added this to the 1.2.1 milestone Nov 14, 2017

@evnm

This comment has been minimized.

Member

evnm commented Nov 14, 2017

Whoops, looks like this branch has diverged from master. @FredDeschenes, could you please resolve the merge conflicts so we can get it in?

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Nov 14, 2017

I fixed those conflicts this morning in 3b8413d, everything says there's no conflict anymore on my side ("This branch has no conflicts with the base branch"), is it showing you conflicts?

@evnm

This comment has been minimized.

Member

evnm commented Nov 14, 2017

Ah, my bad. The "rebase and merge" option was selected and is a lot less forgiving when it comes to merge conflicts. Squash+merge is green, so we're good to go.

@evnm evnm merged commit d674b0c into dropwizard:master Nov 14, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate 4 fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0007%) to 87.174%
Details
@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Nov 14, 2017

Thanks, I'll go ahead and make another PR to actually fix #2130 !

@FredDeschenes FredDeschenes deleted the FredDeschenes:positive-durations branch Nov 14, 2017

sankate pushed a commit to sankate/dropwizard that referenced this pull request Nov 21, 2017

@arteam arteam modified the milestones: 1.2.1, 1.3.0 Nov 22, 2017

aaanders added a commit to aaanders/dropwizard that referenced this pull request Sep 20, 2018

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