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

Fix for #479 #237

Closed

Conversation

FroMage
Copy link
Contributor

@FroMage FroMage commented May 20, 2011

http://play.lighthouseapp.com/projects/57987-play-framework/tickets/479

Replaces #55

I've split it up nice and simple so you can see why you also want the refactorings for simplifying the binder and the Factory caching for performance reasons.

Also this is now against the 1.2.1 tag

Stephane Epardaud and others added 16 commits May 17, 2011 16:47
Also refactor fields traversal and ignore non-mapped classes in the hierarchy.
Also refactor binder for slightly saner code.
The binding is necessary because it was removed from the factory as this caused a mismatch with JPABase.findById that did not make sense.
…nse.

It is possible that the first error message was explicitely desired, however I am not sure it made more sense.
Turns out that with refactoring a validation error that wasn't previously
reported is not reported:

In JPAPlugin.bind we try to find an entity from the DB if it has an ID, and
previously we didn't add any validation error if that entity was not found.
On the other hand, code that did the same thing but for relations like collections
or @*ToOne relations, DID produce an error on invalid IDs in GenericModel.
So toplevel invalid ID was no error, but nested invalid ID was error.
With the refactoring now both toplevel and nested entity binding from the DB
use the same code, which adds the error, making it more consistent.

Therefore, I have dropped the invalid ID from the binding tests to make
sure we did not get any error about that since we wanted to validate the
Color enum binding.

And I added a new test to make sure that invalid IDs do get us an error.
@pepite
Copy link
Contributor

pepite commented Nov 29, 2011

This has been somehow merged and enhanced in play 1.2.4

@pepite pepite closed this Nov 29, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants