Conversation
Restores `json` sub-path constraints (e.g. `settings.approved`) in polymorphic join `where` clauses, which #290 made fail closed. This is the cheapest of the categories that PR disabled: it needs no per-branch join mechanism, only the JSON traversal each adapter already uses on the non-polymorphic query path. Each branch compiles its own traversal against its own `json` column, so the combined caller + access `where` still applies in full to every branch. A target collection without the `json` column reads as SQL NULL and resolves to a boolean constant derived from the operator alone, rather than borrowing another branch's field to sanitize the value. Shapes that cannot be compiled soundly stay fail closed: negated operators (`not_equals`, `not_in`, `not_like`), null and non-scalar values, mixed-type `in` lists, localized `json` fields, a sub-path that is a real column or has-many value table in another target, and adapters with no JSON traversal.
…name `getFieldByPath` does not narrow its field list when a segment's field has no subfields, so `title.tags` resolves to an unrelated top-level `tags` field. In the polymorphic plan builder that produced two wrong outcomes: - a `json` sub-path whose key matches another field name never reached the JSON handler at all, so only keys with no namesake worked - the bogus plan then looked for a has-many select value table that cannot exist and threw a raw `Error`, i.e. a 500 rather than a 400 `QueryError` The plan builder now confirms every ancestor segment is a container it can descend into before trusting the resolved field. A path that fails that check falls through to the `json` sub-path and unsupported-shape checks, so `settings.title` compiles as a JSON traversal and `title.tags` fails closed with a `QueryError`. Both new integration tests fail without this change — `title.tags` with the raw "has no value table" error, and `settings.title` by not matching.
Contributor
📦 esbuild Bundle Analysis for payloadThis analysis was generated by esbuild-bundle-analyzer. 🤖
Largest pathsThese visualization shows top 20 largest paths in the bundle.Meta file: packages/next/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_shared.json, Out file: esbuild/exports/shared.js
Meta file: packages/richtext-lexical/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_shared.json, Out file: esbuild/exports/shared_optimized/index.js
DetailsNext to the size is how much the size has increased or decreased compared with the base branch of this PR.
|
This branch has not been deployed
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
1 participant
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.
Written by AI
Draft. Stacked on #18235 — follow-up to 9339d63 (PYLD-3840). Restores another of the polymorphic-join
whereshapes that 9339d63 made fail-closed. Base is #18235 branch so the diff shows only this change; retarget to3.xonce #18235 lands or is dropped.What this restores
Constraints that reach into a
jsonfield, e.g.{ 'settings.approved': { equals: true } }. Of the categories 9339d63 disabled this is the cheapest: it needs no per-branch join mechanism, only the JSON traversal each adapter already uses on the non-polymorphic query path (createJSONQueryon postgres,convertPathToJSONTraversalon sqlite).Each branch compiles its own traversal against its own
jsoncolumn, so the combined caller + accesswherestill applies in full to every branch — nothing dropped or truncated (no PYLD-3840 regression).A target collection that has no
jsoncolumn at the path reads as SQL NULL and resolves to a boolean constant derived from the operator alone, rather than borrowing another branch's field to sanitize the value. That borrow is the cross-cutting fail-open hazard called out in the plan tracked on themainPR #18233, and this is the first category to avoid it.Shapes that cannot be compiled soundly stay fail-closed:
not_equals,not_in,not_like) — on an absent branch "key missing" and "value does not match" are indistinguishable, so a negated constraint could widen the access rule it came frominlistsjsonfieldsStill deferred
Localized fields,
array/blocksrows,relationship/uploadtraversal and the geo operators remain fail-closed. The per-category plan — including the text-vs-number fail-open hazard (Number('x')→NaN→null→IS NULLmatching all value-less rows) that must be guarded before category 5 can ever be enabled — is tracked in #370.One correction to that plan worth recording: the
equals+ sanitized-null →IS NULLconversion is not polymorphic-specific.parseParams.ts:379does the same thing, so{numberField: {equals: 'x'}}already matches all value-less rows on the ordinary query path. It remains the right blocker for mixing text and number branches (one shared value coercing to null on just the number branch is new), but the guard belongs upstream inparseParams, not here.Verification
packages/drizzle/src/find): 106 pass (was 85)test/joins/int.spec.ts: 118 pass / 14 skip (sqlite), 120 pass / 12 skip (postgres) — exactly +4 tests over the base branch, no pass→skip drifttsc, eslint and prettier cleanIndependently written and run against #18235 branch. Left as a draft for review — not for merge.
Companion: #18235 (base), and the follow-up that restores localized and separate-row paths on top of this one.
Follow-up fix (second commit)
Review of this branch turned up a bug that made the feature only half-work.
getFieldByPathdoes not narrow its field list when a segment's field has no subfields, sotitle.tagsresolves to an unrelated top-leveltagsfield. Two consequences in the plan builder:jsonsub-path whose key matched another field name never reached the JSON handler, so only keys with no namesake worked —settings.approvedcompiled,settings.tagsdid notError, i.e. a 500 rather than a 400QueryError, reachable from an access ruleThe plan builder now confirms every ancestor segment is a container it can descend into before trusting the resolved field. Both added integration tests fail without the change —
title.tagswith the raw "has no value table" error, andsettings.titleby not matching.Updated verification: unit 108 pass,
test/joins/int.spec.ts120 pass / 14 skip (sqlite) and 122 pass / 12 skip (postgres).Known issue inherited from the base
Not fixed here: the base branch has a separate fail-open on
containsover amixedSelectpath.containsis rewritten to an exact match only whenfield.hasMany, so the single-select branch compiles to a substringilikeand admits any option value containing the permitted one as a substring on SQLite, while failing outright against a Postgres enum column. A tested patch for that exists but belongs on the base PR.