#1409 confirmed

ValidationErrors#to_json doesn't get declared if requirements are done in wrong order (e.g. via bundler)

Reported by mark | September 11th, 2010 @ 01:45 AM

Title is pretty much explanatory. ActiveSupport overrides #to_json, leading to wrong json generated.


Notice how dm-validations is included after dm-serialize. Bundler doesn't know that dm-validations has some great stuff to be added from dm-serialize and thus can put their requires in any order.

I see some ways to fix this:
1. Add #as_json method to ValidationErrors. It should be there anyways. This way, AS will still override #to_json, but will also properly encode your errors
2. Ask yehuda to use some stable topological sort (I don't know if there is such a thing at all) thus making it possible to manually manage the requirement order
3. Move #to_json / #as_json code to dm-validations instead of dm-serialization -- possibly causes the same problem but reversed

Comments and changes to this ticket

  • mark

    mark September 11th, 2010 @ 01:52 AM

    NB: Please notice that you should add #as_json method or otherwise [i.errors] (array with i.errors) will get improperly encoded again.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) September 11th, 2010 @ 01:59 AM

    • State changed from “new” to “confirmed”

    I think that we should still ask Yehuda about optional dependencies in bundler, but we can't assume everyone using DM is going to be using bundler. Some people use isolate, some use rvm gemsets, some just rubygems directly and manage their deps by hand.

    When figuring out how dependencies should be structured I think you have to make sure they only go in one direction. So you pick one package, like dm-serializer to know about dm-validations only. It gets nasty when there's a bidirectional dependencies, so I want to avoid those. Another solution is to put something in between that knows about both packages, and they have no idea about each other.

    (I think I'd rule out this last solution except in rare cases because it would mean a huge increase in the number of deps for each gem)

    I don't know for sure if the current dependency (dm-serializer adding methods to dm-validations) is the ideal approach, it could the other direction be just as valid. I will add that IMHO it feels like the right direction based on my limited experience with the internals of dm-serializer. I think I would rather have dm-serializer know that the ValidationErrors object (when it exists) needs #to_json and #as_json, rather than adding that specific detail into dm-validations directly.

    The solution I would recommend is that we use the same approach as in dm-transactions, dm-constraints and dm-migrations where we test for the existence of a namespace and mixin into it immediately if it's present. Otherwise we add const_added callbacks that are manually triggered when the namespace is defined. These callbacks can perform the same mixin action, thus allowing us to have the same behaviour regardless of the require order.

  • mark

    mark September 11th, 2010 @ 03:31 AM

    Hmm, I probably should apologize to yehuda, since you actually can modify requirement order in bundler gemfile.

    Sorry again, this ticket is probably invalid.

    p.s. Defining #as_json for errors is still a good idea.

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 »

People watching this ticket