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

Read JSON::Any Columns as .to_json'd String #467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jon-sully
Copy link

Greetings 👋

Submitting this small PR as I recently ran into an issue when using import for a model. If I created the model one-off and used .new (add attribute values) then .save, the model would save just fine. If I instead set up a bunch of models then used .import, I'd get an error that stemmed down to this issue.

The short of it is that each adapter's import (pg's here) calls read_attribute under the hood and passes the then-read value along to the query builder but does so with its native type. If you ask a model for the value of a JSON::Any-typed column, you'll get a JSON::Any typed value. The query builder doesn't like that type though and will error.

This PR injects a .to_json in the process for any column whose type includes JSON::Any. This does indeed convert the value to a string and makes the query builder happy, but this PR may well be a proof of concept. There is definitely a better way of doing this, and this may overall be a stop-gap where correctly wiring up the Granite Converter(s) could be the right choice. Submitting this to explain my story, give some context, and hopefully inspire a better path forward!

Cheers!

@crimson-knight
Copy link
Member

Thanks for bringing this to my attention. I hadn't gotten notice of any PR's put on this repo, so I'm sorry about the late reply!

I'll have to dig into this more because I want to expand how Granite handles types and help improve it.

Have you joined our Discord yet? https://discord.gg/vwvP5zakSn

@crimson-knight
Copy link
Member

Closed by accident.

Ultimately this sounds like the import needs to be updated to handle bulk importing columns with JSON::Any types.

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.

2 participants