#21 ✓opinion
Erwan Loisant

Add option to validate entities loaded from a file by Fixtures.load

Reported by Erwan Loisant | October 5th, 2010 @ 11:56 AM

Currently Fixtures.load will take in entities from a given file as-is. This means that entities which may normally not validate are loaded into the database. This is a problem because (at least for me) it's fairly easy for me to forget a line for an entity, and I'd like to know at the time it's loaded I oopsed, not when one of my app pages blows up because a required field was null.

Therefore I've attached a patch that validates an entity after it is loaded and throws an error containing the entity type, id, and error messages if it fails. Considering that not everyone might want this and for backwards compatibility, this feature does not run by default and can be turned on by putting "fixtures.validate=true" in the application.conf file.

As to my patch, if there's a better way within Play! of figuring out whether a conf-file value is true, please feel free to change that. I couldn't see a nice way other than simple String comparison as the closest solution would have been getBoolean("fixtures.validate", false) if Play.configuration is was a play.utils.Properties not a java.util.Properties instance.

Imported from Launchpad: https://bugs.launchpad.net/play/+bug/603843

Comments and changes to this ticket

  • Art McBain

    Art McBain November 9th, 2010 @ 08:36 AM

    Importing patch from Launchpad.

  • Guillaume Bort

    Guillaume Bort November 13th, 2010 @ 04:13 PM

    • State changed from “new” to “opinion”

    Please

  • Art McBain

    Art McBain November 13th, 2010 @ 10:34 PM

    I honestly do not think I am the only person who might make mistakes in a Fixtures-loaded yaml file, hence this ticket. The feature the patch adds is optional. Application developers must manually enable it via an application config file entry to use it. Therefore the patch does not force my view on other Play! developers.

  • opensource21

    opensource21 November 14th, 2010 @ 12:47 PM

    Don't know what the state opinion means. However you are not the only one which is interested on this feature.

  • Guillaume Bort

    Guillaume Bort November 14th, 2010 @ 05:58 PM

    Yes why not. But I think it would be better to provide an alternate method, like Fixtures.validateAndLoad

  • Art McBain

    Art McBain November 14th, 2010 @ 06:34 PM

    Having a separate method would make it more apparent to developers it is available.

    In the end, whichever route this ends up going, I think my patch should be revisited a bit. If I recall correctly, some not so friendly things happen the next time after an aborted load because of a non-validating item. What I should have done was wrap all the loads while validated is enabled into a transaction, to be rolled back if one failed. Hopefully someone with a bit more Play!/JPA-foo could make that work.

  • Art McBain

    Art McBain November 15th, 2010 @ 12:03 AM

    Okay! Patch attached diffed with Fixtures.java from the current Play! 1.1 download. It copies the existing load method code into a loadAndValidate, wraps it in a transaction, and validates each loaded item. Failing validation or other exceptions cause the transaction to rollback. If The transaction is still active when the finally hits, it is committed.

    At this point, I'm actually not sure why the original load method isn't transactioned too ...

  • Art McBain

    Art McBain November 15th, 2010 @ 01:11 AM

    I apparently wasn't calling it when I thought I tested it earlier. Unless something changes the result between 1.0.3 and 1.1 (I applied it to my local 1.0.3 instance I still use for the moment), that patch will cause it to error out with "transaction already active" when you call loadAndValidate. I guess at this point, as I said earlier, someone with more Play!/JPA-foo needs to give it a whirl.

  • Art McBain

    Art McBain November 15th, 2010 @ 01:14 AM

    Er, I should clarify (I can't edit previous posts), I made the method first in 1.0.3, then downloaded 1.1, copied it's Fixtures.load to Fixtures.loadAndValidate, then inserted the necessary lines for the transaction and validation. That became the patch I uploaded. I just realized those lines (at least in 1.0.3), while they should work, do not for the error given above.

  • Art McBain

    Art McBain November 15th, 2010 @ 01:16 AM

    Ugh. Okay. The patch is based entirely on 1.1 code. Running a backport of only the transaction and validation lines in 1.0.3 results in "transaction already active".

  • Art McBain

    Art McBain November 15th, 2010 @ 01:38 AM

    Updated patch. Uses what seems to be the preferred method, JPA.setRollbackOnly().

  • Art McBain

    Art McBain November 15th, 2010 @ 04:36 AM

    Last patch update. Removes setRollbackOnly calls, they're unnecessary. Play handles that automatically.

  • Art McBain

    Art McBain February 5th, 2011 @ 11:57 PM

    Are there any further updates on inclusion of this feature? The last posted patch was ready and good to go. The way Fixtures is handled may to have changed slightly since then, but the code for validation (lines 47-57 of the Nov. 15th, 2010 patch) is really the only new code needed to be inserted into any appropriate method.

  • Art McBain

    Art McBain May 1st, 2011 @ 09:39 PM

    Any chance of this getting included? I still think it useful for ensuring only valid data is loaded when yaml files are used for default data.

    To facilitate ease of future updates, I think an appropriate approach might now be make all the existing load() methods private, with an optional boolean argument to validate. The public facing methods could be then called the same and call the private ones with validation argument as false. New ones could be written in the naming convention of loadAndValidate() that pass along true for that value. Otherwise, the existing items could just be modified to include the boolean argument directly.

    If you still think this is an "opinion", note that I'm not advocating all Fixtures items automatically validate, but rather this be an optional item application writers can choose to use.

  • Art McBain

    Art McBain May 1st, 2011 @ 09:40 PM

    (Fixed formatting):

    Any chance of this getting included? I still think it useful for ensuring only valid data is loaded when yaml files are used for default data.

    To facilitate ease of future updates, I think an appropriate approach might now be make all the existing load() methods private, with an optional boolean argument to validate. The public facing methods could be then called the same and call the private ones with validation argument as false. New ones could be written in the naming convention of loadAndValidate() that pass along true for that value. Otherwise, the existing items could just be modified to include the boolean argument directly.

    If you still think this is an "opinion", note that I'm not advocating all Fixtures items automatically validate, but rather this be an optional item application writers can choose to use.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2>Play framework</h2>

Play makes it easier to build Web applications with Java. It is a clean alternative to bloated Enterprise Java stacks. It focuses on developer productivity and targets RESTful architectures. Learn more on the <a href="http://www.playframework.org">http://www.playframework.org</a> website.<br><br>

<h2>Source code is hosted on github</h2>Check out our repository at <a href="http://github.com/playframework/play">http://github.com/playframework/play</a><br><br>

<h2>Contributing, creating a patch</h2> Please read the <a href="http://play.lighthouseapp.com/projects/57987/contributor-guide">contributor guide</a><br><br>

<h2>Reporting Security Vulnerabilities</h2> Since all bug reports are public, please report any security vulnerability directly to <em>guillaume dot bort at gmail dot com</em>.<br><br>

<h2>Creating a bug report</h2> Bug reports are incredibly helpful, so take time to report bugs and request features in our ticket tracker. We’re always grateful for patches to Play’s code. Indeed, bug reports with attached patches will get fixed far quickly than those without any.<br><br>

Please include as much relevant information as possible including the exact framework version you're using and a code snippet that reproduces the problem.<br><br>

Don't have too much expectations. Unless the bug is really a serious "everything is broken" thing, you're creating a ticket to start a discussion. Having a patch (or a branch on Github we can pull from) is better, but then again we'll only pull high quality branches that make sense to be in the core of Play.

Attachments

Pages