From 120464cff35096b939b1d6b13d351521e8c61747 Mon Sep 17 00:00:00 2001 From: Samuel Brucksch Date: Thu, 6 Jun 2024 14:22:21 +0200 Subject: [PATCH 1/6] 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) From fea37294242a8df7ddec59be8506507c5e3eeee7 Mon Sep 17 00:00:00 2001 From: Samuel Brucksch Date: Thu, 6 Jun 2024 14:25:19 +0200 Subject: [PATCH 2/6] formatting --- db-service/lib/SQLService.js | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/db-service/lib/SQLService.js b/db-service/lib/SQLService.js index c7ed45226..a745a9a35 100644 --- a/db-service/lib/SQLService.js +++ b/db-service/lib/SQLService.js @@ -41,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)) @@ -240,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] }) @@ -469,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() { From 8141bb3edd4db63e265750cb9c7c41f9f9662f9d Mon Sep 17 00:00:00 2001 From: Samuel Brucksch Date: Thu, 6 Jun 2024 15:29:05 +0200 Subject: [PATCH 3/6] add explicit delete all to tests --- db-service/test/cqn2sql/__snapshots__/delete.test.js.snap | 2 +- db-service/test/cqn2sql/delete.test.js | 2 +- sqlite/test/general/stream.compat.test.js | 4 ++-- sqlite/test/general/stream.test.js | 4 ++-- sqlite/test/general/uuid.test.js | 4 ++-- test/compliance/SELECT.test.js | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/db-service/test/cqn2sql/__snapshots__/delete.test.js.snap b/db-service/test/cqn2sql/__snapshots__/delete.test.js.snap index 98c13e2e9..4039a2090 100644 --- a/db-service/test/cqn2sql/__snapshots__/delete.test.js.snap +++ b/db-service/test/cqn2sql/__snapshots__/delete.test.js.snap @@ -6,7 +6,7 @@ exports[`delete test simple cascade delete for entity with 'not exists' 1`] = `" exports[`delete test simple cascade delete for entity with 'not in' 1`] = `"DELETE FROM Foo as Foo WHERE Foo.x not in (SELECT Foo2.a FROM Foo2 as Foo2)"`; -exports[`delete test with from entity 1`] = `"DELETE FROM Foo as Foo"`; +exports[`delete test with from entity 1`] = `"DELETE FROM Foo as Foo WHERE ? = ?"`; exports[`delete test with from ref 1`] = `"DELETE FROM Foo as Foo"`; diff --git a/db-service/test/cqn2sql/delete.test.js b/db-service/test/cqn2sql/delete.test.js index 2b89725e7..c47831fa1 100644 --- a/db-service/test/cqn2sql/delete.test.js +++ b/db-service/test/cqn2sql/delete.test.js @@ -10,7 +10,7 @@ beforeAll(async () => { }) describe('delete', () => { test('test with from entity', () => { - const cqnDelete = DELETE.from(cds.model.definitions.Foo) + const cqnDelete = DELETE.from(cds.model.definitions.Foo).where('1=1') const { sql } = cqn2sql(cqnDelete) expect(sql).toMatchSnapshot() }) diff --git a/sqlite/test/general/stream.compat.test.js b/sqlite/test/general/stream.compat.test.js index 1e5006dba..6e08d0eb5 100644 --- a/sqlite/test/general/stream.compat.test.js +++ b/sqlite/test/general/stream.compat.test.js @@ -25,7 +25,7 @@ describe('streaming', () => { afterAll(async () => { const { Images } = cds.entities('test') - await DELETE.from(Images) + await DELETE.from(Images).where('1=1') }) test('READ stream property with .from and .where', async () => { @@ -101,7 +101,7 @@ describe('streaming', () => { afterAll(async () => { const { Images } = cds.entities('test') - await DELETE.from(Images) + await DELETE.from(Images).where('1=1') }) describe('READ', () => { diff --git a/sqlite/test/general/stream.test.js b/sqlite/test/general/stream.test.js index 3d776a3ef..b6564c6d2 100644 --- a/sqlite/test/general/stream.test.js +++ b/sqlite/test/general/stream.test.js @@ -23,7 +23,7 @@ describe('streaming', () => { afterAll(async () => { const { Images } = cds.entities('test') - await DELETE.from(Images) + await DELETE.from(Images).where('1=1') }) test('READ stream property with .from and .where', async () => cds.tx(async () => { @@ -105,7 +105,7 @@ describe('streaming', () => { afterAll(async () => { const { Images } = cds.entities('test') - await DELETE.from(Images) + await DELETE.from(Images).where('1=1') }) describe('READ', () => { diff --git a/sqlite/test/general/uuid.test.js b/sqlite/test/general/uuid.test.js index 2c449cbba..247eff816 100644 --- a/sqlite/test/general/uuid.test.js +++ b/sqlite/test/general/uuid.test.js @@ -10,7 +10,7 @@ describe('UUID Generation', () => { const result = await SELECT.from('test.bar') expect(result).toEqual([{ ID: expect.any(String) }]) - await DELETE('test.bar') + await DELETE('test.bar').where('1=1') }) }) test('INSERT with multiple entries', async () => { @@ -22,7 +22,7 @@ describe('UUID Generation', () => { expect(result).toEqual([{ ID: expect.any(String) }, { ID: expect.any(String) }]) expect(result[0].ID).not.toEqual(result[1].ID) - await DELETE('test.bar') + await DELETE('test.bar').where('1=1') }) }) diff --git a/test/compliance/SELECT.test.js b/test/compliance/SELECT.test.js index d4b707974..53516f4c7 100644 --- a/test/compliance/SELECT.test.js +++ b/test/compliance/SELECT.test.js @@ -255,7 +255,7 @@ describe('SELECT', () => { const entity = `basic.literals.dateTime` const dateTime = '1970-02-02T10:09:34Z' const timestamp = dateTime.slice(0, -1) + '.000Z' - await DELETE.from(entity) + await DELETE.from(entity).where('1=1') await INSERT({ dateTime }).into(entity) const dateTimeMatches = await SELECT('dateTime').from(entity).where(`dateTime = `, dateTime) assert.strictEqual(dateTimeMatches.length, 1, 'Ensure that the dateTime column matches the dateTime value') From 2c284d431513063c232e7d694b14d826a6f0d373 Mon Sep 17 00:00:00 2001 From: Samuel Brucksch Date: Thu, 6 Jun 2024 15:32:24 +0200 Subject: [PATCH 4/6] add more checks --- db-service/lib/SQLService.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db-service/lib/SQLService.js b/db-service/lib/SQLService.js index a745a9a35..d082c955d 100644 --- a/db-service/lib/SQLService.js +++ b/db-service/lib/SQLService.js @@ -23,7 +23,7 @@ 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) { + 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.`) } From 6948889c8f8b5ff85b4b90107a1dc02e28c4130a Mon Sep 17 00:00:00 2001 From: Samuel Brucksch Date: Thu, 6 Jun 2024 15:38:53 +0200 Subject: [PATCH 5/6] more --- db-service/test/cqn4sql/DELETE.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db-service/test/cqn4sql/DELETE.test.js b/db-service/test/cqn4sql/DELETE.test.js index fa772a502..7b11cc676 100644 --- a/db-service/test/cqn4sql/DELETE.test.js +++ b/db-service/test/cqn4sql/DELETE.test.js @@ -21,7 +21,7 @@ describe('DELETE', () => { it('DELETE with where exists expansion', () => { const { DELETE } = cds.ql - let d = DELETE.from('bookshop.Books:author') + let d = DELETE.from('bookshop.Books:author').where('1=1') const query = cqn4sql(d, model) // how to express this in CQN? // DELETE.from({ref: ['bookshop.Authors'], as: 'author'}).where('exists ( SELECT 1 from bookshop.Books as Books where author_ID = author.ID)') @@ -186,7 +186,7 @@ describe('DELETE', () => { it('DELETE with assoc filter and where exists expansion', () => { const { DELETE } = cds.ql - let d = DELETE.from('bookshop.Reproduce[author = null and ID = 99]:accessGroup') + let d = DELETE.from('bookshop.Reproduce[author = null and ID = 99]:accessGroup').where('1=1') const query = cqn4sql(d, model) const expected = { From 504006292accfc7e709f527f038133901c7badc3 Mon Sep 17 00:00:00 2001 From: Samuel Brucksch Date: Fri, 7 Jun 2024 09:18:33 +0200 Subject: [PATCH 6/6] fix another test --- postgres/test/ql.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/postgres/test/ql.test.js b/postgres/test/ql.test.js index 50e41a017..2bd99c494 100644 --- a/postgres/test/ql.test.js +++ b/postgres/test/ql.test.js @@ -180,7 +180,7 @@ describe('QL to PostgreSQL', () => { test('-> multiple rows', async () => { const { Beers } = cds.entities('csw') - const affectedRows = await cds.run(UPDATE(Beers).set({ abv: 1.0 })) + const affectedRows = await cds.run(UPDATE(Beers).set({ abv: 1.0 })).where('1=1') expect(affectedRows).to.equal(11) }) })