fix: add placeholder for string values #733
Conversation
mariayord
requested review from
danjoa,
johannes-vogel,
stewsk,
patricebender and
sjvans
as code owners
mariayord
changed the title
patricebender
changed the title
|
@johannes-vogel there are |
db-service/lib/cqn2sql.js
Outdated
| let sql = this.expr({ param: false, __proto__: x }) | ||
|
|
||
| let sql = x.param !== true && (typeof x.val === 'number' || x.val === null) ? this.expr({ param: false, __proto__: x }): this.expr(x) |
There was a problem hiding this comment.
Why the null it doesn't have a type. So whatever type the ? is assigned by the database null is accepted and will be returned correctly as a null.
There was a problem hiding this comment.
There was a problem hiding this comment.
@BobdenOs Do you think the generated snapshot is wrong and in this case there should be a placeholder for null ?
I added a check to fix the test.
There was a problem hiding this comment.
When changing behavior all snapshots are wrong by default. It is required to check the difference between the snapshots when changing the behavior. I would rather look at the integration tests whether the behavior is correct. If Postgres and HANA don't have problem with handling placeholders for null values in the select columns. I would interpretate null as placeholders.
There was a problem hiding this comment.
It is required to check the difference between the snapshots when changing the behavior.
I checked this , the question is if the old result of the test is correct, with no placeholder for null value.
If you say it is not the problem for db to have a null placeholder, the test result before was wrong!
There was a problem hiding this comment.
When changing behavior all snapshots are wrong by default
I would rather look at the integration tests whether the behavior is correct.
Does SELECT([{val: null, as:'Null'}]) return [{"Null":null}] when executed on all the databases ? If that is the case then the snapshot is wrong.
test/scenarios/bookshop/read.test.js
Outdated
| @@ -53,6 +53,13 @@ describe('Bookshop - Read', () => { | |||
| expect(columns).to.contain('title') | |||
| }) | |||
|
|
|||
| test('Path expression with null value', async () => { | |||
There was a problem hiding this comment.
- the test name is incorrect
- there is already a test doing this as was shared with you before here
There was a problem hiding this comment.
- Please make a suggestion for the test name if you don't like it.
- This is an integration test (I suppose it should run with Hana, Postgres..), the other one is unit, if this is not the case please point where to add the integration test.
Fixes : cap/issues/issues/16220