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

Fix compound arrays #464

Closed
wants to merge 2 commits into from
Closed

Fix compound arrays #464

wants to merge 2 commits into from

Conversation

alno
Copy link
Contributor

@alno alno commented Sep 17, 2013

Calling type_cast for array elements, which fixes serialization of int arrays from strings and compound arrays (for example, arrays of json objects).

Also fixes deserializing compound arrays (calling type_cast for deserialized element)

@kares
Copy link
Member

kares commented Sep 17, 2013

Thanks a lot Alex, but I'm wondering ... is this actually supported by ("native") AR itself ?

@alno
Copy link
Contributor Author

alno commented Sep 17, 2013

By my tests deserialization problem isn't present in native AR.

About serialization - there is same issue there. With several open PRs:

I have no idea why no one of these is merged.

If it's preferred to maintain bug-to-bug compatibility, it may be better not to merge this PR until it will be fixed in native AR.

@kares
Copy link
Member

kares commented Sep 17, 2013

I see ... thanks a lot for the links ... I'm a bit hesitant esp. since you mentioned the [ JSON ] case.

Seems a bit "over-engineering" to me - the meta-data returned by the DB does not provide anything like that and I feel like following what the DB gives us is a good idea (anything more is a bit of a magic) ... while numbers might be fine compound JSON seems like too much - it will also be INconsistent with custom SELECTs where AR column type information does simply not exists (note that nested arrays are not implemented due the same reasons since it's a bit of a "smart" parsing feature not something the DB supports - no one complained about it ... yet :).

But if AR does this than we likely will as well - so we'll wait if you don't mind.
Thank you very much for bringing this up!

@kares kares added the postgres label Aug 5, 2014
@kares kares added this to the 1.4.0 milestone Aug 24, 2014
@kares
Copy link
Member

kares commented Jul 25, 2015

this issue should now be irrelevant as its handled by the AR-type system introduced in 4.2

@kares kares closed this Jul 25, 2015
@kares kares removed this from the 1.4.0 milestone Jul 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants