Skip to content
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

Dynamic OR query causes processSQLQueryIR to fail #462

Closed
filipecatraia opened this issue Sep 15, 2022 · 2 comments
Closed

Dynamic OR query causes processSQLQueryIR to fail #462

filipecatraia opened this issue Sep 15, 2022 · 2 comments

Comments

@filipecatraia
Copy link

Given a simple query with dynamic parameters as such:

SELECT id, email
FROM entity.users
WHERE (id = :id OR email = :email)
AND account_deleted IS NOT TRUE;

Before upgrading to 1.0.1 from 0.13, the generated query ran without issues. The query is run via (simplified):

async function findUser({id, email}: {id?: string; email?: string}) {
    findUserByIdOrEmail.run({id, email}, pool)
    
}

With the upgrade, all queries with dynamic OR parameters crash if one of the parameters is undefined. We have a few queries like that and they all fail.

I've started digging in, and traced it down to processSQLQueryIR in preprocessor-sql.js. When you filter down for usedParams, parameters with a value of undefined might need to be skipped?

const usedParams = queryIR.params.filter((p) => p.name in queryIR.usedParamSet);

let i = 1;
const intervals = [];
for (const usedParam of usedParams) {
    
    // 🚨 The problem is here, as locs is undefined for this usedParam
    usedParam.locs.forEach… 
    
}

UPDATE: Turns out that filtering for undefined values doesn't change anything, as locs is still missing for valid parameters passed to the query.

Any idea why this could be happening, i.e. why is locs in each usedParam missing?

@filipecatraia
Copy link
Author

Downgrading to 0.13 fixes the issue, so we at least have that temporary solution :)

@adelsz
Copy link
Owner

adelsz commented Jan 28, 2023

Not able to reproduce this in v1.1.0.
An additional improvement for optional parameter UX was added in #482.
Please reopen if issue persists.

@adelsz adelsz closed this as completed Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants