Fixed #36085 -- Added JSONField support for negative array indexing on SQLite. #19030
Conversation
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
7f48874 to
e5bb2d4
Compare
8b0ed71 to
8550319
Compare
django/db/models/fields/json.py
Outdated
| @@ -152,7 +152,10 @@ def compile_json_path(key_transforms, include_root=True): | |||
| path.append(".") | |||
| path.append(json.dumps(key_transform)) | |||
| else: | |||
| path.append("[%s]" % num) | |||
| if connection.vendor == "sqlite" and num < 0: | |||
There was a problem hiding this comment.
I'd rather delegate this to a DatabaseOperations method in case other database backends also need this hook. In fact, compile_json_path()might be a candidate to delegate to the database backend. The snowflake backend needed to override it.
There was a problem hiding this comment.
@timgraham I'm fairly confident I can move compile_json_path into [Base]DatabaseOperations, but I'm not sure whether this would be in scope for this PR. I could
- make it part of this PR
- modify this PR to only delegate the "numerical append" operation to
DatabaseOperations, and leave delegation/migration of the whole function to a separate PR & ticket - make a separate PR & ticket to migrate the whole function, then rebase this PR on top
I'm not that familiar with Django development conventions in this regard, but I'm happy to do either of the above, or another suggestion. Please advise!
There was a problem hiding this comment.
Now that compile_json_path is backend-specific, I think it makes sense to move it to DatabaseOperations. I'm not sure if it has to be a separate PR, but it definitely has to be in a separate commit.
I'd suggest keeping it in the same PR for now so it's easier to read the refactoring in context, but keep it in a separate commit so we can split the PR later if needed.
Though, if we only extract the whole compile_json_path to DatabaseOperations and not separate the "numerical" part to its own operation, we'll have to duplicate most of the logic for SQLite. So perhaps split the function into two operation methods? (I'm not sure myself...)
There was a problem hiding this comment.
Sounds good to me! I must have misunderstood the Django guidelines -- I thought that each PR must be a single commit. I will do the refactor work here, and ping you when it's ready for review.
There was a problem hiding this comment.
I think eventually it will need to be separate PRs. It's just that for refactoring we generally need to build a case for it and have an agreement before it can be accepted. Having the context in the same PR helps other reviewers understand the rationale for refactoring.
If you're happy with creating a new PR just for the refactoring and have this PR based on top of that one, I'm sure it's fine too. But as you incorporate any feedback in the refactoring PR, you'll have to keep this one in sync, which can be a bit more work compared to have just the one PR and split it when we're happy.
There was a problem hiding this comment.
Refactoring (with multiple commits) is sometimes included in the same PR as a feature. See #19073 for an example. I doubt there would be a need to split it unless it gets quite unwieldy to review.
dc6326e to
86f834b
Compare
|
Thanks everyone for your inputs! I sat on this for a while because I wasn't sure whether development was going to continue in #19182 or not, but I decided to finish it up here when that PR was closed. Thanks to @Hesham942 for taking the first shot at the suggested refactor. I took the liberty of cherry-picking the refactor commits from #19182 so that your contribution would appear in this PR. On top of that, I made the following changes:
Next stepsI believe that the The changes pertaining just to fixing ticket-36085 are spread across 138122b & d9c1252 (tests & feature flag) and 86f834b (SQLite-specific fix). We discussed keeping the changes together in one PR, or splitting them up into multiple ones. @timgraham & @laymonage I'm happy to do either as convenient/appropriate for Django, please let me know your thoughts on this. |
Thank you so much, @savanto! I really appreciate your understanding and for acknowledging my contribution. That means a lot! 🌟 |
4704507 to
34dd616
Compare
69ebd5f to
b60d89e
Compare
|
Fixed merge conflicts. |
|
Thanks @laymonage ! I've applied your suggested wording. |
a7018d1 to
58bb175
Compare
| .. admonition:: MySQL and MariaDB | ||
|
|
||
| Using negative JSON array indices is not currently supported. | ||
|
|
There was a problem hiding this comment.
Can you add a .. versionchanged:: 6.0 note for this and a release note for 6.0?
ce42c50 to
f58ea27
Compare
f58ea27 to
0306dcd
Compare
0306dcd to
f204a62
Compare
f204a62 to
b22427b
Compare
|
buildbot, test on oracle. |
f54991c to
e9930d1
Compare
|
buildbot, test on oracle. |
e9930d1 to
86469dc
Compare
|
buildbot, test on oracle. |
Trac ticket number
ticket-36085
Branch description
The SQLite backend has special syntax for performing negative-indexing in jsonfields that is different from other backends: negative indices must be prepended by a literal
#character.Refactor
This change includes a refactor to turn the function
django.db.models.fields.json.compile_json_pathinto a method of[Base]DatabaseOperations, to allow backend-specific customization. The refactor is cherry-picked from #19182, cleaned up, and modified to reduce duplicated code when customizing backend behavior.Checklist
mainbranch.