fix: cds failing tests #154

BobdenOs · 2023-08-04T00:24:39Z

Description

After investigating #153 I determined there are more breaking changes merged into main. As part of cleanup and refactor PRs. This PR contains fixes and analysis of why cds tests started to fail.

…-tests

BobdenOs · 2023-08-04T09:10:48Z

@stewsk In the recent change of cqn4sql the wildcard logic no longer include the generated foreign keys. While the cds tests expect them to be returned. Any suggestions ?

stewsk · 2023-08-04T09:28:31Z

Hi @BobdenOs ,

In the recent change

which change are you referring to?

danjoa · 2023-08-04T09:52:00Z

db-service/lib/cqn2sql.js

@@ -239,7 +239,8 @@ class CQN2SQLRenderer {
      : SELECT.columns.map(x => {
          const name = this.column_name(x)
          // REVISIT: can be removed when alias handling is resolved properly
-          const d = elements[name] || elements[name.substring(1, name.length - 1)]
+          // REVISIT: for some reason not all elements are available in the elements list
+          const d = x.element || elements[name]


Better than before, anyways, but...

x.element should always be defined, and the fallback never required

q.elements should contain all expected elements definitions ac cording to the result set type

x.element does not exist when x is an expand sub select.

Does an entry for that exist in q.elements? → then x.element should exist as well and be the same

db-service/lib/cqn2sql.js

danjoa · 2023-08-04T09:53:38Z

db-service/lib/cqn2sql.js

+    // SELECT.from() does not accept joins
+    select.SELECT.from = from


Why do we see joins here?

We get a join when streaming from draft entities. between *.drafts and DraftAdministrativeData.

danjoa · 2023-08-04T09:55:30Z

db-service/lib/cqn2sql.js

+    // Column names like "Order" clash with "ORDER" keyword so toUpperCase is required
+    if (s.toUpperCase() in this.class.ReservedWords || /^\d|[$' ?@./\\]/.test(s)) return '"' + s + '"'


Better add the capitalised casing to ReservedWords

danjoa · 2023-08-04T09:59:20Z

sqlite/lib/SQLiteService.js

+      Timestamp: e => `ISO(${e})`,
+      // REVISIT: generic-virtual.test.js expects now to be returned as ISO string
+      // test 'field with current timestamp is correctly formatted' fails otherwise


Lets fix this test please

sqlite/lib/SQLiteService.js

danjoa · 2023-08-04T10:00:54Z

sqlite/lib/SQLiteService.js

+      Float: expr => `nullif(quote(${expr}),'NULL')->'$'`,
+      Double: expr => `nullif(quote(${expr}),'NULL')->'$'`,


Shouldn't we keep these out as before and discussed?

…o fix-cds-tests

danjoa · 2023-08-05T11:09:23Z

Replaced fuzzyKeys as that seemed too much and with strange output:

> fuzzyKeys('Order')
{
  Order: 1,
  order: 1,
  'O�der': 1,
  'o�der': 1,
  'Or�er': 1,
  'or�er': 1,
  'O��er': 1,
  'o��er': 1,
  'Ord�r': 1,
  'ord�r': 1,
  'O�d�r': 1,
  'o�d�r': 1,
  'Or��r': 1,
  'or��r': 1,
  'O���r': 1,
  'o���r': 1,
  'Orde�': 1,
  'orde�': 1,
  'O�de�': 1,
  'o�de�': 1,
  'Or�e�': 1,
  'or�e�': 1,
  'O��e�': 1,
  'o��e�': 1,
  'Ord��': 1,
  'ord��': 1,
  'O�d��': 1,
  'o�d��': 1,
  'Or���': 1,
  'or���': 1,
  'O����': 1,
  'o����': 1
}

BobdenOs · 2023-08-05T11:26:06Z

fuzzyKeys was build with the expectation that all keywords where provided in uppercase.

My understanding is that the number of keys on an object should negatively impact the key lookup speed. That is why I added the question mark.

My main concern with only having reasonable keys in the map is that the behaviour is different from having the check be case insensitive. So adding a secondary case insensitive check would be required to maintain functional consistency.

BobdenOs · 2023-08-05T11:39:37Z

Current issue is that * sometimes does not include foreign keys. My understanding is that * should not include any generated foreign key columns. But the current tests expect all foreign key columns to be included.

…o fix-cds-tests

stewsk · 2023-08-07T06:34:02Z

I think that * should include the foreign keys. When having a managed association in the select list, the respective foreign key(s) should be respected, and a * includes also the managed associations.
In which situation does * currently not include the FKs?

BobdenOs · 2023-08-07T06:39:07Z

When someone combines * and expand. The foreign key of the expand is not returned as a flat column anymore.

I fixed this issue here , but this makes all the cqn4sql tests fail.

stewsk · 2023-08-07T06:43:30Z

Just to avoid that we misunderstand each other: can you provide an example for what the select looks like, what you expect it should do, and what it actually does (currently)?

BobdenOs · 2023-08-07T06:50:04Z

Sure here are all the scenarios:

simple

input:

select * from Books

output:

select ID, tittle, descr, author_ID, genre_ID from Books

with expand

input:

select *, author { * } from Books

output:

select 
  ID,
  tittle,
  descr,
  (select ID, name from Author where Author.ID = Books.author_ID) as author,
  genre_ID
from Books

expected:

select 
  ID,
  tittle,
  descr,
  (select ID, name from Author where Author.ID = Books.author_ID) as author,
  author_ID, -- this column is currently missing
  genre_ID
from Books

stewsk · 2023-08-07T07:05:28Z

Ok, I see.
I would say: works as designed. The reasoning for the current behavior is:
Selecting author { * } results in a structured thing called author.
Now there is the "smart *" mechanism, which implicitly "removes" all elements from the * that are also explicitly selected - otherwise that would lead to duplicate names in the result. This mechanism operates before the flattening and it operates on the names in the result set. So in

select from Books { *, author }

the author is implicitly removed from the *, as it is explicitly selected.
And the same happens for

select from Books { *, author { * } }

as the effective/resulting name of author {*} is author.

If you wrote

select from Books { *, author as a { * } }

then you would get the foreign key from the *

…o fix-cds-tests

stewsk · 2023-08-07T10:21:54Z

@patricebender - we need to have a detailed look at this when you are back.
The smart "*" is intended to avoid duplicates in the inferred elements list. In the situation described by Bob above there apparently is no such conflict - at least for flat access. We should check however, that the latest fix did not introduce problems in other situations.

this reverts the cqn4sql part of the hotfix in #154 --- The problem was, that in a flat model, the foreign keys were not part of the result anymore, in case that the respective association was overwritten by the smart wildcard mechanism. E.g. by an expand. The reason was, that in case of a flat model, we skipped the foreign keys and still flattened the association. This caused the bug where overwritting an association caused the lack of the foreign key in the result. The fix here is simple, skip the association from the wildcard expansion instead. The compiler already, correctly generated the foreign key elements for us.

…ode (#176) * cqn4sql: skip assocs from wildcard expansion instead of fks in flat mode this reverts the cqn4sql part of the hotfix in #154 --- The problem was, that in a flat model, the foreign keys were not part of the result anymore, in case that the respective association was overwritten by the smart wildcard mechanism. E.g. by an expand. The reason was, that in case of a flat model, we skipped the foreign keys and still flattened the association. This caused the bug where overwritting an association caused the lack of the foreign key in the result. The fix here is simple, skip the association from the wildcard expansion instead. The compiler already, correctly generated the foreign key elements for us. * adapt test description * no debug * Update db-service/test/cqn4sql/wildcards.test.js Co-authored-by: Steffen Weinstock <[email protected]> --------- Co-authored-by: Steffen Weinstock <[email protected]>

Address most cds test regressions after refactors

71816e8

BobdenOs requested review from David-Kunz, patricebender, danjoa, gregorwolf, johannes-vogel, vobu, stewsk, sebastianesch and sjvans as code owners August 4, 2023 00:24

Merge branch 'main' of https://github.com/cap-js/cds-dbs into fix-cds…

87156ec

…-tests

danjoa requested changes Aug 4, 2023

View reviewed changes

danjoa and others added 15 commits August 4, 2023 12:01

Merge branch 'main' into fix-cds-tests

1f713d4

cqn2sql val returns always SQL strings

19a2ead

Remove Float output converter

a4ecd4c

Merge branch 'fix-cds-tests' of https://github.com/cap-js/cds-dbs int…

8fdb91b

…o fix-cds-tests

Add element to expand SELECT column

8e384da

Make in ReservedWords case insensitive

3d96229

Move require stream to the top

d49eac1

Fix OData streaming test

8dbec53

Adjust test expectation to string decimal

a25a24d

Cleanup Decimal converter comment

d17a492

Filter foreign keys again

3353164

Add removed brackets

598fffe

Resolve conflicts

5b2ff2d

Upper, lower, and capitalized keywords

0f12c6c

✨prettier✨

328513a

danjoa enabled auto-merge (squash) August 5, 2023 11:09

AutoIncrement

9b497ae

BobdenOs added 2 commits August 5, 2023 13:57

Always include foreign keys of association in wildcard

adc2b47

Merge branch 'fix-cds-tests' of https://github.com/cap-js/cds-dbs int…

9442e96

…o fix-cds-tests

minor cosmetics

3e1af1d

danjoa approved these changes Aug 7, 2023

View reviewed changes

BobdenOs added 2 commits August 7, 2023 11:57

Adjust wildcard cqn4sql test

f65afe7

Merge branch 'fix-cds-tests' of https://github.com/cap-js/cds-dbs int…

de66504

…o fix-cds-tests

danjoa merged commit d2bd96e into main Aug 7, 2023

danjoa deleted the fix-cds-tests branch August 7, 2023 10:01

patricebender mentioned this pull request Aug 17, 2023

cqn4sql: skip assocs from wildcard expansion instead of fks in flat mode #176

Merged

cap-bots mentioned this pull request Jul 18, 2024

chore: release main #743

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cds failing tests #154

fix: cds failing tests #154

BobdenOs commented Aug 4, 2023

BobdenOs commented Aug 4, 2023

stewsk commented Aug 4, 2023

danjoa Aug 4, 2023

BobdenOs Aug 4, 2023

danjoa Aug 4, 2023

danjoa Aug 4, 2023

BobdenOs Aug 4, 2023

danjoa Aug 4, 2023

danjoa Aug 4, 2023

danjoa Aug 4, 2023

danjoa commented Aug 5, 2023

BobdenOs commented Aug 5, 2023

BobdenOs commented Aug 5, 2023

stewsk commented Aug 7, 2023

BobdenOs commented Aug 7, 2023

stewsk commented Aug 7, 2023

BobdenOs commented Aug 7, 2023

stewsk commented Aug 7, 2023 •

edited

Loading

stewsk commented Aug 7, 2023 •

edited

Loading

		// SELECT.from() does not accept joins
		select.SELECT.from = from

		// Column names like "Order" clash with "ORDER" keyword so toUpperCase is required
		if (s.toUpperCase() in this.class.ReservedWords \|\| /^\d\|[$' ?@./\\]/.test(s)) return '"' + s + '"'

		Float: expr => `nullif(quote(${expr}),'NULL')->'$'`,
		Double: expr => `nullif(quote(${expr}),'NULL')->'$'`,

fix: cds failing tests #154

fix: cds failing tests #154

Conversation

BobdenOs commented Aug 4, 2023

Description

BobdenOs commented Aug 4, 2023

stewsk commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjoa commented Aug 5, 2023

BobdenOs commented Aug 5, 2023

BobdenOs commented Aug 5, 2023

stewsk commented Aug 7, 2023

BobdenOs commented Aug 7, 2023

stewsk commented Aug 7, 2023

BobdenOs commented Aug 7, 2023

simple

with expand

stewsk commented Aug 7, 2023 • edited Loading

stewsk commented Aug 7, 2023 • edited Loading

stewsk commented Aug 7, 2023 •

edited

Loading

stewsk commented Aug 7, 2023 •

edited

Loading