Skip to content

Commit

Permalink
fix(sqlite): Retain Error object for unique constraint violation (#446
Browse files Browse the repository at this point in the history
)
  • Loading branch information
BobdenOs authored Feb 16, 2024
1 parent eb857de commit d27ee79
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 26 deletions.
22 changes: 22 additions & 0 deletions hana/lib/HANAService.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ class HANAService extends SQLService {
}

async onINSERT({ query, data }) {
try {
const { sql, entries, cqn } = this.cqn2sql(query, data)
if (!sql) return // Do nothing when there is nothing to be done
const ps = await this.prepare(sql)
Expand All @@ -134,6 +135,17 @@ class HANAService extends SQLService {
: ps.run(entries[0])
: ps.run())
return new this.class.InsertResults(cqn, results)
} catch (err) {
throw _not_unique(err, 'ENTITY_ALREADY_EXISTS')
}
}

async onUPDATE(req) {
try {
return await super.onUPDATE(req)
} catch (err) {
throw _not_unique(err, 'UNIQUE_CONSTRAINT_VIOLATION') || err
}
}

// Allow for running complex expand queries in a single statement
Expand Down Expand Up @@ -1090,6 +1102,16 @@ Buffer.prototype.toJSON = function () {
return this.toString('hex')
}

function _not_unique(err, code) {
if (err.code === 301)
return Object.assign(err, {
originalMessage: err.message, // FIXME: required because of next line
message: code, // FIXME: misusing message as code
code: 400, // FIXME: misusing code as (http) status
})
return err
}

const is_regexp = x => x?.constructor?.name === 'RegExp' // NOTE: x instanceof RegExp doesn't work in repl
const ObjectKeys = o => (o && [...ObjectKeys(o.__proto__), ...Object.keys(o)]) || []
const _managed = {
Expand Down
3 changes: 3 additions & 0 deletions hana/lib/drivers/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ const prom = function (dbc, func) {
return new Promise((resolve, reject) => {
dbc[func](...args, (err, res, output) => {
if (err) {
if (!(err instanceof Error)) {
Object.setPrototypeOf(err, Error.prototype)
}
const sql = typeof args[0] === 'string' && args[0]
// Enhance insufficient privilege errors with details
if (err.code === 258) {
Expand Down
53 changes: 39 additions & 14 deletions postgres/lib/PostgresService.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,14 +308,30 @@ GROUP BY k
return super.onSELECT({ query, data })
}

async onINSERT(req) {
try {
return await super.onINSERT(req)
} catch (err) {
throw _not_unique(err, 'ENTITY_ALREADY_EXISTS')
}
}

async onUPDATE(req) {
try {
return await super.onUPDATE(req)
} catch (err) {
throw _not_unique(err, 'UNIQUE_CONSTRAINT_VIOLATION')
}
}

static CQN2SQL = class CQN2Postgres extends SQLService.CQN2SQL {
_orderBy(orderBy, localized, locale) {
return orderBy.map(
localized
? c =>
this.expr(c) +
(c.element?.[this.class._localized] ? ` COLLATE "${locale}"` : '') +
(c.sort === 'desc' || c.sort === -1 ? ' DESC' : ' ASC')
this.expr(c) +
(c.element?.[this.class._localized] ? ` COLLATE "${locale}"` : '') +
(c.sort === 'desc' || c.sort === -1 ? ' DESC' : ' ASC')
: c => this.expr(c) + (c.sort === 'desc' || c.sort === -1 ? ' DESC' : ' ASC'),
)
}
Expand Down Expand Up @@ -365,9 +381,8 @@ GROUP BY k
})
// REVISIT: Remove SELECT ${cols} by adjusting SELECT_columns
let obj = `to_jsonb(${queryAlias}.*)`
return `SELECT ${
SELECT.one || SELECT.expand === 'root' ? obj : `coalesce(jsonb_agg (${obj}),'[]'::jsonb)`
} as _json_ FROM (SELECT ${cols} FROM (${sql}) as ${queryAlias}) as ${queryAlias}`
return `SELECT ${SELECT.one || SELECT.expand === 'root' ? obj : `coalesce(jsonb_agg (${obj}),'[]'::jsonb)`
} as _json_ FROM (SELECT ${cols} FROM (${sql}) as ${queryAlias}) as ${queryAlias}`
}

doubleQuote(name) {
Expand Down Expand Up @@ -552,14 +567,14 @@ class QueryStream extends Query {
this.stream = new Readable({
read: this.rows
? () => {
this.stream.pause()
// Request more rows
this.connection.execute({
portal: this.portal,
rows: this.rows,
})
this.connection.flush()
}
this.stream.pause()
// Request more rows
this.connection.execute({
portal: this.portal,
rows: this.rows,
})
this.connection.flush()
}
: () => {},
})
this.push = this.stream.push.bind(this.stream)
Expand Down Expand Up @@ -740,4 +755,14 @@ class ParameterStream extends Writable {
}
}

function _not_unique(err, code) {
if (err.code === '23505')
return Object.assign(err, {
originalMessage: err.message, // FIXME: required because of next line
message: code, // FIXME: misusing message as code
code: 400, // FIXME: misusing code as (http) status
})
return err
}

module.exports = PostgresService
9 changes: 5 additions & 4 deletions sqlite/lib/SQLiteService.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,21 +245,21 @@ class SQLiteService extends SQLService {
try {
return await super.onINSERT(req)
} catch (err) {
throw _not_unique(err, 'ENTITY_ALREADY_EXISTS') || err
throw _not_unique(err, 'ENTITY_ALREADY_EXISTS')
}
}

async onUPDATE(req) {
try {
return await super.onUPDATE(req)
} catch (err) {
throw _not_unique(err, 'UNIQUE_CONSTRAINT_VIOLATION') || err
throw _not_unique(err, 'UNIQUE_CONSTRAINT_VIOLATION')
}
}
}

// function _not_null (err) {
// if (err.code === "SQLITE_CONSTRAINT_NOTNULL") return Object.assign ({
// if (err.code === "SQLITE_CONSTRAINT_NOTNULL") return Object.assign (err, {
// code: 'MUST_NOT_BE_NULL',
// target: /\.(.*?)$/.exec(err.message)[1], // here we are even constructing OData responses, with .target
// message: 'Value is required',
Expand All @@ -268,11 +268,12 @@ class SQLiteService extends SQLService {

function _not_unique(err, code) {
if (err.message.match(/unique constraint/i))
return Object.assign({
return Object.assign(err, {
originalMessage: err.message, // FIXME: required because of next line
message: code, // FIXME: misusing message as code
code: 400, // FIXME: misusing code as (http) status
})
return err
}

module.exports = SQLiteService
1 change: 1 addition & 0 deletions test/scenarios/bookshop/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require('./read.test')
require('./insert.test')
require('./delete.test')
require('./update.test')
require('./funcs.test')
Expand Down
16 changes: 16 additions & 0 deletions test/scenarios/bookshop/insert.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
const cds = require('../../cds.js')
const bookshop = cds.utils.path.resolve(__dirname, '../../bookshop')

describe('Bookshop - Insert', () => {
const { expect } = cds.test(bookshop)

test('unique constraing violation throws error', async () => {
const { Books } = cds.entities('AdminService')
const insert = INSERT({ ID: 201, title: 'Harry Potter' }).into(Books)
const err = await expect(insert).rejected
// Works fine locally, but refuses to function in pipeline
// expect(err).to.be.instanceOf(Error)
// expect(err instanceof Error).to.be.true
expect(err.message).to.be.eq('ENTITY_ALREADY_EXISTS')
})
})
31 changes: 23 additions & 8 deletions test/scenarios/bookshop/update.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ describe('Bookshop - Update', () => {
})

test('Update array of', async () => {
const { Books } = cds.entities('sap.capire.bookshop')
// create book
const insert = INSERT.into('sap.capire.bookshop.Books').columns(['ID']).values([150])
const insert = INSERT.into(Books).columns(['ID']).values([150])
await cds.run(insert)

const update = await PUT(
Expand Down Expand Up @@ -107,31 +108,45 @@ describe('Bookshop - Update', () => {
})

test('programmatic update without body incl. managed', async () => {
const { modifiedAt } = await SELECT.from('sap.capire.bookshop.Books', { ID: 251 })
const affectedRows = await UPDATE('sap.capire.bookshop.Books', { ID: 251 })
const { Books } = cds.entities('sap.capire.bookshop')
const { modifiedAt } = await SELECT.from(Books, { ID: 251 })
const affectedRows = await UPDATE(Books, { ID: 251 })
expect(affectedRows).to.be.eq(1)
const { modifiedAt: newModifiedAt } = await SELECT.from('sap.capire.bookshop.Books', { ID: 251 })
const { modifiedAt: newModifiedAt } = await SELECT.from(Books, { ID: 251 })
expect(newModifiedAt).not.to.be.eq(modifiedAt)
})

test('programmatic update without body excl. managed', async () => {
const affectedRows = await UPDATE('sap.capire.bookshop.Genres', { ID: 10 })
const { Genres } = cds.entities('sap.capire.bookshop')
const affectedRows = await UPDATE(Genres, { ID: 10 })
expect(affectedRows).to.be.eq(0)
})

test('programmatic update with unique constraint conflict', async () => {
const { Genres } = cds.entities('sap.capire.bookshop')
const update = UPDATE(Genres).set('ID = 201')
const err = await expect(update).rejected
// Works fine locally, but refuses to function in pipeline
// expect(err).to.be.instanceOf(Error)
// expect(err instanceof Error).to.be.true
expect(err.message).to.be.eq('UNIQUE_CONSTRAINT_VIOLATION')
})


test('Update with path expressions', async () => {
const updateRichardsBooks = UPDATE.entity('AdminService.RenameKeys')
const { RenameKeys } = cds.entities('AdminService')
const updateRichardsBooks = UPDATE.entity(RenameKeys)
.where(`author.name = 'Richard Carpenter'`)
.set('ID = 42')
const selectRichardsBooks = CQL`SELECT * FROM AdminService.RenameKeys where author.name = 'Richard Carpenter'`
const selectRichardsBooks = CQL`SELECT * FROM ${RenameKeys} where author.name = 'Richard Carpenter'`

await cds.run(updateRichardsBooks)
const afterUpdate = await cds.db.run(selectRichardsBooks)
expect(afterUpdate[0]).to.have.property('foo').that.equals(42)
})

test('Upsert draft enabled entity', async () => {
const res = await UPSERT.into('DraftService.DraftEnabledBooks').entries({ID: 42, title: 'Foo'})
const res = await UPSERT.into('DraftService.DraftEnabledBooks').entries({ ID: 42, title: 'Foo' })
expect(res).to.equal(1)
})
})

0 comments on commit d27ee79

Please sign in to comment.