-
Notifications
You must be signed in to change notification settings - Fork 239
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
feat: add safe empty where in plugin #925
base: master
Are you sure you want to change the base?
feat: add safe empty where in plugin #925
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/plugin/safe-empty-array-where-in/safe-empty-array-where-in.transformer.ts
Outdated
Show resolved
Hide resolved
src/plugin/safe-empty-array-where-in/safe-empty-array-where-in.transformer.ts
Outdated
Show resolved
Hide resolved
src/plugin/safe-empty-array-where-in/safe-empty-array-where-in.transformer.ts
Outdated
Show resolved
Hide resolved
src/plugin/safe-empty-array-where-in/safe-empty-array-where-in.transformer.ts
Outdated
Show resolved
Hide resolved
src/plugin/safe-empty-array-where-in/safe-empty-array-where-in.transformer.ts
Outdated
Show resolved
Hide resolved
src/plugin/safe-empty-array-where-in/safe-empty-array-where-in.transformer.ts
Outdated
Show resolved
Hide resolved
src/plugin/safe-empty-array-where-in/safe-empty-array-where-in.transformer.ts
Outdated
Show resolved
Hide resolved
src/plugin/safe-empty-array-where-in/safe-empty-array-where-in.transformer.ts
Outdated
Show resolved
Hide resolved
...binaryOperatorNode, | ||
rightOperand: { | ||
...rightOperand, | ||
values: [null], |
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.
what about situations where the value in db is null
?
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.
in (null) should always return nothing, its the equivalent of 1=0
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.
OK let's make sure this is correct across all 4 dialects:
- have a record in data that has a
null
in that column. - run query against db and expect results are empty.
But do it once. No need to communicate with DB for all test cases.. just not cost effective.
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.
Alternatively, could just alter the tree to 1=0
or equivalent and call it a day.
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.
015ee37 added tests here. Let me know if it is sufficient
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.
@austinwoon I agree with @igalklebanov that you should switch to 1=0
and 1=1
rewrites for in/not-in clauses. Right now the transform does not seem to work correctly for the not in
operator, since select 1 where 1 not in (null);
does not return any rows. Following code seems to transform things correctly.
// NOTE: Translating the clause to `1 = 1` behaves semantically as truthy
if (operator.operator === 'not in') {
return BinaryOperationNode.create(
ValueNode.create(1),
OperatorNode.create('='),
ValueNode.create(1)
);
}
// NOTE: Translating the clause to `1 = 0` behaves semantically as falsey
return BinaryOperationNode.create(
ValueNode.create(1),
OperatorNode.create('='),
ValueNode.create(0)
);
We were testing this PR in our project, and noticed the issue, thanks for all your work!
await db.schema.dropTable('safeEmptyArrayPerson').ifExists().execute() | ||
await createTableWithId(db.schema, dialect, 'safeEmptyArrayPerson') | ||
.addColumn('firstName', 'varchar(255)') | ||
.execute() | ||
|
||
await db | ||
.insertInto('safeEmptyArrayPerson') | ||
.values([ | ||
{ | ||
firstName: 'John', | ||
}, | ||
{ | ||
firstName: 'Mary', | ||
}, | ||
{ | ||
firstName: 'Tom', | ||
}, | ||
]) | ||
.execute() |
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.
why do we need a special table for this suite?
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.
whats the convention in this project? im doing this just so to avoid collisions with other tests suites if things are run in parallel
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.
Things are not run in parallel. Each suite creates and destroys everything.
let result = await query.execute() | ||
|
||
expect(result).to.deep.equal([]) |
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.
no need to execute really, testing the compiled sql is more than enough.
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.
i think we should execute to make sure there are no run-time errors, which was the case before.
let result = await query.execute() | ||
|
||
expect(result).to.have.length(2) | ||
expect(result).to.deep.equal([ | ||
{ firstName: 'John' }, | ||
{ firstName: 'Mary' }, | ||
]) |
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.
no need to execute really, testing the compiled sql is more than enough.
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.
Could we opt to keep to test for runtime errors?
let result = await query.execute() | ||
|
||
expect(result).to.deep.equal([new DeleteResult(BigInt(0))]) |
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.
no need to execute really, testing the compiled sql is more than enough.
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.
Ah, can i leave it here since its already there? Would be good for us to know side-effects if any for results as well.
@igalklebanov ive addressed all of the PR comments, could i check if this is ready to be merged? |
return this.#transformer.transformNode(args.node) | ||
} | ||
|
||
transformResult( |
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.
Nit: Can just use async
keyword here, avoid explicit Promise.resolve
call.
Addresses concerns #709 with a plugin.