Skip to content
This repository has been archived by the owner on Apr 17, 2018. It is now read-only.

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

Open
solnic opened this issue May 17, 2011 · 3 comments

Comments

@solnic
Copy link
Contributor

solnic commented May 17, 2011

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

http://pastie.org/private/1mgbqwfskokwcs4htkccba

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

Created by mark - 2010-09-11 06:59:36 UTC

Original Lighthouse ticket: http://datamapper.lighthouseapp.com/projects/20609/tickets/1409

@solnic
Copy link
Contributor Author

solnic commented May 17, 2011

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

by mark

@solnic
Copy link
Contributor Author

solnic commented May 17, 2011

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.

by Dan Kubb (dkubb)

@solnic
Copy link
Contributor Author

solnic commented May 17, 2011

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.

by mark

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant