-
Notifications
You must be signed in to change notification settings - Fork 6
fix(sql-orm-client): selectIncludeStrategy reads namespaced capability flags #425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ad9c9ea
fix(sql-orm-client): selectIncludeStrategy reads capability flags from
saevarb 32cefcf
test(sql-orm-client): regression guards for namespaced capability
saevarb 5e32fab
test(sql-orm-client): preserve literal capability types in withCapabi…
saevarb 9b4ad81
test(sql-orm-client): force multi-query strategy in empty-parent test
saevarb 189a65e
Merge branch 'main' into fix/orm-client-include-strategy
saevarb 38cdaef
Merge branch 'main' into fix/orm-client-include-strategy
saevarb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
112 changes: 53 additions & 59 deletions
112
packages/3-extensions/sql-orm-client/test/include-strategy.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,82 +1,76 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { selectIncludeStrategy } from '../src/include-strategy'; | ||
| import { getTestContract } from './helpers'; | ||
| import { getTestContract, withCapabilities } from './helpers'; | ||
|
|
||
| // The default test contract has `target: 'postgres'`, `targetFamily: 'sql'`, | ||
| // and capabilities populated under those two namespaces. The strategy | ||
| // selector reads only those namespaces, so each test uses | ||
| // `withCapabilities(...)` to swap in the override the scenario needs. | ||
|
|
||
| describe('selectIncludeStrategy', () => { | ||
| it('returns multiQuery when include capabilities are absent', () => { | ||
| const contract = getTestContract(); | ||
| const strategy = selectIncludeStrategy(contract); | ||
| const contract = withCapabilities(getTestContract(), {}); | ||
|
|
||
| expect(selectIncludeStrategy(contract)).toBe('multiQuery'); | ||
| }); | ||
|
|
||
| it('returns correlated when jsonAgg is enabled in the family namespace without lateral', () => { | ||
| const contract = withCapabilities(getTestContract(), { | ||
| sql: { jsonAgg: true }, | ||
| }); | ||
|
|
||
| expect(strategy).toBe('multiQuery'); | ||
| expect(selectIncludeStrategy(contract)).toBe('correlated'); | ||
| }); | ||
|
|
||
| it('returns correlated when jsonAgg is enabled without lateral', () => { | ||
| const contract = { | ||
| ...getTestContract(), | ||
| capabilities: { | ||
| jsonAgg: { | ||
| enabled: true, | ||
| }, | ||
| }, | ||
| }; | ||
| it('returns lateral when both flags are enabled in the same namespace', () => { | ||
| const contract = withCapabilities(getTestContract(), { | ||
| postgres: { jsonAgg: true, lateral: true }, | ||
| }); | ||
|
|
||
| const strategy = selectIncludeStrategy(contract); | ||
| expect(strategy).toBe('correlated'); | ||
| expect(selectIncludeStrategy(contract)).toBe('lateral'); | ||
| }); | ||
|
|
||
| it('returns lateral when both lateral and jsonAgg are enabled', () => { | ||
| const contract = { | ||
| ...getTestContract(), | ||
| capabilities: { | ||
| lateral: { | ||
| enabled: true, | ||
| }, | ||
| jsonAgg: { | ||
| enabled: true, | ||
| }, | ||
| }, | ||
| }; | ||
| it('returns lateral when flags are split across family and target namespaces', () => { | ||
| // Real-world shape: SQL family declares `jsonAgg`; the postgres | ||
| // target adds `lateral` on top. | ||
| const contract = withCapabilities(getTestContract(), { | ||
| sql: { jsonAgg: true }, | ||
| postgres: { lateral: true }, | ||
| }); | ||
|
|
||
| const strategy = selectIncludeStrategy(contract); | ||
| expect(strategy).toBe('lateral'); | ||
| expect(selectIncludeStrategy(contract)).toBe('lateral'); | ||
| }); | ||
|
|
||
| it('reads object capabilities via enabled flag', () => { | ||
| const contract = { | ||
| ...getTestContract(), | ||
| capabilities: { | ||
| lateral: { enabled: true }, | ||
| jsonAgg: { enabled: false, fallback: true }, | ||
| }, | ||
| }; | ||
| it('ignores capability flags in unrelated namespaces', () => { | ||
| // The default test contract's target/family are 'postgres' / 'sql'. | ||
| // A `mongo: { lateral: true }` namespace must not enable lateral | ||
| // on a postgres runtime — namespaces are scoped to the running | ||
| // target/family. | ||
| const contract = withCapabilities(getTestContract(), { | ||
| mongo: { jsonAgg: true, lateral: true }, | ||
| nonsense: { lateral: true }, | ||
| }); | ||
|
|
||
| const strategy = selectIncludeStrategy(contract); | ||
| expect(strategy).toBe('lateral'); | ||
| expect(selectIncludeStrategy(contract)).toBe('multiQuery'); | ||
| }); | ||
|
|
||
| it('accepts top-level boolean capability flags', () => { | ||
| const contract = { | ||
| ...getTestContract(), | ||
| capabilities: { | ||
| lateral: true, | ||
| jsonAgg: true, | ||
| }, | ||
| } as unknown as ReturnType<typeof getTestContract>; | ||
| it('treats non-boolean capability values as missing', () => { | ||
| // The Contract type declares capability values as `boolean`. Anything | ||
| // else (string, object, undefined) is treated as not present. | ||
| // The cast on `'yes'` is deliberate — we're feeding an invalid value | ||
| // through a valid-typed contract to exercise the runtime check. | ||
| const contract = withCapabilities(getTestContract(), { | ||
| sql: { jsonAgg: 'yes' as unknown as boolean, lateral: true }, | ||
| }); | ||
|
|
||
| const strategy = selectIncludeStrategy(contract); | ||
| expect(strategy).toBe('lateral'); | ||
| expect(selectIncludeStrategy(contract)).toBe('multiQuery'); | ||
| }); | ||
|
|
||
| it('ignores non-boolean, non-object capability values', () => { | ||
| const contract = { | ||
| ...getTestContract(), | ||
| capabilities: { | ||
| lateral: 'yes', | ||
| jsonAgg: true, | ||
| }, | ||
| } as unknown as ReturnType<typeof getTestContract>; | ||
| it('treats explicit `false` as not enabled', () => { | ||
| const contract = withCapabilities(getTestContract(), { | ||
| sql: { jsonAgg: true, lateral: false }, | ||
| }); | ||
|
|
||
| const strategy = selectIncludeStrategy(contract); | ||
| expect(strategy).toBe('correlated'); | ||
| expect(selectIncludeStrategy(contract)).toBe('correlated'); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an old mistake in the emission process: capability flags should not be namespaced, they're either present or not, and we don't care which framework component contributes them. Your fix is more correct than the current implementation, but still working around this core mistake.