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.

Sign up for GitHub

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

Jump to bottom

fix(order by): reject non-fk traversals of own columns in order by #599

Merged
merged 3 commits into from
Apr 19, 2024
Merged

fix(order by): reject non-fk traversals of own columns in order by #599

merged 3 commits into from
Apr 19, 2024

Conversation

patricebender
Copy link
Member

@patricebender patricebender commented Apr 18, 2024 •
edited
Loading

refs in order by can address queries own columns, if such a ref traverses an association to a non-foreign key field as in this example:

SELECT from bookshop.Books as foo {
        ID,
        author,
        coAuthor as co
      }
      order by
        co.name -- join with query "foo" not possible

we must reject the traversal, as the join partner of Authors is the query itself

other changes:

fix(order by): do not mistake simple name with infix filter
another fix do not mistake simple name with infix filter: If
the query has columns but they solely consist of *,
cqn4sql wrongly assumed that an association traversal has to be resolved in the queries elements, this lead to a
confusing error complaining that only foreign key of assocs can be traversed in infix filters.

@patricebender
fix(order by): do not mistake simple name with infix filter
0d982a6 
If the query has `columns` but they solely consist
of `*`, `cqn4sql` wrongly assumed that an association
traversal has to be resolved in the queries elements,
this lead to a confusing error complaining that only foreign key
of assocs can be traversed in infix filters.

Actually, if we have something like this:

```
select from Books {
  author as bubu
} order by bubu.name
```

we must reject the non foreign key traversal of `bubu`,
as we can't build the correct join. Instead, a user must use
the association `Books:author`. This change aligns the behavior
with the cds compiler.
@patricebender
reject non-fk traversals of queries own cols in order by
f32f6b3
@patricebender patricebender changed the title fix(order by): do not mistake simple name with infix filter fix(order by): reject non-fk traversals of own columns in order by Apr 19, 2024
@patricebender patricebender marked this pull request as ready for review April 19, 2024 10:22
patricebender
patricebender commented Apr 19, 2024
View reviewed changes
db-service/test/cqn4sql/table-alias.test.js
Comment on lines +549 to +554
it('do not try to resolve ref in columns if columns consists of star', () => {
let input = CQL`SELECT from bookshop.SimpleBook { * } order by author.name`
let query = cqn4sql(input, model)
const expected = CQL`SELECT from bookshop.SimpleBook as SimpleBook left join bookshop.Authors as author on author.ID = SimpleBook.author_ID
{ SimpleBook.ID, SimpleBook.title, SimpleBook.author_ID } order by author.name`
expect(query).to.deep.equal(expected)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamuelBrucksch this is the scenario you described

@patricebender patricebender enabled auto-merge (squash) April 19, 2024 16:19
@patricebender patricebender merged commit 3288d63 into main Apr 19, 2024
4 of 6 checks passed
@patricebender patricebender deleted the patrice/orderby branch April 19, 2024 16:21
@cap-bots cap-bots mentioned this pull request Apr 19, 2024
Merged
johannes-vogel added a commit that referenced this pull request May 8, 2024
@cap-bots @johannes-vogel @patricebender
chore: release main (#587)
bca93fd 
🤖 I have created a release *beep* *boop*
---


<details><summary>db-service: 1.9.0</summary>

##
[1.9.0](db-service-v1.8.0...db-service-v1.9.0)
(2024-05-08)


### Added

* Add missing `func` cqn structures
([#629](#629))
([9d7539a](9d7539a))


### Fixed

* **`order by`:** reject non-fk traversals of own columns in order by
([#599](#599))
([3288d63](3288d63))
* Align all quote functions with @sap/cds-compiler
([#619](#619))
([42e9828](42e9828))
* assign artificial alias if selecting from anonymous subquery
([#608](#608))
([e1a7711](e1a7711))
* avoid spread operator
([#630](#630))
([a39fb65](a39fb65))
* flat update with arbitrary where clauses
([#598](#598))
([f108798](f108798))
* improved `=` and `!=` with val `null` for HANA and Postgres
([#626](#626))
([cbcfe3b](cbcfe3b))
* Improved placeholders and limit clause
([#567](#567))
([d5d5dbb](d5d5dbb))
* multiple result responses
([#602](#602))
([bf0bed4](bf0bed4))
* only consider persisted columns for simple operations
([#592](#592))
([6e31bda](6e31bda))


### Changed

* require `&gt;= sap/cds-compiler@4.9`
([f4d09e2](f4d09e2))
* require `&gt;= sap/cds@7.9.0`
([#627](#627))
([f4d09e2](f4d09e2))
</details>

<details><summary>sqlite: 1.7.0</summary>

##
[1.7.0](sqlite-v1.6.0...sqlite-v1.7.0)
(2024-05-08)


### Added

* select decimals as strings if cds.env.features.string_decimals
([#616](#616))
([39addbf](39addbf))


### Fixed

* Change `sql` property to `query` for errors
([#611](#611))
([585577a](585577a))
* **hana:** Remove encoding from hana-client streams
([#623](#623))
([fed8f6f](fed8f6f))
* Improved placeholders and limit clause
([#567](#567))
([d5d5dbb](d5d5dbb))
</details>

<details><summary>postgres: 1.8.0</summary>

##
[1.8.0](postgres-v1.7.0...postgres-v1.8.0)
(2024-05-08)


### Added

* select decimals as strings if cds.env.features.string_decimals
([#616](#616))
([39addbf](39addbf))


### Fixed

* Align all quote functions with @sap/cds-compiler
([#619](#619))
([42e9828](42e9828))
* Change `sql` property to `query` for errors
([#611](#611))
([585577a](585577a))
* Improved placeholders and limit clause
([#567](#567))
([d5d5dbb](d5d5dbb))
* Use json datatype for Postgres insert
([#582](#582))
([f1c9c89](f1c9c89))
</details>

<details><summary>hana: 0.3.0</summary>

##
[0.3.0](hana-v0.2.0...hana-v0.3.0)
(2024-05-08)


### Added

* select decimals as strings if cds.env.features.string_decimals
([#616](#616))
([39addbf](39addbf))


### Fixed

* Add multi `concat` function to `@cap-js/hana`
([#624](#624))
([df436fe](df436fe))
* Align all quote functions with @sap/cds-compiler
([#619](#619))
([42e9828](42e9828))
* Change `sql` property to `query` for errors
([#611](#611))
([585577a](585577a))
* Disconnect HANA tenant when deleted
([#589](#589))
([a107db9](a107db9))
* **hana:** Align not found behavior in @cap-js/hana
([#603](#603))
([54d2efb](54d2efb))
* **hana:** Allow custom fuzzy search cqn
([#620](#620))
([80383f0](80383f0))
* **hana:** Allow HANA to use != and == inside xpr combinations
([#607](#607))
([c578e9f](c578e9f))
* **hana:** Reference column alias in order by
([#615](#615))
([7cd3a26](7cd3a26))
* **hana:** Remove encoding from hana-client streams
([#623](#623))
([fed8f6f](fed8f6f))
* **hana:** Support associations with static values
([#604](#604))
([05babcf](05babcf))
* improved `=` and `!=` with val `null` for HANA and Postgres
([#626](#626))
([cbcfe3b](cbcfe3b))
* Improved placeholders and limit clause
([#567](#567))
([d5d5dbb](d5d5dbb))
* Use json datatype for Postgres insert
([#582](#582))
([f1c9c89](f1c9c89))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: Johannes Vogel <johannes.vogel@sap.com>
Co-authored-by: Patrice Bender <patrice.bender@sap.com>
@cap-bots cap-bots mentioned this pull request Jul 18, 2024
Merged
This was referenced Jan 28, 2025
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stewsk stewsk stewsk approved these changes

@danjoa danjoa Awaiting requested review from danjoa danjoa is a code owner

@johannes-vogel johannes-vogel Awaiting requested review from johannes-vogel johannes-vogel is a code owner

@BobdenOs BobdenOs Awaiting requested review from BobdenOs BobdenOs is a code owner

@SamuelBrucksch SamuelBrucksch Awaiting requested review from SamuelBrucksch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@patricebender @stewsk