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: only consider persisted columns for simple operations #592

Merged
merged 8 commits into from
Apr 19, 2024
Merged

fix: only consider persisted columns for simple operations #592

merged 8 commits into from
Apr 19, 2024

Conversation

patricebender
Copy link
Member

@patricebender patricebender commented Apr 17, 2024 •
edited
Loading

it doesnt make sense to UPDATE, DELETE or to INSERT a virtual element, which only exists during runtime, such as:

  • virtual elements
  • calculated elements
  • associations

@patricebender
test: calculated elements should not be considered for simple ops
4e63d2a 
it doesnt make sense to `UPDATE`, `DELETE` or to `INSERT` a calculated element,
this change shows that it is done anyway
@BobdenOs
Copy link
Contributor

Might be solved by: #408

@BobdenOs
Copy link
Contributor

Looks like the update column check is not working correctly. As the snapshot tests show all update columns being removed. Probably the check should be inverted

@BobdenOs BobdenOs marked this pull request as ready for review April 18, 2024 10:09
@BobdenOs
Copy link
Contributor

@patricebender I guess now that the test is fixed the title should be updated?

@patricebender
Copy link
Member Author

@patricebender I guess now that the test is fixed the title should be updated?

If this is the proper fix, I'd be glad to merge this. The pr was originally only to showcase the issue.

@patricebender patricebender changed the title test: calculated elements should not be considered for simple ops fix: only consider persisted columns for simple operations Apr 18, 2024
johannes-vogel
johannes-vogel reviewed Apr 19, 2024
View reviewed changes
db-service/lib/cqn2sql.js
if (!elements || (c in elements && !elements[c].virtual)) {
if (cds.unfold && elements?.[c].is_struct) continue // skip structs from universal csn
const columnExistsInDatabase =
elements && c in elements && !elements[c].virtual && !elements[c].isAssociation && !elements[c].value
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
elements && c in elements && !elements[c].virtual && !elements[c].isAssociation && !elements[c].value
c in elements && !elements[c].virtual && !elements[c].isAssociation && !elements[c].value

check below ensures this already

Copy link
Member Author

Choose a reason for hiding this comment

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

If elements is undefined we'd get a type error:

> 'bar' in elements
VM240:1 Uncaught TypeError: Cannot use 'in' operator to search for 'bar' in undefined
    at <anonymous>:1:7

@patricebender patricebender enabled auto-merge (squash) April 19, 2024 10:55
@patricebender patricebender merged commit 6e31bda into main Apr 19, 2024
4 of 6 checks passed
@patricebender patricebender deleted the patrice/calc branch April 19, 2024 10:57
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BobdenOs BobdenOs BobdenOs approved these changes

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

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

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

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

Successfully merging this pull request may close these issues.
3 participants
@patricebender @BobdenOs @johannes-vogel