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

Deserialization issue #14

Closed
wants to merge 5 commits into from
Closed

Deserialization issue #14

wants to merge 5 commits into from

Conversation

boronine
Copy link
Contributor

See 2e4bc5b for failing test.
See 1d4a029 for fix.

@boronine
Copy link
Contributor Author

The fix is no good. Turns out the serialized value is a Python string (with unicode literals), not a JSON string. I'm gonna fix the test (to make it fail again).

@boronine
Copy link
Contributor Author

Okay, now the value is serialized with JSON as well. Test passes.

@niwinz
Copy link
Member

niwinz commented Jun 18, 2013

Hi!

Thanks for your efforts. But, your approach is wrong. Because:

I need some test project for reproduce this strange behavior but this can not be fixed with this workaround.
This seems fixes this stranges behavior for some people but is not a good solution because a reasons that I explain before.

;)
Andrey

@boronine
Copy link
Contributor Author

Hi Andrey,

  1. Yes, Hstore is not JSON, but why can't it be treated as JSON? In order for Django fixtures to work the field needs to be serialized somehow. I think JSON is the best choice as it's supported by most languages and is secure (unlike eval or pickle, for example).
  2. Looking back at the issue, I don't think it's going to get fixed by this pull request, however this pull request definitely fixes the lack of serialization support for djorm-ext-store. We need fixtures for our project so we're currently using my patched version.
  3. I added a simple test for serialization support. It would be nice to test it more thoroughly, but I think this is better than nothing. Either way, the error is confusing, a NotImplementedError would be better than the current random exception (which happens, I think, from trying to iterate over a string as if it was a dict).

@niwinz
Copy link
Member

niwinz commented Jun 18, 2013

I' am completely confused, sorry. I have been spoken about other issue.

Your are right. I would like some time for review it. But currently I have very litle time for it.
I try review it sooner as possible.

Sorry and thanks for your work.

@boronine
Copy link
Contributor Author

No worries, and thanks for the project, we're finding it very useful :)

@niwinz
Copy link
Member

niwinz commented Jun 24, 2013

Now merged with some fixes for python3.
A lot of thanks.

@niwinz niwinz closed this Jun 24, 2013
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.

2 participants