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.

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

fix: deep delete for views without navigation #434

Merged
merged 41 commits into from
Mar 5, 2024
Merged

Conversation

larslutz96
Copy link
Contributor

@larslutz96 larslutz96 commented Jan 31, 2024

Support deep delete when navigation is not included in the view.
Rewrite first incoming query in deep_delete() to a delete from database table where keys in select from view and call onDELETE again with this query.

db-service/lib/SQLService.js Outdated Show resolved Hide resolved
test/compliance/resources/srv/index.cds Outdated Show resolved Hide resolved
@larslutz96 larslutz96 requested a review from BobdenOs February 27, 2024 13:50
Copy link
Contributor

@BobdenOs BobdenOs left a comment

Choose a reason for hiding this comment

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

Looks good. Still very compact solution.

db-service/lib/SQLService.js Outdated Show resolved Hide resolved
db-service/lib/SQLService.js Show resolved Hide resolved
@larslutz96 larslutz96 requested a review from BobdenOs February 28, 2024 08:38
@larslutz96
Copy link
Contributor Author

larslutz96 commented Feb 28, 2024

@BobdenOs
Johannes mentioned a scenario:

service S {
  entity RootP as projection on Root {
        key ID,
        children
    };

   entity ChildP as projection on Child where x = 1
}

DELETE.from(RootP(5)) deletes root with id 5 and all children. Johannes said:
I think that this is not what we want. The compiler redirects the composition of RootP to the filtered projection ChildP. Hence, I believe we must respect the modeled where clause and must not delete children which are excluded by the projection.

It also contradicts the behavior of deep update where we compare the data on projection level which respects the where clause.

Copy link
Member

@patricebender patricebender left a comment

Choose a reason for hiding this comment

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

👍 before merging, would you please update the PR title and explain your approach in the PR description in more detail? Thanks!

@larslutz96 larslutz96 changed the title fix: deep delete fix: deep delete for views without navigation Feb 28, 2024
db-service/lib/SQLService.js Outdated Show resolved Hide resolved
@BobdenOs BobdenOs self-requested a review February 29, 2024 14:18
@larslutz96 larslutz96 requested a review from patricebender March 4, 2024 15:09
@larslutz96 larslutz96 merged commit 3ebc9c2 into main Mar 5, 2024
4 checks passed
@larslutz96 larslutz96 deleted the fix-deep-delete branch March 5, 2024 09:31
@cap-bots cap-bots mentioned this pull request Mar 5, 2024
danjoa added a commit that referenced this pull request Mar 5, 2024
@danjoa danjoa restored the fix-deep-delete branch March 5, 2024 12:03
@danjoa
Copy link
Contributor

danjoa commented Mar 5, 2024

Had to revert this commit as it broke our internal pipelines du to the incompatible override of db.exists().
I'd also like to have a review of the implementation: I'm not in favour of reusing more internal @sap/cds functions

patricebender added a commit that referenced this pull request Mar 22, 2024
🤖 I have created a release *beep* *boop*
---


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

##
[1.7.0](db-service-v1.6.4...db-service-v1.7.0)
(2024-03-22)


### Added

* also support lowercase matchespattern function
([#528](#528))
([6ea574e](6ea574e))
* forUpdate and forShareLock
([#148](#148))
([99a1170](99a1170))
* **hana:** drop prepared statements after end of transaction
([#537](#537))
([b1f864e](b1f864e))
* **orderby:** allow to disable collations with
[@cds](https://github.com/cds).collate: false
([#492](#492))
([820f971](820f971))


### Fixed

* **cqn2sql:** Smart quoting of columns inside UPSERT rows
([#519](#519))
([78fe10b](78fe10b))
* deep delete for views without navigation
([#434](#434))
([3ebc9c2](3ebc9c2))
* Getting rid of quirks mode
([#514](#514))
([c9aa6e8](c9aa6e8))
* issue with reused select cqns
([#505](#505))
([916d175](916d175))
* joins without columns are rejected
([#535](#535))
([eb9beda](eb9beda))
* **search:** dont search non string aggregations
([#527](#527))
([c87900c](c87900c))
* **search:** search on aggregated results in HAVING clause
([#524](#524))
([61d348e](61d348e))
</details>

<details><summary>sqlite: 1.6.0</summary>

##
[1.6.0](sqlite-v1.5.1...sqlite-v1.6.0)
(2024-03-22)


### Added

* forUpdate and forShareLock
([#148](#148))
([99a1170](99a1170))
* **hana:** drop prepared statements after end of transaction
([#537](#537))
([b1f864e](b1f864e))


### Fixed

* **`sqlite`:** use keyword list from compiler
([#526](#526))
([a227c61](a227c61))
</details>

<details><summary>postgres: 1.6.0</summary>

##
[1.6.0](postgres-v1.5.1...postgres-v1.6.0)
(2024-03-22)


### Added

* also support lowercase matchespattern function
([#528](#528))
([6ea574e](6ea574e))
* forUpdate and forShareLock
([#148](#148))
([99a1170](99a1170))


### Changed

* use new cds build API @sap/cds-dk &gt;= 7.5.0
([#508](#508))
([ef22ebe](ef22ebe))
</details>

<details><summary>hana: 0.1.0</summary>

##
[0.1.0](hana-v0.0.6...hana-v0.1.0)
(2024-03-22)


### Added

* also support lowercase matchespattern function
([#528](#528))
([6ea574e](6ea574e))
* forUpdate and forShareLock
([#148](#148))
([99a1170](99a1170))
* **hana:** drop prepared statements after end of transaction
([#537](#537))
([b1f864e](b1f864e))


### Fixed

* **`hana`:** use keyword list from compiler
([#525](#525))
([c6993d9](c6993d9))
* **hana:** improve search inside where clause detection
([#538](#538))
([51b8af3](51b8af3))
* **hana:** reduce service manager calls for failing tenants
([#533](#533))
([e95fd17](e95fd17))
* issue with reused select cqns
([#505](#505))
([916d175](916d175))
* joins without columns are rejected
([#535](#535))
([eb9beda](eb9beda))
* mass insert for unknown entities
([#540](#540))
([f2ea4af](f2ea4af))
</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 <31311694+johannes-vogel@users.noreply.github.com>
Co-authored-by: Patrice Bender <patrice.bender@sap.com>
@cap-bots cap-bots mentioned this pull request Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants