-
Notifications
You must be signed in to change notification settings - Fork 896
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
Add support for where_object and where_object_changes with JSON and JSONB columns #518
Add support for where_object and where_object_changes with JSON and JSONB columns #518
Conversation
On Ruby 2.2.1, '::YAML' returns the class Psych, which has no ::ENGINE constant defined.
Column types can (rarely) change dynamically as an application runs. We should check the type of a column on every query.
The tests pass for me locally. Not sure what's going on with Travis. |
@@ -118,12 +139,12 @@ def primary_key_is_int? | |||
|
|||
# Returns whether the `object` column is using the `json` type supported by PostgreSQL | |||
def object_col_is_json? | |||
@object_col_is_json ||= [:json, :jsonb].include?(columns_hash['object'].type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we shouldn't cache / memoize these?
Hmm.. I just upgraded to Postgres 9.4 on my local system and ran the test suite and saw the same errors that Travis is returning so I'll see if I can do some hacking to figure out what's up. Thanks for the pull request, let me know if you have any ideas regarding the invalid token errors. |
Thanks a lot for the pull request! This is super helpful. I was able to get all the tests to pass on Travis and even added some additional tests for good measure surrounding the column types. It turns out that |
Nice! Glad you got it to pass on Travis. |
Any plans on merging this PR into master? |
The commits are already in master. See, e.g. a98d88c GH doesn't make it easy to tell they're already in master, does it :) |
Thanks Jared, I did not bother to look in the actual file, and you're right, GH did not indicate that it was merged in |
No description provided.