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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Descriptive constraint violation messages #1039

Merged
merged 2 commits into from May 28, 2015

Conversation

Projects
None yet
4 participants
@nickbabcock
Contributor

nickbabcock commented May 8, 2015

This pull requests attempts to return user friendly error messages when constraints are violated either in the response or the request.

So instead of error messages of

{"errors":["blazer.arg0.name may not be empty"]}

and

{"errors":["blaze.<return value> length must be between 0 and 3"]}

more user friendly error messages are returned

{"errors":["query param name may not be empty"]}
{"errors":["server response length must be between 0 and 3"]}

This pull request isn't done, so don't merge (hardly any documentation and I didn't completely mull over edge cases). As it happens, I'll be off the grid for the next couple weeks, but I wanted to get some eyes on the code (hence the pull request), but if someone is extremely enthusiastic they can carry this pull request the rest of the way, else I'll pick off where I left off and incorporate any suggestions in the pull request.

The code isn't fast, concise, documented, or complete -- but it passes all the test cases I wrote for it 馃槃

Opinions wanted!

@arteam

This comment has been minimized.

Member

arteam commented May 16, 2015

I hacked a little on your first version of ConstraintViolationExceptionMapper. Here is my work.

I think, it's a little more concise and readable. If you like it, you can derive your work from this
version. Or just borrow some concepts.

I didn't touch the tests. Will try to review them in a short time and give some feedback also.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented May 26, 2015

Awesome, thanks for the alternative implementation while I was away, I'll start basing work on it and reply back once I'm satisfied.

@nickbabcock nickbabcock force-pushed the nickbabcock:constraint-messages branch 2 times, most recently from 604f1fd to caa15e0 May 26, 2015

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented May 26, 2015

  • Largely adapted @arteam's implementation
  • Added benchmarks so someone can come along and optimize if needed

If everything looks good, I can start writing up some prose documentation.

@nickbabcock nickbabcock force-pushed the nickbabcock:constraint-messages branch from caa15e0 to 7fc6e68 May 26, 2015

private ConstraintViolation<ConstraintViolationBenchmark> paramViolation;
private ConstraintViolation<ConstraintViolationBenchmark> objViolation;
public String paramFunc(@HeaderParam("cheese") @NotEmpty String secretSauce) {

This comment has been minimized.

@arteam

arteam May 26, 2015

Member

Maybe we could we create a static class Resource and move both these methods there?
It will separate the benchmark from the test data.

return foo.toString();
}
public class Foo {

This comment has been minimized.

@arteam

arteam May 26, 2015

Member

The class can be be static

getAccessibleMethod(getClass(), "paramFunc", String.class),
new Object[]{""} // the parameter value
);
paramViolation = Iterators.getOnlyElement(paramViolations.iterator());

This comment has been minimized.

@arteam

arteam May 26, 2015

Member

I think it's safe just to call iterator.next, because we know that the violation is exist.

getAccessibleMethod(getClass(), "objectFunc", Foo.class),
new Object[]{new Foo()} // the parameter value
);
objViolation = Iterators.getOnlyElement(objViolations.iterator());

This comment has been minimized.

@arteam

arteam May 26, 2015

Member

Ditto

} else {
return String.format("%s %s", v.getPropertyPath(), v.getMessage());
}
}
public static <T> String validationMethodFormatted(ConstraintViolation<T> v) {

This comment has been minimized.

@arteam

arteam May 26, 2015

Member

We probably could reuse Joiner and don't use String.format.

public static <T> String validationMethodFormatted(ConstraintViolation<T> v) {
        final ImmutableList<Path.Node> nodes = ImmutableList.copyOf(v.getPropertyPath());
        String usefulNodes = JOINER.join(nodes.subList(0, nodes.size() - 1));
        String msg = usefulNodes + (v.getMessage().startsWith(".") ? "" : " ") + v.getMessage();
        return msg.trim();
    }

Micro-benchmark, kindly provided by you, gives this on my machine:

Benchmark                                    Mode  Cnt     Score      Error  Units
ConstraintViolationBenchmark.objViolation    avgt    5   711.064 卤   22.911  ns/op
ConstraintViolationBenchmark.paramViolation  avgt    5  7400.960 卤 2268.245  ns/op

Benchmark                                    Mode  Cnt     Score     Error  Units
ConstraintViolationBenchmark.objViolation    avgt    5   693.016 卤  10.078  ns/op
ConstraintViolationBenchmark.paramViolation  avgt    5  6585.522 卤 815.698  ns/op
@arteam

This comment has been minimized.

Member

arteam commented May 26, 2015

Looks good to me! Apart from some remarks for benchmarks and one micro optimization, I think this change can be prepared for merging.

By the way, I guess it's the first PR where we try to use dropwizard-benchmarks for improving an implementation :)

@jplock jplock added this to the 0.9.0 milestone May 27, 2015

@nickbabcock nickbabcock force-pushed the nickbabcock:constraint-messages branch from 7fc6e68 to 18d1187 May 27, 2015

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented May 27, 2015

Suggestions taken, thanks for the review.

@coveralls

This comment has been minimized.

coveralls commented May 27, 2015

Coverage Status

Coverage increased (+0.23%) to 71.65% when pulling 18d1187 on nickbabcock:constraint-messages into 5d0dd61 on dropwizard:master.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented May 28, 2015

Maybe it is inappropriate for this pull request but I couldn't resist adding in a simple caching mechanism that increases performance by up to 250x when there is a hit.

Benchmark                                    Mode  Cnt     Score     Error  Units
ConstraintViolationBenchmark.objViolation    avgt    5  3416.563 卤 214.989  ns/op
ConstraintViolationBenchmark.paramViolation  avgt    5  5605.761 卤  90.774  ns/op

Benchmark                                    Mode  Cnt   Score   Error  Units
ConstraintViolationBenchmark.objViolation    avgt    5  22.430 卤 0.359  ns/op
ConstraintViolationBenchmark.paramViolation  avgt    5  22.643 卤 1.384  ns/op

@nickbabcock nickbabcock force-pushed the nickbabcock:constraint-messages branch 2 times, most recently from be8e20f to 8ff3e96 May 28, 2015

@nickbabcock nickbabcock force-pushed the nickbabcock:constraint-messages branch from 8ff3e96 to 67fd54f May 28, 2015

@coveralls

This comment has been minimized.

coveralls commented May 28, 2015

Coverage Status

Coverage decreased (-0.65%) to 71.72% when pulling 67fd54f on nickbabcock:constraint-messages into 150bac9 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented May 28, 2015

Coverage Status

Coverage increased (+0.27%) to 72.65% when pulling 67fd54f on nickbabcock:constraint-messages into 150bac9 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented May 28, 2015

Coverage Status

Coverage decreased (-0.65%) to 71.72% when pulling 67fd54f on nickbabcock:constraint-messages into 150bac9 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented May 28, 2015

Coverage Status

Coverage increased (+0.27%) to 72.65% when pulling 67fd54f on nickbabcock:constraint-messages into 150bac9 on dropwizard:master.

arteam added a commit that referenced this pull request May 28, 2015

Merge pull request #1039 from nickbabcock/constraint-messages
Descriptive constraint violation messages

@arteam arteam merged commit 0594e58 into dropwizard:master May 28, 2015

@arteam

This comment has been minimized.

Member

arteam commented May 28, 2015

Thanks! Merged to master.

UPD. I missed your comment about caching and merged this too early. Fixed up this a little in a21ce52

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