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

feat: reject update/delete without where #676

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SamuelBrucksch
Copy link
Contributor

@SamuelBrucksch SamuelBrucksch commented Jun 6, 2024

To discuss:

  • singletons -> check needed?
  • DELETE Foo(123)/bar -> would fail right now, should we check for to-one?

@SamuelBrucksch SamuelBrucksch force-pushed the reject-delete-update-all-by-default branch from 287c322 to 120464c Compare June 6, 2024 12:23
@SamuelBrucksch SamuelBrucksch changed the title reject update/delete without where feat: reject update/delete without where Jun 6, 2024
@BobdenOs BobdenOs added the next release pr to be checked for next release label Jun 14, 2024
@schiwekM
Copy link

To discuss:

  • singletons -> check needed?
  • DELETE Foo(123)/bar -> would fail right now, should we check for to-one?

I'd say for singletons not needed and to-one should be excluded as it is spec compliant

@johannes-vogel johannes-vogel removed the next release pr to be checked for next release label Sep 2, 2024
@@ -21,6 +21,14 @@ const BINARY_TYPES = {

class SQLService extends DatabaseService {
init() {
this.on(['UPDATE', 'DELETE'], ({ query}, next) => {
const cqn = query.UPDATE ?? query.DELETE
if (!cqn.where && !cqn.from?.ref?.at(-1).where && !cqn.entity?.ref?.at(-1).where) {
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
if (!cqn.where && !cqn.from?.ref?.at(-1).where && !cqn.entity?.ref?.at(-1).where) {
if (!cqn.where && !(cqn.from || cqn.entity)?.ref?.at(-1).where) {

Comment on lines +27 to +28
const op = query.DELETE ? 'delete' : 'update'
return cds.error(`Trying to ${op} all entites. Call .where(...) explicitely, to ${op} all entities.`)
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
const op = query.DELETE ? 'delete' : 'update'
return cds.error(`Trying to ${op} all entites. Call .where(...) explicitely, to ${op} all entities.`)
return cds.error(`${query.DELETE ? 'DELETE' : 'UPDATE'} without a where clause, which would affect all entites in the table. Add where 1=1 if your really want to do so`)

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