Skip to content

Fix handling of inserts with both unless conflict and a with block #1291

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 1 commit into from
Jun 3, 2025

Conversation

jaclarke
Copy link
Member

@jaclarke jaclarke commented Jun 3, 2025

Fixes #788, and should also fix #679

@jaclarke jaclarke requested a review from scotttrinh June 3, 2025 12:31
Copy link
Collaborator

@scotttrinh scotttrinh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏆

Comment on lines +609 to +612
childExprs.push(
...walkExprTree(expr.__expr__, parentScope, ctx),
...insertChildExprs,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m typing this out to ensure I understand the fix here:

In a previous with extraction fix, we extracted child expressions from the unlessConflict expression and stored them in the insertChildExprs array. However, we didn’t add these child expressions to the parent (insert) expression’s child expression list. Consequently, when the expression -> query machinery was validating the with access, it was not able to see that these expressions were allowed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost, we did add the insertChildExprs to the wrapped insert expr, but not the unless conflict wrapper expr. The real issue was we weren't recording that the insert expr (and its children) were child exprs of the unless conflict expr, so it couldn't see in the scope validation step that the child exprs in the insert expr that appeared in the with block were also children of the unless conflict expr.

The insertChildExprs thing is slightly different, and is to assign the exprs in the unless conflict clause as children of the insert expr instead of the unless conflict wrapper expr. Which I think if I remember right is to fix some other issue where something gets confused if the exprs in the conflict clause reference the insert expr, and that appears to be a sibling expr (as it does if both were children of the unless conflict wrapper)...?

@scotttrinh scotttrinh merged commit d26ce0b into master Jun 3, 2025
9 checks passed
@scotttrinh scotttrinh deleted the fix-qb-with-insert-unless-conflict branch June 3, 2025 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants