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-823 Provide contract for customization of property names in constraint violation #1029

Merged
merged 2 commits into from May 14, 2019

Conversation

@dalibegovic
Copy link
Contributor

dalibegovic commented Mar 28, 2019

Added PropertyNodeNameProvider SPI with Property and JavaBeanProperty as supporting interfaces.

This SPI lives in JavaBeanHelper and is used to get the name when creating JavaBeanField and JavaBeanGetter, so when a property path is constructed, this resolved name is used.

If not set, the default implementation will be used that returns the actual name from the class.

This new SPI can be configured through HibernateValidatorConfiguration.

Testing:

  • Added tests for configuration
  • Added a sample implementation by using reflection and custom annotation
  • Added tests for reflection implementation
  • Added a sample implementation by using Jackson lib
  • Added tests for Jackson implementation

Added documentation with examples.

This PR is same as: #1012 + the documentation, but the changes are squashed and rebased.

@dalibegovic dalibegovic mentioned this pull request Mar 28, 2019
@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented Mar 29, 2019

@marko-bekhta is there any more work needed from my side for now?

@marko-bekhta

This comment has been minimized.

Copy link
Contributor

marko-bekhta commented Mar 29, 2019

@dalibegovic all good for now. @gsmet will take a look and might provide some additional feedback.

@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented Apr 9, 2019

@marko-bekhta @gsmet hey, just wanted to check if there are any updates?

@julien-may

This comment has been minimized.

Copy link
Contributor

julien-may commented Apr 16, 2019

May I ask about the state here?

@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented Apr 23, 2019

@marko-bekhta @gsmet Hey, just wanted to check one more time if there are any updates? It has been almost a month since we opened this PR.

We are looking forward to wrapping this thing up, but we need your feedback.

Copy link
Member

gsmet left a comment

So, first, sorry for the delay. I had a lot of things to do and couldn't find time to review this before this week.

I added quite a lot of comments but most of them are very minor and about documentation and javadoc. They should be easily fixed.

I have one thing I would like to be thoroughly tested though: could we have a test with a constraint on a getter to be sure the property name is also right, then? I might have missed it so if there is already one, just point me to it.

Thanks for pushing hard to get this in, we're nearly there!

documentation/src/main/asciidoc/ch12.asciidoc Outdated Show resolved Hide resolved
documentation/src/main/asciidoc/ch12.asciidoc Outdated Show resolved Hide resolved
documentation/src/main/asciidoc/ch12.asciidoc Outdated Show resolved Hide resolved
documentation/src/main/asciidoc/ch12.asciidoc Outdated Show resolved Hide resolved
documentation/src/main/asciidoc/ch12.asciidoc Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented Apr 25, 2019

Just a note that sorting in copyright.txt is a bit broken:

Damir:hibernate-validator dalibegovic$ diff <(cat copyright.txt) <(sort copyright.txt)
9d8
< Carlos Vara
10a10
> Carlos Vara
19d18
< Emmanuel Bernard
20a20
> Emmanuel Bernard

Noting big, just wonted to note what i noticed.

@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented Apr 30, 2019

@gsmet here is the test with a constraint on a getter that you requested: 0198b1d#diff-ebf404feb5c6d2ea86fdf3552618061fR81

I have also addressed all other comments, so I think we are ready for a merge.

If there is anything needed from my side for now, please let me know.

@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented May 10, 2019

Hey @gsmet, we are looking forward seeing this changes merged, so just wanted to check what the status is? Thanks!

@gsmet

This comment has been minimized.

Copy link
Member

gsmet commented May 13, 2019

Argh, thanks for pinging, I had it checked as done in my mind...

There is a javadoc issue I'm currently fixing. Will review the latest changes, squash and merge.

dalibegovic and others added 2 commits Dec 26, 2018
…aint violation

Added PropertyNodeNameProvider SPI with Property and JavaBeanProperty as supporting interfaces.

This SPI lives in JavaBeanHelper and is used to get the name when creating JavaBeanField and JavaBeanGetter,
so when a property path is constructed, this resolved name is used.

If not set, the default implementation will be used that returns the actual name from the class.

This new SPI can be configured through HibernateValidatorConfiguration.

Testing:
- Added tests for configuration
- Added a sample implementation by using reflection and custom annotation
- Added tests for reflection implementation
- Added a sample implementation by using Jackson lib
- Added tests for Jackson implementation

Added documentation with examples.
@gsmet gsmet force-pushed the olmero:HV-823b branch from 3e6e6ad to 68b7fbd May 13, 2019
@gsmet gsmet added the 6.1 label May 13, 2019
@gsmet

This comment has been minimized.

Copy link
Member

gsmet commented May 13, 2019

@dalibegovic I just pushed an update for CI:

  • I fixed the Javadoc issue and squashed the fix up commits
  • I adjusted a few things in the doc: a couple of typos and a few minor improvements. You might notice some purely cosmetic changes: one thing we try to do is to have one line per sentence (or more if it's a long sentence) so that diffs are easier to read in the future.

Could you take a look at the last commit and see if everything is OK for you?

@gsmet
gsmet approved these changes May 13, 2019
@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented May 14, 2019

@gsmet thanks for the changes, everything looks great.

I didn't know about the one line per sentence rule, but it makes sense.

@gsmet

This comment has been minimized.

Copy link
Member

gsmet commented May 14, 2019

@dalibegovic OK, perfect. We have a build instability with the latest JDK 11 I want to resolve before merging this (otherwise CI will fail once again). It shouldn't be long as I already created a PR that will hopefully fix it. As soon as it's fixed, I'll merge this and release a new alpha of the 6.1 branch so that you can use an official release with this feature in.

Thanks a lot for driving this. Sorry about the delay for the reviews.

@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented May 14, 2019

@gsmet great, looking forward to that.

If there is anything else needed from our side, please let us know.

@gsmet gsmet merged commit 7af9454 into hibernate:master May 14, 2019
1 check passed
1 check passed
default Build finished.
Details
@gsmet

This comment has been minimized.

Copy link
Member

gsmet commented May 14, 2019

And merged! Thanks everyone!

@gsmet

This comment has been minimized.

Copy link
Member

gsmet commented Jun 5, 2019

@dalibegovic @marko-bekhta we just had this very valuable input on SO: https://stackoverflow.com/questions/56467824/how-to-get-query-parameter-name-from-constraintviolationexception/56467930#56467930 .

We should probably generalize this idea to all the constraint locations. Not sure if we should generalize the contract (that was my initial thinking) or have different contracts for each type of location.

What do you all think?

@gsmet

This comment has been minimized.

Copy link
Member

gsmet commented Jun 5, 2019

Hmmm, let's forget about it, Gunnar reminded me about the ParameterNameProvider contract :).

@dalibegovic

This comment has been minimized.

Copy link
Contributor Author

dalibegovic commented Jun 6, 2019

@gsmet the library is even more powerful then we sometimes realize :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.