Skip to content

Conversation

laymonage
Copy link
Contributor

@laymonage laymonage commented Jul 26, 2020

Ticket #31829.

I made a mistake and created a KeyTransformContains that's based on lookup.Contains. This makes the contains lookup to not use JSON containment logic when it's performed on a KeyTransform. Therefore, it's incompatible with the previous PostgreSQL implementation, which uses the overridden lookup.

This PR fixes that by removing KeyTransformContains. The custom JSON_CONTAINS function on SQLite has also been extended to follow the rules of JSON_CONTAINS on MySQL. On Oracle, the implementation is turned into a chained KeyTransformExact lookup if the rhs is a dict, otherwise it falls back to the builtin lookup.Contains.

I also added notes to the docs, but I'm not sure if that's necessary.

@laymonage laymonage force-pushed the ticket_31829 branch 2 times, most recently from 6121960 to 760c73c Compare July 26, 2020 12:00
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laymonage Thanks 👍 I left comments.

@felixxm felixxm changed the title Fixed #31829 -- Changed contains lookup for KeyTransform to use overridden lookup. Fixed #31829 -- Used JSONField __contains lookup on key transforms. Jul 28, 2020
@felixxm
Copy link
Member

felixxm commented Jul 28, 2020

@laymonage Thanks 👍 I rebased from master and pushed small edits.

@felixxm felixxm merged commit 2d8dcba into django:master Jul 28, 2020
@@ -695,8 +714,11 @@ def test_contains_contained_by_with_key_transform(self):
)),
),
]
# PostgreSQL requires a layer of nesting.
if connection.vendor != 'postgresql':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a feature flag as CockroachDB (which tries to emulate PostgreSQL in a lot of ways) has the same restriction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, Do you have any name suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal at #13255.

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.

3 participants