Fixed #26511 -- Documented KeyTransform and KeyTextTransform #15956

AllenJonathan · 2022-08-12T19:24:48Z

No description provided.

AllenJonathan · 2022-08-12T19:47:35Z

Looks to me that there is no use in using KeyTextTransform alone. I think it should be Cast to TextField for it to have any functionality of text. Seems to me that this could be done implicitly.

felixxm · 2022-08-13T12:32:36Z

Looks to me that there is no use in using KeyTextTransform alone. I think it should be Cast to TextField for it to have any functionality of text. Seems to me that this could be done implicitly.

That's not true, see e.g. comment.

felixxm

@AllenJonathan Thanks 👍 I left initial comments.

felixxm · 2022-08-15T10:18:30Z

docs/topics/db/queries.txt

@@ -1009,6 +1009,63 @@ Unless you are sure you wish to work with SQL ``NULL`` values, consider setting
 Key, index, and path transforms
 -------------------------------

+.. class:: KeyTransform(key_name, *args, **kwargs)


Suggested change

.. class:: KeyTransform(key_name, *args, **kwargs)

.. class:: json.KeyTransform(key_name, *args, **kwargs)

I would move it to the end this paragraph (before the closing notes, warnings, and admonitions).

felixxm · 2022-08-15T10:21:06Z

docs/topics/db/queries.txt

+    in ``'"labrador"'`` instead of ``'labrador'``. So filtering
+    ``breed_name__contains='dor"'`` would also be true.
+
+.. class:: KeyTextTransform


I would move it to the end this paragraph (before the closing notes, warnings, and admonitions). Also, add the full signature.

felixxm · 2022-08-15T10:21:30Z

docs/topics/db/queries.txt

+    in ``'"labrador"'`` instead of ``'labrador'``. So filtering
+    ``breed_name__contains='dor"'`` would also be true.
+
+.. class:: KeyTextTransform


Suggested change

.. class:: KeyTextTransform

.. class:: json.KeyTextTransform(key_name, *args, **kwargs)

felixxm · 2022-08-15T10:22:30Z

docs/topics/db/queries.txt

+.. note::
+
+    This feature is currently supported only in postgresql databases.


We should rather document that on non-PostgreSQL backends it's and alias to KeyTransform.

felixxm · 2022-08-15T10:23:55Z

docs/topics/db/queries.txt

+    Casting a resulting KeyTransform might still be in resemble json format.
+    For example, ``Cast(KeyTransform('breed', 'data'), TextField())`` would result
+    in ``'"labrador"'`` instead of ``'labrador'``. So filtering
+    ``breed_name__contains='dor"'`` would also be true.


This is not true for all databases.

felixxm · 2022-08-15T10:25:24Z

docs/topics/db/queries.txt

+
+.. warning::
+
+    Casting a resulting KeyTransform might still be in resemble json format.


KeyTransform should be backticked everywhere.

felixxm · 2022-08-15T10:26:35Z

docs/topics/db/queries.txt

+    ...     breed_name=Cast(KeyTransform('breed', 'data'), TextField())
+    ...     ).filter(breed_name__contains='lab')


Fix indentation (everywhere):

Suggested change

... breed_name=Cast(KeyTransform('breed', 'data'), TextField())

... ).filter(breed_name__contains='lab')

... breed_name=Cast(KeyTransform('breed', 'data'), TextField())

... ).filter(breed_name__contains='lab')

felixxm · 2022-08-15T10:27:05Z

docs/topics/db/queries.txt

+    ...         )).filter(owner_other_pet_name__exact=[{'name': 'Fishy'}])
+    <QuerySet [<Dog: Rufus>]>
+
+To query the resulting json as text, the .. class:: Cast function needs to be


.. class:: Cast is not a valid directive in Sphinx.

Please fix all invalid directives, e.g. .. class:: json.KeyTextTransform or .. class:: json.KeyTransform.

AllenJonathan · 2022-08-16T15:56:35Z

Is there a way to run all the doc tests locally?

felixxm · 2022-08-16T17:46:40Z

Is there a way to run all the doc tests locally?

You can use make html or make spelling, see docs.

charettes · 2022-08-17T16:39:21Z

docs/topics/db/queries.txt

+To query the resulting json as text, the :class:`~django.db.models.functions.Cast`
+function needs to be used. This is done as follows::
+
+    >>> Dogs.objects.annotate(
+    ...     breed_name=Cast(KeyTransform('breed', 'data'), TextField())
+    ... ).filter(breed_name__contains='lab')
+    <QuerySet [<Dog: Rufus>]>


I think this is bad advice as it will result in quotes around the string being included? Only KeyTextTransform should be used for that really.

charettes · 2022-08-17T16:47:54Z

docs/topics/db/queries.txt

+.. note::
+
+    .. class:: json.KeyTextTransform is alias to .. class:: json.KeyTransform for
+    non-postgresql databases.


Is this an oversight? I'm surprised to learn that.

Should we make sure that KeyTextTransform actually results in text (or whatever database specific type is used to represent strings) before making it public?

It seems the ->> operator is supported on MySQL 5.7+ and on other backend we could make sure that the return value is automatically wrapped in a CAST (e.g. a CAST over json_value on Oracle)

It works on SQLite and Oracle as a side-effect of the ticket-32213. I've prepared a fix for MySQL, see #15970.

AllenJonathan · 2022-08-23T12:44:55Z

Should the earlier ways to do path, index, and key transforms on JSONfield be deleted? Or at least it should be specified which way is recommended.

charettes · 2022-08-24T05:24:21Z

Last minute check but should we consider changing the signature of KeyTransform and KeyTextTransform to (source, *keys) instead of (key, source)? It has always seemed backward to me and once the API is public it's hard to change things of this nature.

If this is not possible for backward compatibility reasons should we consider exposing aliases à la F to hide this weird implementation detail?

What about

class KeyTransform(Transform):
    ...
    @classmethod
    def from_lookup(cls, lookup):
        transform, *keys = lookup.split(LOOKUP_SEP)
        if not keys:
            raise ValueError("Missing keys")
        for key in keys:
            transform = cls(key, transform)
        return transform

K = KeyTransform.from_lookup
KT = KeyTextTransform.from_lookup

It seems like these K and KT helpers would make for a way nicer public API that mimics the way values, filter and friends allow a __ notation for key accesses.

e.g. to take the documented examples

Dogs.objects.annotate(
    owner_name=KT('data__owner__name')
).filter(owner_name="Bob")

charettes · 2022-08-24T05:51:16Z

Actually it seems like K = KeyTransform.from_lookup would not provide any value over F as the latter can already be used for the examples added here in a more ergonomic way through JSONField.get_transform.

The usefulness of KT = KeyTextTransform.from_lookup remains though as that's not something that can be achieved through F. Maybe we only need to introduce KT and document it then?

Any thoughts @AllenJonathan and @felixxm? Allen, can you confirm that using F("data__breed") produces the same thing as KeyTransform('breed', 'data') in your examples.

AllenJonathan · 2022-08-27T05:58:27Z

Yes, F(data_breed) uses KeyTransform to build the query and has the same results. I think the changes you suggested would be good. Nesting KeyTransforms didn't look so good, and using __ makes it easier to users.

I could do the above changes. Should that be a new ticket and a PR? I think this ticket could be put on hold till these changes are made.

charettes · 2022-08-28T23:44:46Z

Agreed, F already supports transforms so no need for K but exposing KT still seems like something worth doing one way or the other. Even only documenting KeyTextTransform.from_lookup seems like a better UX than nesting KeyTextTransform with the reverse (key, expr).

Any thoughts on the subject @felixxm?

felixxm · 2022-08-29T06:00:43Z

Agreed, let's add and document only the alias KT.

Also, we recently documented hstore.KeyTransform(). I think we should revert 7faf25d and wontfix'ed ticket-30711 as we can achieved the same with F() so there is no need to expose an extra API.

…icket_26511

AllenJonathan · 2022-09-09T16:02:18Z

Is assuming KT as a class correct? Should I add that KT is a constructor for KeyTextTransform?
Is there anything else that has to be added?

AllenJonathan · 2022-09-09T16:10:39Z

Final Work Submission - Link

This is going to be my final work submission for GSOC. Please go through it and suggest changes or additions if any.

felixxm · 2022-09-12T08:59:22Z

@AllenJonathan Thanks 👍 Please move docs changes to the #16016, we should add and documented KT in a single commit.

AllenJonathan added 3 commits August 13, 2022 00:46

documents KeyTransform and KeyTextTransform

52d9588

small changes

3d1c6cc

mispelled word

6e41439

AllenJonathan mentioned this pull request Aug 12, 2022

Fixed #29799 -- Allowed registering lookups per field instances. #15787

Merged

felixxm reviewed Aug 15, 2022

View reviewed changes

unfinished changes

920ef3a

charettes reviewed Aug 17, 2022

View reviewed changes

AllenJonathan and others added 3 commits August 19, 2022 14:18

Merge branch 'django:main' into main

d565f55

nothing

e3fa8f3

none

7ead9f0

AllenJonathan added 5 commits August 23, 2022 18:28

small changes

85334f3

small changes

6143031

more changes

717fead

indent changed

30b2f3f

moves changes

3302cb0

felixxm mentioned this pull request Aug 31, 2022

Reverted "Fixed #30711 -- Doc'd django.contrib.postgres.fields.hstore.KeyTransform()." #16020

Merged

AllenJonathan added 3 commits September 8, 2022 17:56

Merge branch 'main' of https://github.com/AllenJonathan/django into t…

6749f13

…icket_26511

documents KT

3a9bfbf

small change

75a7227

felixxm closed this Sep 12, 2022

AllenJonathan deleted the ticket_26511 branch May 10, 2023 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #26511 -- Documented KeyTransform and KeyTextTransform #15956

Fixed #26511 -- Documented KeyTransform and KeyTextTransform #15956

AllenJonathan commented Aug 12, 2022

AllenJonathan commented Aug 12, 2022 •

edited

felixxm commented Aug 13, 2022

felixxm left a comment

felixxm Aug 15, 2022

felixxm Aug 15, 2022

felixxm Aug 15, 2022

felixxm Aug 15, 2022

felixxm Aug 15, 2022

felixxm Aug 15, 2022

felixxm Aug 15, 2022

felixxm Aug 15, 2022

felixxm Aug 15, 2022

felixxm Aug 16, 2022

AllenJonathan commented Aug 16, 2022

felixxm commented Aug 16, 2022

charettes Aug 17, 2022

charettes Aug 17, 2022 •

edited

felixxm Aug 18, 2022

AllenJonathan commented Aug 23, 2022 •

edited

charettes commented Aug 24, 2022 •

edited

charettes commented Aug 24, 2022 •

edited

AllenJonathan commented Aug 27, 2022

charettes commented Aug 28, 2022

felixxm commented Aug 29, 2022

AllenJonathan commented Sep 9, 2022 •

edited

AllenJonathan commented Sep 9, 2022

felixxm commented Sep 12, 2022

	.. class:: KeyTransform(key_name, args, *kwargs)
	.. class:: json.KeyTransform(key_name, args, *kwargs)

	.. class:: KeyTextTransform
	.. class:: json.KeyTextTransform(key_name, args, *kwargs)

		.. note::

		This feature is currently supported only in postgresql databases.


		.. warning::

		Casting a resulting KeyTransform might still be in resemble json format.

		... breed_name=Cast(KeyTransform('breed', 'data'), TextField())
		... ).filter(breed_name__contains='lab')

Fixed #26511 -- Documented KeyTransform and KeyTextTransform #15956

Fixed #26511 -- Documented KeyTransform and KeyTextTransform #15956

Conversation

AllenJonathan commented Aug 12, 2022

AllenJonathan commented Aug 12, 2022 • edited

felixxm commented Aug 13, 2022

felixxm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllenJonathan commented Aug 16, 2022

felixxm commented Aug 16, 2022

Choose a reason for hiding this comment

charettes Aug 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllenJonathan commented Aug 23, 2022 • edited

charettes commented Aug 24, 2022 • edited

charettes commented Aug 24, 2022 • edited

AllenJonathan commented Aug 27, 2022

charettes commented Aug 28, 2022

felixxm commented Aug 29, 2022

AllenJonathan commented Sep 9, 2022 • edited

AllenJonathan commented Sep 9, 2022

felixxm commented Sep 12, 2022

AllenJonathan commented Aug 12, 2022 •

edited

charettes Aug 17, 2022 •

edited

AllenJonathan commented Aug 23, 2022 •

edited

charettes commented Aug 24, 2022 •

edited

charettes commented Aug 24, 2022 •

edited

AllenJonathan commented Sep 9, 2022 •

edited