This seems correct according to https://apex.oracle.com/database-features/

However, the function seems to be documented for 19c as well: https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/json_transform.html

According to https://stackoverflow.com/a/75286939 it was backported to 19c in 19.10. Could we update this and see if it works? According to https://code.djangoproject.com/wiki/CI we use v19.3.0.0.0 on the CI though, so that may need to be upgraded, or we can test ourselves.

We agreed on following the https://apex.oracle.com/database-features/ website.

There's a bit of round trip around the transforms, so let me explain the flow.

• We do MyModel.objects.update(json_field__key1="foo", json_field__key2__key2a=123)
• QuerySet.update() will construct an UpdateQuery which calls add_update_values() to prepare the expressions for each field that we are updating.
• In add_update_values(), we'll split the kwargs using LOOKUP_SEP.
  • Then, we ensure each name in the resulting split are valid transforms by doing a get_transform() on the field with each string to get the transform class. If it is found, we'll construct the transform. In JSONField's case, it will always give you KeyTransform (because keys can be of any string).
  • If the transform is chained, we keep on building the transforms by composing them, e.g. KeyTransform(KeyTransform(...)). For other fields, this may not be of the same class, e.g. TransformA(TransformB(...)). There's no concrete example for this right now, though.
  • After constructing the final Transform instance (the last one from the result of .split(LOOKUP_SEP)), we call its get_update_expression() method.
  • In KeyTransform, we use the unwrap_transforms so that we can "unwrap" the transform composition e.g. KeyTransform("key2a", KeyTransform("key2", "json_field")) back into field, ["key2", "key2a"].
  • Then, we join the keys with LOOKUP_SEP again, so we can pass it to JSONSet()/JSONRemove() as JSONSet(field, key2__key2a=value).
  • Then JSONSet()/JSONRemove() is going to – yet again –, split the kwargs with LOOKUP_SEP, because it needs to build the final JSON path string e.g. $.key2.key2a.
  • We can probably optimize the last bits by introducing a classmethod on JSONSet/JSONRemove that accepts a pair of a KeyTransform instance and the value...

Alternatively, we could also make it so that the field itself, rather than the transform, defines the expression. This lets us skip the round trip of splitting/joining on LOOKUP_SEP entirely.

For example, if this get_update_expression is implemented in JSONField (instead of KeyTransform), we could just pass the kwargs from .update() directly to the JSONSet() and JSONRemove() functions. This skips KeyTransform entirely.

This approach will result in a much more straightforward code flow. However, it would mean that any use of LOOKUP_SEP in update() is handled entirely by the field itself, rather than going through the transforms mechanism like in .filter().

I think, to make this extensible and more consistent with .filter(), we should do the transforms approach, even if it means doing a bit of round trips in the case of KeyTransform.

I like the idea of using a sentinel to differentiate between null and key deletions (it's something that I believe is missing for JSON null vs SQL NULL) bet I'm not sure we should piggy back on that one since it was meant for a completely different purpose.

I agree @charettes! This was a temporary solution as suggested by Markus the other day, and I linked to your exact comment: https://fosstodon.org/@laymonage/112949402890177435 (Like I said, big fan 😂)

Happy to introduce a new sentinel for this, although if we want to also support that sentinel in JSONField at the same time, that sounds like a separate work in itself.

I think the idea of making for_save a (enum.Enum, str) to differentiate between different scenario could could as most usage out there are doing if for_save and most likely don't need to differentite between both.

That's what I thought too – though I suppose it could still break if people check for the boolean explicitly e.g. if for_save is True. Still doable, just need to be more careful when we do the transition.

My immediate concerns with these changes are that they are possibly trying to do too much at once?

Is there a reason why we can't first land support for JSONSet / JSONRemove in a first set of changes targeted for 5.2, let them simmer for a release, and then consider adding support for the transform combining feature later on?

No particular reason, really. When we implemented the first pass for the JSONSet and JSONRemove functions, we were curious how much work it would take to implement the transforms support in update() directly. Turns out it wasn't much, but indeed it's a bit more "controversial".

Sarah and I discussed this at the DjangoCon US sprints and we decided to split this PR into two. One for the DB functions, and one for the nicer syntactic sugar via update() directly – just like you said. I noted this in my comment #18489 (review) just yesterday.

@adzshaf and I are still working on polishing the DB functions to be ready for a new, smaller PR. We found a few more edge cases so it's taking us a bit longer than we thought. We'll post an update on the ticket when we put the PR up 😄

I like the ideas put forward by this patch and the separation of concern between field and transform logic but these are more fundamental changes to the ORM works and bring larger questions. For example, should we support save(update_fields) with transforms as well? Is it time to introduce a specialized type of transform that denotes nested data structure access instead of trying to push the current abstraction in this space?

IMO these questions can likely be more easily answered while users are allowed to play with JSONSet and JSONRemove and these primitives are iterated on to iron out bugs first.

Thank you, and yes there are definitely more areas of the ORM that we haven't considered like bulk_{create,update}, update_or_create, etc. and save(update_fields) is one of them. I'd be happy to focus on getting JSONSet and JSONRemove in first, then discuss the more uncertain bits after.

Thanks for the update, it's great to see you push all of these JSONField improvement forward. It's has been a game changer to be able to use JSONField for non-Postgres database and a lot of that is thanks to you 🙇

I think that the two-commit approach has a lot of merit considering how hard it is to get these things right the first time. Let me know when things have shappen if you'd like more feedback. As I said already I think the proposed approach is great in the sense that it demonstrates that it can be done without too many invasive changes.

Could we extract the changes in Cast(), along with the corresponding test, into a separate commit please?

Done ✔️

Reason why we need this:

https://www.sqlite.org/json1.html#jset

If the value of a path/value pair is an SQLite TEXT value, then it is normally inserted as a quoted JSON string, even if the string looks like valid JSON. However, if the value is the result of another json function (such as json() or json_array() or json_object()) or if it is the result of the -> operator, then it is interpreted as JSON and is inserted as JSON retaining all of its substructure. Values that are the result of the ->> operator are always interpreted as TEXT and are inserted as a JSON string even if they look like valid JSON.

It's very similar to the other workarounds in as_mysql for MariaDB and in as_oracle. Instead of putting the workarounds directly in Cast.as_vendor(), do we want to introduce a new database operation specifically for casting JSONField instead? We have something similar for the datetime transforms:

Let's create a separate commit for JSONSet+tests, and another commit for JSONRemove+tests. Then, create a separate commit for adding KeyTransform.get_update_expression() and the corresponding tests.

Done ✔️

Value() with JSONField() would allow us to reuse its get_db_prep_value() which does the serialization of Python objects based on the DB vendor. Then, we wrap it in Cast() because this value will be used as a parameter to the DB's JSON_SET function, and some DBs (e.g. SQLite) will treat the value as a plain SQL string (even if it's valid JSON) if we don't use Cast.

This is unlike saving values directly to the column, in which case the DBs will happily accept a JSON-formatted string and do any conversion to the actual JSON data type (if they have one).

We don't need Cast() because Value() with a JSONField is correctly handled as JSONB by Postgres.

Casting to JSONB with ::jsonb on Postgres only works for strings (text) that are JSON-formatted. If you have an expression that resolves to a non-string SQL value, e.g. when you refer to an integer column with F(), using Cast() doesn't work. Postgres has a dedicated to_jsonb function for this: https://www.postgresql.org/docs/current/functions-json.html#FUNCTIONS-JSON-CREATION-TABLE

Interpolating the JSON path directly is nothing new:

Though I think we should move the comment to be outside the if, because this applies to both cases.

Using FORMAT JSON is similar to using Cast() in other DBs but this is specific to Oracle's JSON_TRANSFORM.

https://docs.oracle.com/en/database/oracle/oracle-database/21/adjsn/oracle-sql-function-json_transform.html

If the result expression evaluates to a SQL value that is not JSON type, you can convert it to JSON data by following the expression immediately with keywords FORMAT JSON. This is particularly useful to convert the SQL string 'true' or 'false' to the corresponding JSON-language value true or false. Example 11-7 illustrates this.