From 120464cff35096b939b1d6b13d351521e8c61747 Mon Sep 17 00:00:00 2001
From: Samuel Brucksch <samuel.brucksch@sap.com>
Date: Thu, 6 Jun 2024 14:22:21 +0200
Subject: [PATCH] reject update/delete without where

---
 db-service/lib/SQLService.js           | 36 ++++++++++++++++----------
 test/compliance/api.test.js            |  2 +-
 test/scenarios/bookshop/update.test.js |  2 +-
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/db-service/lib/SQLService.js b/db-service/lib/SQLService.js
index 9fc9f9f38..c7ed45226 100644
--- a/db-service/lib/SQLService.js
+++ b/db-service/lib/SQLService.js
@@ -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) {
+        const op = query.DELETE ? 'delete' : 'update'
+        return cds.error(`Trying to ${op} all entites. Call .where(...) explicitely, to ${op} all entities.`)
+      }
+      return next()
+    })
     this.on(['INSERT', 'UPSERT', 'UPDATE'], require('./fill-in-keys')) // REVISIT should be replaced by correct input processing eventually
     this.on(['INSERT', 'UPSERT', 'UPDATE'], require('./deep-queries').onDeep)
     if (cds.env.features.db_strict) {
@@ -33,8 +41,8 @@ class SQLService extends DatabaseService {
           operation.columns ||
           Object.keys(
             operation.data || operation.entries?.reduce((acc, obj) => {
-              return Object.assign(acc, obj)
-            }, {}),
+                return Object.assign(acc, obj)
+              }, {}),
           )
         const invalidColumns = columns.filter(c => !(c in elements))
 
@@ -232,8 +240,8 @@ class SQLService extends DatabaseService {
             } else if (visited.includes(c._target.name))
               throw new Error(
                 `Transitive circular composition detected: \n\n` +
-                `  ${visited.join(' > ')} > ${c._target.name} \n\n` +
-                `These are not supported by deep delete.`,
+                  `  ${visited.join(' > ')} > ${c._target.name} \n\n` +
+                  `These are not supported by deep delete.`,
               )
             // Prepare and run deep query, à la CQL`DELETE from Foo[pred]:comp1.comp2...`
             const query = DELETE.from({ ref: [...from.ref, c.name] })
@@ -461,16 +469,16 @@ const _target_name4 = q => {
 }
 
 const _unquirked = !cds.env.ql.quirks_mode ? q => q : q => {
-  if (!q) return q
-  else if (typeof q.SELECT?.from === 'string') q.SELECT.from = { ref: [q.SELECT.from] }
-  else if (typeof q.INSERT?.into === 'string') q.INSERT.into = { ref: [q.INSERT.into] }
-  else if (typeof q.UPSERT?.into === 'string') q.UPSERT.into = { ref: [q.UPSERT.into] }
-  else if (typeof q.UPDATE?.entity === 'string') q.UPDATE.entity = { ref: [q.UPDATE.entity] }
-  else if (typeof q.DELETE?.from === 'string') q.DELETE.from = { ref: [q.DELETE.from] }
-  else if (typeof q.CREATE?.entity === 'string') q.CREATE.entity = { ref: [q.CREATE.entity] }
-  else if (typeof q.DROP?.entity === 'string') q.DROP.entity = { ref: [q.DROP.entity] }
-  return q
-}
+      if (!q) return q
+      else if (typeof q.SELECT?.from === 'string') q.SELECT.from = { ref: [q.SELECT.from] }
+      else if (typeof q.INSERT?.into === 'string') q.INSERT.into = { ref: [q.INSERT.into] }
+      else if (typeof q.UPSERT?.into === 'string') q.UPSERT.into = { ref: [q.UPSERT.into] }
+      else if (typeof q.UPDATE?.entity === 'string') q.UPDATE.entity = { ref: [q.UPDATE.entity] }
+      else if (typeof q.DELETE?.from === 'string') q.DELETE.from = { ref: [q.DELETE.from] }
+      else if (typeof q.CREATE?.entity === 'string') q.CREATE.entity = { ref: [q.CREATE.entity] }
+      else if (typeof q.DROP?.entity === 'string') q.DROP.entity = { ref: [q.DROP.entity] }
+      return q
+    }
 
 const sqls = new (class extends SQLService {
   get factory() {
diff --git a/test/compliance/api.test.js b/test/compliance/api.test.js
index a66f912fc..574344f4b 100644
--- a/test/compliance/api.test.js
+++ b/test/compliance/api.test.js
@@ -23,7 +23,7 @@ const { expect } = cds.test(__dirname + '/resources')
   test('Update returns affected rows', async () => {
     const { count } = await SELECT.one`count(*)`.from('complex.associations.Books')
     
-    const affectedRows = await UPDATE.entity('complex.associations.Books').data({title: 'Book'})
+    const affectedRows = await UPDATE.entity('complex.associations.Books').data({title: 'Book'}).where('1=1')
     expect(affectedRows).to.be.eq(count)
   })
 
diff --git a/test/scenarios/bookshop/update.test.js b/test/scenarios/bookshop/update.test.js
index 0eb66d705..273b2c966 100644
--- a/test/scenarios/bookshop/update.test.js
+++ b/test/scenarios/bookshop/update.test.js
@@ -124,7 +124,7 @@ describe('Bookshop - Update', () => {
 
   test('programmatic update with unique constraint conflict', async () => {
     const { Genres } = cds.entities('sap.capire.bookshop')
-    const update = UPDATE(Genres).set('ID = 201')
+    const update = UPDATE(Genres).set('ID = 201').where('1=1')
     const err = await expect(update).rejected
     // Works fine locally, but refuses to function in pipeline
     // expect(err).to.be.instanceOf(Error)