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

Model attributes stored with JSON coder don't deserialize (via reify) correctly #5

Open
npafundi opened this issue Jul 25, 2014 · 9 comments

Comments

@npafundi
Copy link
Contributor

This is a great gem! Even in the experimental state, it works incredibly well.

I found one issue when storing an attribute coded in JSON. When I reify from a draft, the attribute is returned as an encoded String rather than the expected JSON object.

As a brief example, assume I have a model with a settings attribute that looks something like this:

class Widget < ActiveRecord::Base
  store :settings, coder: JSON
end

In the database, the settings column is simply a text type field.

Assume I have a widget object and set its settings attribute to the following JSON object:
{"key1":"val1", "key2":"val2"}
If I run a draft_update, this will be saved into the drafts table correctly. Publishing also works as expected. However, reifying seems to return this object as a String rather than hash (or JSON object). widget.draft.reify.settings would return the following:
"{\"key1\":\"val1\", \"key2\":\"val2\"}"

I haven't yet found a workaround, but I'm still looking. If there's something simple you might recommend, let me know.

Thanks for all the development work you've put into draftsman!

@chrisdpeters
Copy link
Collaborator

Thanks for the compliment, @npafundi!

Looks like someone (perhaps I) would need to modify the reify logic to introspect whether or not the attribute has been configured to use a coder like you have. I'm surprised that the way it calls settings= in your case doesn't handle it automatically. (See https://github.com/live-editor/draftsman/blob/master/lib/draftsman/draft.rb#L200)

I must admit that I am not familiar with the store setting that you are using there. I'll have to look into that. I may also look to see if PaperTrail has done anything to address it too.

@npafundi
Copy link
Contributor Author

Thanks for the fast reply, @chrisdpeters!

I looked into this a little more, and the simplest solution that I could find was to call JSON.parse(widget.settings) after reifying in my controllers (or serializers). Doing this doesn't require changes to draftsman, of course.

This does seem like an odd issue -- I was also surprised that the settings= didn't work when looking at the reify code. I'm not very familiar with what options can be determined with introspection, but it seems difficult to determine what coder is being used on a specific attribute.

It may be possible to check the data type of the model attribute (the actual data type of the column), but that would require that the column's data type be set to json (we're using Postgres, which has a json data type: http://www.postgresql.org/docs/9.3/static/datatype-json.html). However, this is only a partial solution. It's still possible to use store with a JSON coder when the column is another data type (text, varchar, etc.), and those cases wouldn't be covered.

Since I wasn't able to find a way to determine the attribute's coder type, I decided to call JSON.parse on my end, which works (albeit not as elegantly).

All that said, I'm not sure if this is something that's worth fixing. It's probably a small subset of projects using this serialization technique, and there is a relatively simple workaround by parsing the returned string. I'll keep it in the back of my mind, but if you'd like to close the issue that's fine by me.

Again, thanks for putting this gem together!

@chrisdpeters
Copy link
Collaborator

I actually started working on adding support for the Postgres json datatype this morning. I'm mostly done with it, and I'd imagine it'll be done in a few days. You'd be more than welcome to help me battle-test it.

Do you think that would solve the issue for your situation?

P. S. I plan on trying this blog post's suggestion for converting my text columns storing JSON to json. I love any post that starts out with, "This is an 'I fucked up so you don’t have to' post." :)

@npafundi
Copy link
Contributor Author

That's awesome.

It may work, depending on how you plan on handling the loading of those json objects. In my particular case, I believe I'd need the json attributes to not be serialized into a String, or to be parsed back out after reifying.

I'm not quite sure, but I'd love to see what you have and see if it'll fit in. Worst case, I can still run the parse on my end.

I'll probably have to make some adjustments to our code either way. Let me know when you have something, and I'll give it a shot.

Thanks!

@chrisdpeters
Copy link
Collaborator

I am about to commit some code that adds PostgreSQL JSON support for the object, object_changes, and previous_draft columns. After some thought about your issue, I don't think this change will help you though. My change applies to the draft data, not your application's unique models.

On a side note, I'm wondering if changing your settings column to JSON would fix the issue. Would Rails then automatically deserialize the JSON string into a hash? If not, I think your workaround is the most sensible approach for the time being. It bugs me that it's not automatic though...

@npafundi
Copy link
Contributor Author

Hey Chris, sorry for the slow reply. I had the same thought, and earlier today I reworked some of our code and migrated to the Postgres json data type. Good news - it seems to be working great! My testing has been limited, but from what I've seen, it looks like everything is serializing and deserializing correctly.

Because this removes our dependence of serializing to/from JSON using a coder, it looks like it fixed the whole issue. This may not work for MySQL users, but they should be able to use the workaround we discussed.

I'm still looking forward to pulling in your new changes. I haven't fully tested my own changes yet, so if anything related comes up, I'll let you know.

Thanks for the suggestions/solution!

@chrisdpeters
Copy link
Collaborator

You're welcome and no problem.

I just cut version 0.3.0 of the gem. I added support for PostgreSQL JSON because I needed to query some of the data within draft#object. Works very nice!

If you need to create a migration to convert object, object_changes, and previous_draft from text to json, again, I recommend using the query in this blog post.

Let me know if you experience any other issues or think the gem's API could be improved.

@chrisdpeters
Copy link
Collaborator

I figured out why this wasn't working. Draftsman calls write_attribute in several places, and that doesn't use the default accessor (in your case, settings=):
http://www.davidverhasselt.com/set-attributes-in-activerecord/

I'm working on fixing another issue, but I may try to get this fixed in the next release for our friends using databases other than PostgreSQL.

@chrisdpeters chrisdpeters reopened this Aug 11, 2014
@npafundi
Copy link
Contributor Author

Good find! I'm sure anyone who can't switch databases would find that useful.

Everything has been working great since we changed to the json datatype in PG. I haven't finished integrating draftsman completely (working on some other tasks), but so far, so good.

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

No branches or pull requests

2 participants