fix!: harden v2 beta release - #763
Merged
Merged
Conversation
Preserve existing TypeORM predicates, validate parser filters, reject malformed expression input, and correct SQL regex/dialect behavior. Prepare all public workspaces for synchronized beta publishing and add package license metadata. BREAKING CHANGE: relation aliases now use length-prefixed path segments to avoid collisions.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds asynchronous parser and URL codec APIs, schema-aware filter validation, expression parser hardening, deterministic SQL relation aliases, regex rendering updates, TypeORM predicate preservation, and beta release/package configuration. ChangesAsync parsing and codec flow
SQL and TypeORM behavior
Beta release and package metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/schema/parameter/filters/types.ts`:
- Around line 10-15: Reject asynchronous validator results at runtime to
preserve the synchronous Validator contract: update
packages/core/src/schema/parameter/filters/types.ts lines 10-15 and
packages/core/src/schema/parameter/filters/schema.ts lines 60-65 so
schema.validate detects Promise results and throws before they enter the filter
tree; update packages/core/src/parser/parameter/filters/validate.ts around line
38 to propagate that rejection rather than accepting the returned value.
In `@packages/typeorm/src/adapter/filters.ts`:
- Around line 150-155: Update TypeormAdapter.execute() and the filter-building
flow around queryBuilder.andWhere(sql, params) so repeated executions do not
accumulate adapter-owned predicates on a reused QueryBuilder. Reset or replace
only the adapter-owned WHERE fragment before applying new filters, while
preserving any application-owned baseline predicate already present on the
builder.
In `@packages/typeorm/test/unit/adapter/module.spec.ts`:
- Around line 91-102: Update the assertion in the empty-query test around
TypeormAdapter.execute to expect the configured driver’s parameter placeholder
syntax rather than the literal value 1, while continuing to verify that the
caller-owned where clause is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74fe824a-2ab9-4485-bcfc-6640b11aa144
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (67)
.agents/architecture.md.agents/conventions.md.agents/migration-notes.md.agents/references/typeorm-extension.md.agents/references/typeorm.md.github/workflows/release.yml.release-please-manifest.jsonLICENSEpackage.jsonpackages/codec-url-expression/LICENSEpackages/codec-url-expression/package.jsonpackages/codec-url-simple/LICENSEpackages/codec-url-simple/package.jsonpackages/codec-url/LICENSEpackages/codec-url/package.jsonpackages/core/LICENSEpackages/core/package.jsonpackages/core/src/parameter/filters/record/types.tspackages/core/src/parser/base.tspackages/core/src/parser/parameter/filters/index.tspackages/core/src/parser/parameter/filters/validate.tspackages/core/src/schema/parameter/filters/schema.tspackages/core/src/schema/parameter/filters/types.tspackages/core/test/unit/parameter/filters-validation.spec.tspackages/docs/guide/executing-queries.mdpackages/docs/guide/filters.mdpackages/docs/guide/migration-typeorm-extension.mdpackages/docs/package.jsonpackages/docs/packages/sql.mdpackages/docs/packages/typeorm.mdpackages/memory/LICENSEpackages/memory/package.jsonpackages/memory/src/parameter/filters/compiler.tspackages/parser-expression/LICENSEpackages/parser-expression/package.jsonpackages/parser-expression/src/parameter/filters/constants.tspackages/parser-expression/src/parameter/filters/module.tspackages/parser-expression/test/unit/parser/filters.spec.tspackages/parser-mongo/LICENSEpackages/parser-mongo/package.jsonpackages/parser-mongo/src/parameter/filters/module.tspackages/parser-mongo/test/unit/parser/filters.spec.tspackages/parser-simple/LICENSEpackages/parser-simple/package.jsonpackages/parser-simple/src/parameter/filters/module.tspackages/parser-simple/test/unit/parser/filters.spec.tspackages/sql/LICENSEpackages/sql/package.jsonpackages/sql/src/adapter/module.tspackages/sql/src/adapter/relations/base.tspackages/sql/src/adapter/relations/types.tspackages/sql/src/dialect/oracle.tspackages/sql/src/dialect/pg.tspackages/sql/src/helpers/relation-alias.tspackages/sql/src/visitor/filters.tspackages/sql/test/unit/adapter.spec.tspackages/sql/test/unit/interpreters/elem-match.spec.tspackages/sql/test/unit/interpreters/regex.spec.tspackages/sql/test/unit/interpreters/relation-nested.spec.tspackages/sql/test/unit/interpreters/relation.spec.tspackages/typeorm/LICENSEpackages/typeorm/package.jsonpackages/typeorm/src/adapter/filters.tspackages/typeorm/test/unit/acceptance.spec.tspackages/typeorm/test/unit/adapter/module.spec.tspackages/typeorm/test/unit/adapter/relations.spec.tsrelease-please-config.json
Comment on lines
+150
to
+155
| if (sql) { | ||
| // The builder may already carry an application-owned predicate | ||
| // (for example a tenant or authorization scope). Rapiq filters | ||
| // narrow that query; they must never replace its baseline WHERE. | ||
| this.queryBuilder.andWhere(sql, params); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Map the file and inspect the relevant area first.
ast-grep outline packages/typeorm/src/adapter/filters.ts --view expanded || true
echo '--- file excerpt ---'
sed -n '1,260p' packages/typeorm/src/adapter/filters.ts
echo '--- search for queryBuilder reuse/reset ---'
rg -n "queryBuilder|andWhere\(|where\(|filters.ts|new .*QueryBuilder|createQueryBuilder" packages/typeorm/src -SRepository: tada5hi/rapiq
Length of output: 10140
🏁 Script executed:
set -euo pipefail
# Narrow search to how this adapter instance is constructed and reused.
rg -n "class .*Adapter|extends .*Adapter|filters.ts|applyFilters|filter" packages/typeorm/src -S
# Inspect nearby files if referenced.
fd -a ".*adapter.*\.ts$" packages/typeorm/srcRepository: tada5hi/rapiq
Length of output: 1420
🏁 Script executed:
set -euo pipefail
# Show the relevant file with line numbers and any surrounding helpers.
cat -n packages/typeorm/src/adapter/filters.ts | sed -n '1,260p'
# Find where this adapter method is called.
rg -n "filters\.ts|apply.*filter|build.*filter|queryBuilder\.andWhere|queryBuilder\.where" packages/typeorm/src -SRepository: tada5hi/rapiq
Length of output: 5797
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("packages/typeorm/src")
targets = ["queryBuilder", "andWhere(", "where("]
for path in root.rglob("*.ts"):
text = path.read_text()
if any(t in text for t in targets):
print(f"\n## {path}")
for i, line in enumerate(text.splitlines(), 1):
if any(t in line for t in targets):
print(f"{i}: {line}")
PYRepository: tada5hi/rapiq
Length of output: 3294
🏁 Script executed:
set -euo pipefail
# Inspect the root adapter flow and the base filter state management.
sed -n '1,220p' packages/typeorm/src/adapter/module.ts
echo '--- search base adapter implementation ---'
rg -n "class FiltersBaseAdapter|clear\(\)|getQueryAndParameters|setChildAttributes|execute\(" . -S
# If the base implementation is vendored in the repo, inspect it too.
fd -a "filters.*\.ts$|base.*adapter.*\.ts$|adapter.*\.ts$" .Repository: tada5hi/rapiq
Length of output: 25209
Avoid appending onto a reused QueryBuilder. TypeormAdapter.execute() clears adapter state, but not the underlying TypeORM builder, so andWhere(sql, params) will accumulate duplicate predicates on repeated calls. Preserve the baseline scope, but reset or replace the adapter-owned WHERE fragment before reapplying filters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/typeorm/src/adapter/filters.ts` around lines 150 - 155, Update
TypeormAdapter.execute() and the filter-building flow around
queryBuilder.andWhere(sql, params) so repeated executions do not accumulate
adapter-owned predicates on a reused QueryBuilder. Reset or replace only the
adapter-owned WHERE fragment before applying new filters, while preserving any
application-owned baseline predicate already present on the builder.
Source: Learnings
Comment on lines
+91
to
102
| it('should preserve a caller-owned where clause for an empty query', () => { | ||
| const queryBuilder = dataSource | ||
| .getRepository(User) | ||
| .createQueryBuilder('user') | ||
| .where('user.id = :actorId', { actorId: 1 }); | ||
|
|
||
| const adapter = new TypeormAdapter({ queryBuilder }); | ||
|
|
||
| adapter.execute(new Query()); | ||
| expect(queryBuilder.getSql()).not.toContain('WHERE'); | ||
|
|
||
| expect(queryBuilder.getSql()).toContain('WHERE "user"."id" = 1'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the TypeORM SQL string assertion for the empty query test.
Similarly, queryBuilder.getSql() will not return the literal value 1 for the caller-owned condition. It will return the placeholder syntax for the configured driver.
🐛 Proposed fix
- it('should preserve a caller-owned where clause for an empty query', () => {
+ it('should preserve a caller-owned where clause for an empty query', () => {
const queryBuilder = dataSource
.getRepository(User)
.createQueryBuilder('user')
.where('user.id = :actorId', { actorId: 1 });
const adapter = new TypeormAdapter({ queryBuilder });
adapter.execute(new Query());
- expect(queryBuilder.getSql()).toContain('WHERE "user"."id" = 1');
+ const [sql, params] = queryBuilder.getQueryAndParameters();
+ expect(sql).toMatch(/WHERE "user"\."id" = (\?|\$1|:actorId)/);
+ expect(params).toEqual(expect.arrayContaining([1]));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should preserve a caller-owned where clause for an empty query', () => { | |
| const queryBuilder = dataSource | |
| .getRepository(User) | |
| .createQueryBuilder('user') | |
| .where('user.id = :actorId', { actorId: 1 }); | |
| const adapter = new TypeormAdapter({ queryBuilder }); | |
| adapter.execute(new Query()); | |
| expect(queryBuilder.getSql()).not.toContain('WHERE'); | |
| expect(queryBuilder.getSql()).toContain('WHERE "user"."id" = 1'); | |
| }); | |
| it('should preserve a caller-owned where clause for an empty query', () => { | |
| const queryBuilder = dataSource | |
| .getRepository(User) | |
| .createQueryBuilder('user') | |
| .where('user.id = :actorId', { actorId: 1 }); | |
| const adapter = new TypeormAdapter({ queryBuilder }); | |
| adapter.execute(new Query()); | |
| const [sql, params] = queryBuilder.getQueryAndParameters(); | |
| expect(sql).toMatch(/WHERE "user"\."id" = (\?|\$1|:actorId)/); | |
| expect(params).toEqual(expect.arrayContaining([1])); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/typeorm/test/unit/adapter/module.spec.ts` around lines 91 - 102,
Update the assertion in the empty-query test around TypeormAdapter.execute to
expect the configured driver’s parameter placeholder syntax rather than the
literal value 1, while continuing to verify that the caller-owned where clause
is preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/schema/parameter/filters/schema.ts`:
- Around line 75-78: Update the promise-handling branch around
this.options.validate(input) and isPromiseLike(output) to attach a no-op
rejection handler to output before throwing
SchemaError.validatorAsyncUnsupported(). Add a regression test covering a
validator that returns a rejected promise, ensuring the synchronous validation
path throws the expected SchemaError without an unhandled rejection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ff1d2d5-aa96-45c9-b5df-f9aecebd0ac4
📒 Files selected for processing (7)
.agents/architecture.mdpackages/core/src/errors/code.tspackages/core/src/errors/schema.tspackages/core/src/schema/parameter/filters/schema.tspackages/core/test/unit/parameter/filters-validation.spec.tspackages/docs/guide/errors.mdpackages/docs/guide/filters.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/test/unit/parameter/filters-validation.spec.ts
- .agents/architecture.md
- packages/docs/guide/filters.md
Owner
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Add explicit async parser and codec entry points while keeping synchronous methods deterministic. Preserve sync-only custom codec compatibility and document both execution paths.
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/typeorm/src/adapter/filters.ts (1)
150-155: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPrevent accumulating duplicate adapter-owned predicates on repeated executions.
Using
andWherecorrectly preserves the application's baseline predicates (like tenant or authorization scopes). However, as flagged in a previous review, if this adapter is executed multiple times on the sameQueryBuilder,andWherewill accumulate duplicate predicates because the previous run's adapter-ownedWHEREfragment is never removed.To resolve this while preserving the baseline, consider tracking the applied adapter condition (e.g., keeping a reference to the specific WHERE clause in
expressionMap.wheres) and selectively removing it before appending the new filters on subsequent runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/typeorm/src/adapter/filters.ts` around lines 150 - 155, Update the filter application logic in the adapter method containing this sql block to remove the previously applied adapter-owned WHERE entry from the query builder’s expressionMap.wheres before adding the new condition. Preserve all application-owned predicates, then append the current filters with andWhere so repeated executions replace only the adapter fragment rather than accumulating duplicates.
🧹 Nitpick comments (3)
packages/parser-simple/src/parameter/filters/module.ts (1)
78-87: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider parallelizing asynchronous filter validations.
Evaluating conditions in a
for...ofloop creates an asynchronous execution waterfall. Since conditions are mutually independent during validation, you can safely usePromise.allto validate them concurrently, which can notably improve throughput if schema validation delegates to database queries or external services.🚀 Proposed refactor to use `Promise.all`
- let items: ICondition[] = []; const parsed = this.run(input, scope); - for (const item of parsed) { - const validated = await applyFiltersSchemaValidationAsync(item, scope.schema); - if (validated) { - items.push(validated); - } - } + const validatedItems = await Promise.all( + parsed.map((item) => applyFiltersSchemaValidationAsync(item, scope.schema)) + ); + + let items: ICondition[] = validatedItems.filter( + (item): item is ICondition => typeof item !== 'undefined' + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/parser-simple/src/parameter/filters/module.ts` around lines 78 - 87, Update the validation loop in the filter parsing method around run and applyFiltersSchemaValidationAsync to execute independent validations concurrently with Promise.all, then retain only successful validated results in items while preserving the existing result ordering and filtering behavior.packages/parser-expression/test/unit/parser/filters.spec.ts (1)
179-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant double-parse in error-code assertions.
Both tests parse the same input twice — once for
toThrow(FiltersParseError), once more in a try/catch just to read.code.toThrowcan match on error shape directly, avoiding the second parse.♻️ Proposed simplification
])('should reject unmatched source characters in %s', (input) => { - expect(() => parser.parseExact(input)).toThrow(FiltersParseError); - - try { - parser.parseExact(input); - } catch (error) { - expect((error as FiltersParseError).code).toEqual(ErrorCode.SYNTAX_INVALID); - } + expect(() => parser.parseExact(input)).toThrow( + expect.objectContaining({ code: ErrorCode.SYNTAX_INVALID }), + ); });Same pattern applies to the excessive-nesting test at Lines 193-204.
Also applies to: 193-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/parser-expression/test/unit/parser/filters.spec.ts` around lines 179 - 191, Update the unmatched-source-character and excessive-nesting tests to parse each input only once, using the assertion framework to verify both the thrown FiltersParseError type and its ErrorCode.SYNTAX_INVALID code directly. Remove the redundant try/catch and second parser.parseExact invocation while preserving the existing test cases.packages/parser-expression/src/parameter/filters/module.ts (1)
56-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSync/async paths duplicate scope-resolution and null/EOF handling.
parse()/parseAsync()(Lines 63-70 vs 87-94) andparseExact()/parseExactAsync()(Lines 111-134 vs 154-177) are near-identical except for the sync vs async validation call. This is a real drift risk in a hardening-focused PR: a future fix to scope resolution, the EOF check, or the defaults fallback could easily be applied to only one of the two paths.Consider extracting the shared logic (scope resolution, tokenize+EOF check, defaults construction) into a private helper reused by both sync and async entry points, with only the validation call diverging.
♻️ Sketch of a shared helper
+ private parseTokens( + input: string, + options: FiltersParseOptions, + ) : { expr: Filters | Filter, scope?: FiltersScope } { + this.pos = 0; + this.tokens = this.tokenize(input); + + let scope : FiltersScope | undefined; + if (options.schema || options.strict) { + scope = ResolutionScope.for(this.registry, Parameter.FILTERS, options.schema, { + relations: options.relations, + throwOnFailure: true, + strict: options.strict, + }) as FiltersScope; + } + + const expr = this.parseFilterExpression(scope); + if (this.peek().type !== FilterTokenType.EOF) { + throw FiltersParseError.syntaxInvalid(`Unexpected token: ${this.peek().type}`); + } + + return { expr, scope }; + } + parseExact<RECORD extends ObjectLiteral = ObjectLiteral>( input: unknown, options: FiltersParseOptions<RECORD> = {}, ) : IFilters | IFilter { if (typeof input !== 'string') { throw FiltersParseError.inputInvalid(); } - this.pos = 0; - this.tokens = this.tokenize(input); - ... + const { expr, scope } = this.parseTokens(input, options); if (!scope) { return expr; } const validated = applyFiltersSchemaValidation(expr, scope.schema); ...Also applies to: 107-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/parser-expression/src/parameter/filters/module.ts` around lines 56 - 105, Refactor the duplicated handling in parse(), parseAsync(), parseExact(), and parseExactAsync() into private shared helpers for scope resolution, null/EOF handling, and defaults construction, while keeping only the synchronous versus asynchronous validation call separate. Ensure both entry points preserve identical behavior for absent input, tokenization, EOF checks, and fallback filter construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sql/src/visitor/filters.ts`:
- Around line 150-168: Update visitFilterRegex to stop constructing JavaScript
RegExp instances for string patterns; pass expr.value directly to the database
adapter and use ignoreCase=false for strings while preserving RegExp handling
and its flags. Remove the unit test expecting AdapterError for invalid string
patterns and revise the SQL documentation so string pattern validation/errors
are delegated to the database.
In `@packages/typeorm/test/unit/adapter/module.spec.ts`:
- Around line 86-88: Update the SQL assertions in
packages/typeorm/test/unit/adapter/module.spec.ts at lines 86-88 and 101 to use
queryBuilder.getQueryAndParameters(). At lines 86-88, assert the generated SQL
uses parameter placeholders for both user.id and user.age, and verify the
returned parameters contain 1 and 18; at line 101, assert the user.id
placeholder and verify the parameters contain 1.
---
Duplicate comments:
In `@packages/typeorm/src/adapter/filters.ts`:
- Around line 150-155: Update the filter application logic in the adapter method
containing this sql block to remove the previously applied adapter-owned WHERE
entry from the query builder’s expressionMap.wheres before adding the new
condition. Preserve all application-owned predicates, then append the current
filters with andWhere so repeated executions replace only the adapter fragment
rather than accumulating duplicates.
---
Nitpick comments:
In `@packages/parser-expression/src/parameter/filters/module.ts`:
- Around line 56-105: Refactor the duplicated handling in parse(), parseAsync(),
parseExact(), and parseExactAsync() into private shared helpers for scope
resolution, null/EOF handling, and defaults construction, while keeping only the
synchronous versus asynchronous validation call separate. Ensure both entry
points preserve identical behavior for absent input, tokenization, EOF checks,
and fallback filter construction.
In `@packages/parser-expression/test/unit/parser/filters.spec.ts`:
- Around line 179-191: Update the unmatched-source-character and
excessive-nesting tests to parse each input only once, using the assertion
framework to verify both the thrown FiltersParseError type and its
ErrorCode.SYNTAX_INVALID code directly. Remove the redundant try/catch and
second parser.parseExact invocation while preserving the existing test cases.
In `@packages/parser-simple/src/parameter/filters/module.ts`:
- Around line 78-87: Update the validation loop in the filter parsing method
around run and applyFiltersSchemaValidationAsync to execute independent
validations concurrently with Promise.all, then retain only successful validated
results in items while preserving the existing result ordering and filtering
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e3aee13-0526-464c-9e8d-6e325fa23f92
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (92)
.agents/architecture.md.agents/conventions.md.agents/migration-notes.md.agents/references/typeorm-extension.md.agents/references/typeorm.md.github/workflows/release.yml.release-please-manifest.jsonLICENSEnx.jsonpackage.jsonpackages/codec-url-expression/LICENSEpackages/codec-url-expression/package.jsonpackages/codec-url-expression/src/decoder/module.tspackages/codec-url-expression/src/encoder/module.tspackages/codec-url-expression/test/unit/encoder-schema.spec.tspackages/codec-url-simple/LICENSEpackages/codec-url-simple/package.jsonpackages/codec-url-simple/src/decoder/module.tspackages/codec-url-simple/src/encoder/module.tspackages/codec-url-simple/test/unit/decoder.spec.tspackages/codec-url-simple/test/unit/encoder-schema.spec.tspackages/codec-url/LICENSEpackages/codec-url/package.jsonpackages/codec-url/src/module.tspackages/codec-url/src/types.tspackages/codec-url/test/unit/registry.spec.tspackages/core/LICENSEpackages/core/package.jsonpackages/core/src/errors/code.tspackages/core/src/errors/schema.tspackages/core/src/parameter/filters/record/types.tspackages/core/src/parser/base.tspackages/core/src/parser/parameter/filters/index.tspackages/core/src/parser/parameter/filters/validate.tspackages/core/src/parser/query.tspackages/core/src/parser/types.tspackages/core/test/unit/parameter/filters-validation.spec.tspackages/docs/guide/errors.mdpackages/docs/guide/executing-queries.mdpackages/docs/guide/filters.mdpackages/docs/guide/migration-typeorm-extension.mdpackages/docs/package.jsonpackages/docs/packages/codec-url-expression.mdpackages/docs/packages/codec-url-simple.mdpackages/docs/packages/codec-url.mdpackages/docs/packages/parser-expression.mdpackages/docs/packages/parser-mongo.mdpackages/docs/packages/parser-simple.mdpackages/docs/packages/sql.mdpackages/docs/packages/typeorm.mdpackages/memory/LICENSEpackages/memory/package.jsonpackages/memory/src/parameter/filters/compiler.tspackages/parser-expression/LICENSEpackages/parser-expression/package.jsonpackages/parser-expression/src/parameter/filters/constants.tspackages/parser-expression/src/parameter/filters/module.tspackages/parser-expression/test/unit/parser/filters.spec.tspackages/parser-mongo/LICENSEpackages/parser-mongo/package.jsonpackages/parser-mongo/src/parameter/filters/module.tspackages/parser-mongo/test/unit/parser/filters.spec.tspackages/parser-simple/LICENSEpackages/parser-simple/package.jsonpackages/parser-simple/src/parameter/fields/module.tspackages/parser-simple/src/parameter/filters/module.tspackages/parser-simple/src/parameter/pagination/module.tspackages/parser-simple/src/parameter/relations/module.tspackages/parser-simple/src/parameter/sorts/module.tspackages/parser-simple/test/unit/parser/filters.spec.tspackages/parser-simple/test/unit/parser/parser.spec.tspackages/sql/LICENSEpackages/sql/package.jsonpackages/sql/src/adapter/module.tspackages/sql/src/adapter/relations/base.tspackages/sql/src/adapter/relations/types.tspackages/sql/src/dialect/oracle.tspackages/sql/src/dialect/pg.tspackages/sql/src/helpers/relation-alias.tspackages/sql/src/visitor/filters.tspackages/sql/test/unit/adapter.spec.tspackages/sql/test/unit/interpreters/elem-match.spec.tspackages/sql/test/unit/interpreters/regex.spec.tspackages/sql/test/unit/interpreters/relation-nested.spec.tspackages/sql/test/unit/interpreters/relation.spec.tspackages/typeorm/LICENSEpackages/typeorm/package.jsonpackages/typeorm/src/adapter/filters.tspackages/typeorm/test/unit/acceptance.spec.tspackages/typeorm/test/unit/adapter/module.spec.tspackages/typeorm/test/unit/adapter/relations.spec.tsrelease-please-config.json
Comment on lines
+86
to
+88
| const sql = queryBuilder.getSql(); | ||
| expect(sql).toContain('WHERE "user"."id" = 1 AND'); | ||
| expect(sql).toContain('"user"."age" = 18'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the TypeORM SQL string assertions to expect parameter placeholders.
TypeORM's getSql() method does not inline literal parameter values (like 1 or 18) into the generated SQL string. Instead, it uses parameter placeholders (such as ?, $1, or :actorId) depending on the active driver. You should use getQueryAndParameters() to robustly assert both the SQL structure and the parameter values.
packages/typeorm/test/unit/adapter/module.spec.ts#L86-L88: extract the SQL and parameters viagetQueryAndParameters()and match against standard placeholders (e.g.,WHERE "user"."id" = ? AND "user"."age" = ?) while verifyingparamscontains1and18.packages/typeorm/test/unit/adapter/module.spec.ts#L101-L101: extract viagetQueryAndParameters()and match against the placeholder for"user"."id"while verifyingparamscontains1.
🐛 Proposed fixes
For lines 86-88:
- const sql = queryBuilder.getSql();
- expect(sql).toContain('WHERE "user"."id" = 1 AND');
- expect(sql).toContain('"user"."age" = 18');
+ const [sql, params] = queryBuilder.getQueryAndParameters();
+ expect(sql).toMatch(/WHERE "user"\."id" = (?:\?|\$1|:actorId) AND/);
+ expect(sql).toMatch(/"user"\."age" = (?:\?|\$2|:[a-zA-Z0-9_]+)/);
+ expect(params).toEqual(expect.arrayContaining([1, 18]));For line 101:
- expect(queryBuilder.getSql()).toContain('WHERE "user"."id" = 1');
+ const [sql, params] = queryBuilder.getQueryAndParameters();
+ expect(sql).toMatch(/WHERE "user"\."id" = (?:\?|\$1|:actorId)/);
+ expect(params).toEqual(expect.arrayContaining([1]));📍 Affects 1 file
packages/typeorm/test/unit/adapter/module.spec.ts#L86-L88(this comment)packages/typeorm/test/unit/adapter/module.spec.ts#L101-L101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/typeorm/test/unit/adapter/module.spec.ts` around lines 86 - 88,
Update the SQL assertions in packages/typeorm/test/unit/adapter/module.spec.ts
at lines 86-88 and 101 to use queryBuilder.getQueryAndParameters(). At lines
86-88, assert the generated SQL uses parameter placeholders for both user.id and
user.age, and verify the returned parameters contain 1 and 18; at line 101,
assert the user.id placeholder and verify the parameters contain 1.
Owner
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Pass string patterns through without parsing them with JavaScript so each database engine can apply its own regex syntax.
Merged
Merged
This was referenced Aug 2, 2026
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.
Summary
Breaking change
Generated relation aliases now use length-prefixed path segments so distinct relation paths cannot collide.
Verification
Release follow-up
After merge, refresh the existing release-please PR and verify that it proposes all ten packages at 2.0.0-beta.0. Remove the one-time release-as override after the first beta is published.
Summary by CodeRabbit