feat: add safe empty where in plugin #925
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
igalklebanov
added
enhancement
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.
what about situations where the value in db is null?
There was a problem hiding this comment.
in (null) should always return nothing, its the equivalent of 1=0
There was a problem hiding this comment.
OK let's make sure this is correct across all 4 dialects:
- have a record in data that has a
nullin 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.
Alternatively, could just alter the tree to 1=0 or equivalent and call it a day.
There was a problem hiding this comment.
015ee37 added tests here. Let me know if it is sufficient
| 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.
why do we need a special table for this suite?
There was a problem hiding this comment.
whats the convention in this project? im doing this just so to avoid collisions with other tests suites if things are run in parallel
| let result = await query.execute() | ||
|
|
||
| expect(result).to.deep.equal([]) |
There was a problem hiding this comment.
no need to execute really, testing the compiled sql is more than enough.
There was a problem hiding this comment.
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.
no need to execute really, testing the compiled sql is more than enough.
There was a problem hiding this comment.
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.
no need to execute really, testing the compiled sql is more than enough.
There was a problem hiding this comment.
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? |
Addresses concerns #709 with a plugin.