json: address nested members, and remove them - #141
Open
hammad-ikhlaq-dubbizlelabs wants to merge 2 commits into
Open
json: address nested members, and remove them#141hammad-ikhlaq-dubbizlelabs wants to merge 2 commits into
hammad-ikhlaq-dubbizlelabs wants to merge 2 commits into
Conversation
`JsonGet` and `JsonSet` built the path by concatenating '$.' with the key, so
the member name landed in the path unquoted. MySQL and MSSQL only accept that
bare form for a name that happens to look like an identifier: a dot, a dash or
a space in the name produces an illegal path expression and the server rejects
the whole statement. `Arel.json(col).get('floor-number')` was not addressable
at all.
Both accept the quoted form, `$."member"`, for any name, so the name is now
always quoted, with `"` and `\` escaped for the path parser. The connection's
own quoting round-trips the value it is given, so the two levels of escaping
compose.
The path is also emitted as one string literal rather than a concatenation
whenever the member name is known when the query is built, which MSSQL needs:
`JSON_VALUE` only accepts a literal or a variable as its path argument, never
an expression. A name the server computes - a column, a concatenation - still
goes through `CONCAT`, now into the quoted form too.
While here, an integer index rendered as `"$[0]"` with double quotes, which is
a string only while `ANSI_QUOTES` is off in MySQL and is an identifier in MSSQL
under the default `QUOTED_IDENTIFIER`. It is now a normal string literal.
The existing json coverage lives in `all_agnostic_test.rb#test_json`, which
opens with an unconditional `skip`, so none of it runs in CI. The tests here go
through the MySQL visitor at the to_sql level instead, which `test:to_sql`
already runs on every matrix entry; `FakeRecord::Connection` gained the two
methods the dialect version gates ask for so that is possible.
Postgres needs no change: it addresses members with `->>` and `jsonb_set(...,
array[...])`, where the name is a value rather than part of a path string.
Two things a json document could not be asked for.
**A path with more than one segment.** `get` and `set` took a single member
name, so a member of a nested document was only reachable by composing calls -
and on MSSQL not even then, because `JSON_VALUE` returns a scalar, so a second
`get` has nothing to walk into. An Array is now a path:
Arel.json(col).get(['dynamic_fields', 'beds'])
# mysql: JSON_EXTRACT("col", '$."dynamic_fields"."beds"')
# pg: "col"::jsonb #>> array['dynamic_fields', 'beds']
# mssql: JSON_VALUE("col", '$."dynamic_fields"."beds"')
An Array is always a path and never a member name: a name is a string, an
integer index, or an expression the server evaluates, and those can be mixed
freely - `['rooms', 0, 'size']` is `$."rooms"[0]."size"`. A single-segment path
renders exactly as it did before, `->>` on postgres included.
**Removal.** There was no way to express one at all, so `remove` is new, taking
any number of paths:
Arel.json(col).remove(['dynamic_fields', 'beds'], 'top')
# mysql: JSON_REMOVE("col", '$."dynamic_fields"."beds"', '$."top"')
# pg: (("col"::jsonb #- array['dynamic_fields', 'beds']) #- array['top'])
`JSON_REMOVE` takes every path in one call; `#-` takes one, so several chain.
Removing nothing is the document itself rather than invalid SQL, the way
`merge({})` already is. MSSQL is left out: it removes a member by assigning
`NULL` through `JSON_MODIFY`, which is the same function it would need for
`JsonSet`, and it has no `JsonSet` today either.
On postgres a jsonb path is a `text[]`, so an integer segment is rendered as
text. `array[0]` is an `int[]`, which no jsonb function accepts, so `set(0, v)`
could not have run before this.
`JsonGet#key` and `JsonSet#key` are kept as readers, returning the single
segment for a one-segment path. Visitors now read `path`.
Note that a nested `set` still does nothing when an intermediate level is
missing - `JSON_SET` will not create one, and `jsonb_set`'s `create_missing`
only covers the last segment. Making an assignment mean an assignment needs the
container bootstrapped in the same expression, which is a separate change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #140: this builds on the path builder that PR introduces, so the branch carries both commits and the diff against
mastershows both. Merge #140 first and this one collapses to its own commit —22011b4is the only one that belongs to this PR. Happy to rebase, squash the two together, or split differently, whatever suits how you want to take them.Two things a json document could not be asked for.
A path with more than one segment
getandsettook a single member name, so a member of a nested document was only reachable by composing calls — and on MSSQL not even then:JSON_VALUEreturns a scalar, so a secondgethas nothing left to walk into. AnArrayis now a path:An
Arrayis always a path and never a member name — a name is a string, an integer index, or an expression the server evaluates — and segment kinds mix freely, so['rooms', 0, 'size']is$."rooms"[0]."size". A segment the server computes still gets spliced in there, and only the literal runs around it are folded into the path string:A single-segment path renders exactly as it did before,
->>on postgres included, which is what theshould keep addressing a single member the way it didtest pins down — it is the one new test that passes without thelib/change.Removal
There was no way to express one, so
removeis new, taking any number of paths:JSON_REMOVEtakes every path in one call;#-takes one, so several chain. Removing nothing is the document itself rather than invalid SQL, the waymerge({})already is.MSSQL is left out on purpose: it removes a member by assigning
NULLthroughJSON_MODIFY, which is the same function it would need forJsonSet— and it has noJsonSettoday either. That felt like one coherent piece of work rather than half of it here.Smaller things that fell out
text[], so an integer segment is rendered as text.array[0]is anint[], which no jsonb function accepts, soset(0, v)could not have run at all before this.JsonGet#keyandJsonSet#keyare kept as readers, returning the single segment for a one-segment path and the whole path otherwise. Visitors readpathnow. Shout if you'd rather they were dropped outright.Deliberately not here
A nested
setstill does nothing when an intermediate level is missing:JSON_SETwill not create one, andjsonb_set'screate_missingonly covers the last segment. Soset(['a', 'b'], 1)on{}is a no-op on both backends rather than{"a":{"b":1}}.Making an assignment mean an assignment needs the container bootstrapped inside the same expression — on MySQL roughly
JSON_SET(COALESCE(col, '{}'), '$."a"', COALESCE(JSON_EXTRACT(col, '$."a"'), JSON_OBJECT()), '$."a"."b"', 1). That is a semantic choice about whatsetpromises, not a rendering detail, so it seemed better asked than assumed. Glad to follow up with it if you want that behaviour.Verification
bundle exec rake test:to_sql— 52 runs, 261 assertions, 0 failures (45 after #140). Withlib/reverted, 6 of the 7 new tests fail: 3 failures + 3 errors.Postgres and MySQL are both asserted at the to_sql level, since
all_agnostic_test.rb#test_jsonis skipped unconditionally and does not run in CI. I have no server to hand, so nothing here has been executed against a real MySQL, postgres or MSSQL — the SQL is reasoned from the docs for each, and the MSSQL side is by symmetry with MySQL.