fix: use subquery with join map one methods #10769
Conversation
58c243d to
727d909
Compare
|
Need this to get merged asap |
1 similar comment
|
Need this to get merged asap |
|
any hope on this merge? |
|
@gioboa It's still be valid. The PR includes test coverage here: https://github.com/typeorm/typeorm/pull/10769/files#diff-fc1093be2aa4e9334350e29375146e0e2cc029295285b80c5caf92dac490c422 Open to comments if it's in need of more tests. |
WalkthroughThe changes refactor the internal logic of subquery handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant TestCase
participant QueryBuilder
participant SubQuery
participant Database
TestCase->>QueryBuilder: Initiate join with subquery
QueryBuilder->>SubQuery: Build subquery (function or string)
SubQuery-->>QueryBuilder: Return subquery SQL and parameters
QueryBuilder->>Database: Execute join with subquery
Database-->>QueryBuilder: Return results (null or no entity)
QueryBuilder-->>TestCase: Return mapped result
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/query-builder/SelectQueryBuilder.ts (3)
2037-2043: Improve sub-query detection robustnessThe heuristic for identifying a string-based sub-query checks only that the first and last characters are
"("/")". This will fail on otherwise valid inputs such as"\n( SELECT 1 ) "or strings padded with comments / leading whitespace.
Consider trimming whitespace (and maybe lower-casing) before the check or, even better, usingstartsWith("(")/endsWith(")")after atrim()to make the test less brittle.-const isSubQuery = - (!isEntity && typeof entityOrProperty === "function") || - (typeof entityOrProperty === "string" && - entityOrProperty.substr(0, 1) === "(" && - entityOrProperty.substr(-1) === ")") +const isSubQuery = + (!isEntity && typeof entityOrProperty === "function") || + (typeof entityOrProperty === "string" && + entityOrProperty.trim().startsWith("(") && + entityOrProperty.trim().endsWith(")"))
2046-2056: Potential misuse ofthiswhen building a sub-query
thisis already aSelectQueryBuilderinside the current context, so the double cast(this as any as SelectQueryBuilder<any>).subQuery()is unnecessary and can hide compile-time errors. Calling
this.subQuery()directly is simpler and safer.-const subQueryBuilder: SelectQueryBuilder<any> = ( - entityOrProperty as any -)((this as any as SelectQueryBuilder<any>).subQuery()) +const subQueryBuilder = (entityOrProperty as any)(this.subQuery())
2071-2075: PasssubQueryunconditionally if already detectedBecause
isSubQueryis guaranteed to be truthy wheneversubQueryis populated,subQuery ? subQuery : undefinedis simpler and guards against accidental divergence between the two flags.- subQuery: isSubQuery ? subQuery : undefined, + subQuery,(This is purely cosmetic; no behaviour change.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/query-builder/SelectQueryBuilder.ts(2 hunks)test/functional/query-builder/join/query-builder-joins.ts(2 hunks)
🔇 Additional comments (2)
test/functional/query-builder/join/query-builder-joins.ts (2)
573-605: Excellent test case for leftJoinAndMapOne subquery edge case.This test effectively validates that
leftJoinAndMapOnewith a subquery properly respects the subquery conditions. The test correctly:
- Creates a Tag with name "audi" and links it to a Post
- Uses a subquery that filters out the existing tag (
tag.name != "audi")- Asserts that the mapped property is null when the subquery finds no matches
This directly addresses the PR objective of ensuring subqueries are not overridden by metadata-based queries in join mapping methods.
1058-1089: Excellent test case for innerJoinAndMapOne subquery edge case.This test effectively validates that
innerJoinAndMapOnewith a subquery properly respects the subquery conditions. The test correctly:
- Creates a Tag with name "audi" and links it to a Post
- Uses a subquery that filters out the existing tag (
tag.name != "audi")- Asserts that the entire result is null when the inner join subquery finds no matches
This complements the leftJoinAndMapOne test and ensures both join mapping methods handle subquery conditions correctly, which aligns perfectly with the PR's fix for issue #5637.
will do |
727d909 to
755f1bb
Compare
|
@xdc0 could you check if #5637 (comment) solves the original issue for you? |
2edda8d to
9cb09d2
Compare
@alumni Yes, it is still an issue when adding Here is the generated sql without my patch (and using SELECT "post"."id" AS "post_id", "post"."title" AS "post_title", "post"."tagId" AS "post_tagId", "post"."authorId" AS "post_authorId", "tag"."id" AS "tag_id", "tag"."name" AS "tag_name" FROM "post" "post" LEFT JOIN "tag" "tag" ON "tag"."id" = "post"."tagId" WHERE "post"."id" = $1And here is the sql with my patch. SELECT "post"."id" AS "post_id", "post"."title" AS "post_title", "post"."tagId" AS "post_tagId", "post"."authorId" AS "post_authorId", "tag"."id" AS "tag_id", "tag"."name" AS "tag_name" FROM "post" "post" LEFT JOIN (SELECT * FROM "tag" "tag" WHERE "tag"."name" != $1) "tag" ON "tag"."id" = "post"."tagId" WHERE "post"."id" = $2 |
9cb09d2 to
6d5c320
Compare
|
Updated fork with upstream. Ready for another review pass. |
85b5a3a to
071ea24
Compare
|
Still an issue. Any change of this getting merged? |
071ea24 to
d4e7c3d
Compare
PR Code Suggestions ✨Latest suggestions up to 3e73121
Previous suggestionsSuggestions up to commit d4e7c3d
|
||||||||||||||||||
This is not new code added in this PR. I'm merely moving up that evaluation without touching any of the current implementation |
|
@gioboa Done updating the branch one more time. The conflict happened due a file name change. No actual code conflict. |
commit: |
pkuczynski
left a comment
There was a problem hiding this comment.
Looks good to me. Few small code changes suggestions...
gioboa
left a comment
There was a problem hiding this comment.
is this PR still valid? there are few feedback to address.
The PR is still as valid as it was almost 2 years ago. I will address feedback today and send it again for review. |
d4e7c3d to
3e73121
Compare
G0maa
left a comment
There was a problem hiding this comment.
Thanks for your contribution. Will mention other maintainers internally so we could get this merged, please do resolve comments if you've fixed them.
The author addressed the suggestions.
Description of change
On #9354 there was a patch to map subquery results to an entity when using the
leftJoinAndMapOneandinnerJoinAndMapOnemethods through a new parameter:mapAsEntity.This unfortunately does not quite work as expected because the subquery is ignored since the join logic detects that there is metadata and the logic will override the subquery in favor of generated queries via metadata.
This problem is described by @cjsauer on the following comment: #9354 (comment)
Fixes #5637
Pull-Request Checklist
masterbranchnpm run formatto apply prettier formattingnpm run testpasses with this changeFixes #0000Summary by CodeRabbit