Inserting with skipOnConflict on whole table yields incorrect sql #63

alfateam · Nov 7, 2023 · 740f0ef · 740f0ef
1 parent e4760a8
commit 740f0ef
Show file tree

Hide file tree

Showing 8 changed files with 277 additions and 20 deletions.
diff --git a/src/client/index.js b/src/client/index.js
@@ -290,7 +290,9 @@ function rdbClient(options = {}) {
 			return adapter.post(body);
 		}
 
-		async function insert(rows, ...args) {
+		async function insert(rows, ...rest) {
+			const strategy = undefined;
+			const args = [strategy].concat(rest);
 			if (Array.isArray(rows)) {
 				let proxy = proxify([], args[0]);
 				proxy.splice.apply(proxy, [0, 0, ...rows]);
@@ -459,7 +461,7 @@ function rdbClient(options = {}) {
 		async function saveArray(array, concurrencyOptions, strategy) {
 			let deduceStrategy;
 			if (arguments.length < 3)
-				deduceStrategy = true;
+				deduceStrategy = false;
 			let { json } = rootMap.get(array);
 			strategy = extractStrategy({ strategy }, array);
 			strategy = extractFetchingStrategy(array, strategy);
@@ -486,7 +488,7 @@ function rdbClient(options = {}) {
 		async function patch(patch, concurrencyOptions, strategy) {
 			let deduceStrategy;
 			if (arguments.length < 3)
-				deduceStrategy = true;
+				deduceStrategy = false;
 			if (patch.length === 0)
 				return;
 			let body = stringify({ patch, options: { strategy, ...concurrencyOptions, deduceStrategy } });
@@ -661,7 +663,7 @@ function rdbClient(options = {}) {
 		async function saveRow(row, concurrencyOptions, strategy) {
 			let deduceStrategy;
 			if (arguments.length < 3)
-				deduceStrategy = true;
+				deduceStrategy = false;
 			strategy = extractStrategy({ strategy }, row);
 			strategy = extractFetchingStrategy(row, strategy);
 

diff --git a/src/mySql/insertSql.js b/src/mySql/insertSql.js
@@ -41,6 +41,10 @@ function insertSql(table, row, options) {
 				values.push('%s');
 				addConflictUpdate(column);
 			}
+		}		
+		if (conflictColumnUpdates.length === 0) {
+			const column = table._primaryColumns[0];
+			conflictColumnUpdates.push(`${column._dbName}=VALUES(${column._dbName})`);
 		}
 		conflictColumnUpdateSql = conflictColumnUpdates.join(',');
 

diff --git a/src/pg/insertSql.js b/src/pg/insertSql.js
@@ -12,13 +12,13 @@ function insertSql(table, row, options) {
 	if (columnNames.length === 0)
 		sql += `${outputInserted()}DEFAULT VALUES ${lastInsertedSql(table)}`;
 	else
-		sql = sql + '('+ columnNames.join(',') + ') ' + outputInserted() +  'VALUES (' + values.join(',') + ')' + onConflict() + lastInsertedSql(table);
+		sql = sql + '(' + columnNames.join(',') + ') ' + outputInserted() + 'VALUES (' + values.join(',') + ')' + onConflict() + lastInsertedSql(table);
 	return sql;
 
 	function onConflict() {
 		if (options.concurrency === 'skipOnConflict' || options.concurrency === 'overwrite') {
 			const primaryKeys = table._primaryColumns.map(x => x._dbName).join(',');
-			return ` ON CONFLICT(${primaryKeys}) DO UPDATE SET ${conflictColumnUpdateSql} `;
+			return ` ON CONFLICT(${primaryKeys}) ${conflictColumnUpdateSql} `;
 		}
 		else return '';
 	}
@@ -44,7 +44,10 @@ function insertSql(table, row, options) {
 				addConflictUpdate(column);
 			}
 		}
-		conflictColumnUpdateSql = conflictColumnUpdates.join(',');
+		if (conflictColumnUpdates.length === 0)
+			conflictColumnUpdateSql = 'DO NOTHING';
+		else
+			conflictColumnUpdateSql = 'DO UPDATE SET ' + conflictColumnUpdates.join(',');
 
 		function addConflictUpdate(column) {
 			let concurrency = options[column.alias]?.concurrency || options.concurrency;

diff --git a/src/sap/mergeSql.js b/src/sap/mergeSql.js
@@ -6,7 +6,12 @@ function insertSql(table, row, options) {
 	addDiscriminators();
 	addColumns();
 
-	let sql = `MERGE INTO ${table._dbName} AS target USING (SELECT ${values.join(',')}) AS source ON ${join()} WHEN MATCHED THEN ${whenMatched()} WHEN NOT MATCHED THEN ${whenNotMatched()};`;
+	const matched = whenMatched();
+	let sql;
+	if (matched)
+		sql = `MERGE INTO ${table._dbName} AS target USING (SELECT ${values.join(',')}) AS source ON ${join()} WHEN MATCHED THEN ${matched} WHEN NOT MATCHED THEN ${whenNotMatched()};`;
+	else
+		sql = `MERGE INTO ${table._dbName} AS target USING (SELECT ${values.join(',')}) AS source ON ${join()} WHEN NOT MATCHED THEN ${whenNotMatched()};`;
 
 	return sql;
 
@@ -22,7 +27,7 @@ function insertSql(table, row, options) {
 
 	function whenMatched() {
 		if (options.concurrency === 'skipOnConflict' || options.concurrency === 'overwrite') {
-			return `UPDATE SET ${conflictColumnUpdateSql}`;
+			return conflictColumnUpdateSql;
 		}
 		else return '';
 	}
@@ -52,7 +57,8 @@ function insertSql(table, row, options) {
 				addConflictUpdate(column);
 			}
 		}
-		conflictColumnUpdateSql = conflictColumnUpdates.join(',');
+		if (conflictColumnUpdates.length > 0)
+			conflictColumnUpdateSql = 'UPDATE SET ' + conflictColumnUpdates.join(',');
 
 		function addConflictUpdate(column) {
 			let concurrency = options[column.alias]?.concurrency || options.concurrency;

diff --git a/src/sqlite/insertSql.js b/src/sqlite/insertSql.js
@@ -19,7 +19,7 @@ function insertSql(table, row, options) {
 	function onConflict() {
 		if (options.concurrency === 'skipOnConflict' || options.concurrency === 'overwrite') {
 			const primaryKeys = table._primaryColumns.map(x => x._dbName).join(',');
-			return ` ON CONFLICT(${primaryKeys}) DO UPDATE SET ${conflictColumnUpdateSql}`;
+			return ` ON CONFLICT(${primaryKeys}) ${conflictColumnUpdateSql}`;
 		} else {
 			return '';
 		}
@@ -46,7 +46,10 @@ function insertSql(table, row, options) {
 				addConflictUpdate(column);
 			}
 		}
-		conflictColumnUpdateSql = conflictColumnUpdates.join(',');
+		if (conflictColumnUpdates.length === 0)
+			conflictColumnUpdateSql =  'DO NOTHING';
+		else 
+			conflictColumnUpdateSql = 'DO UPDATE SET ' + conflictColumnUpdates.join(',');
 
 		function addConflictUpdate(column) {
 			let concurrency = options[column.alias]?.concurrency || options.concurrency;

diff --git a/src/tedious/mergeSql.js b/src/tedious/mergeSql.js
@@ -8,8 +8,12 @@ function insertSql(table, row, options) {
 	addDiscriminators();
 	addColumns();
 
-	let sql = `MERGE INTO ${table._dbName} AS target USING (SELECT ${values.join(',')}) AS source ON ${join()} WHEN MATCHED THEN ${whenMatched()} WHEN NOT MATCHED THEN ${whenNotMatched()} ${outputInsertedSql(table)};`;
-
+	const matched = whenMatched();
+	let sql;
+	if (matched)
+		sql = `MERGE INTO ${table._dbName} AS target USING (SELECT ${values.join(',')}) AS source ON ${join()} WHEN MATCHED THEN ${matched} WHEN NOT MATCHED THEN ${whenNotMatched()} ${outputInsertedSql(table)};`;
+	else
+		sql = `MERGE INTO ${table._dbName} AS target USING (SELECT ${values.join(',')}) AS source ON ${join()} WHEN NOT MATCHED THEN ${whenNotMatched()} ${outputInsertedSql(table)};`;
 	return sql;
 
 	function join() {
@@ -24,7 +28,7 @@ function insertSql(table, row, options) {
 
 	function whenMatched() {
 		if (options.concurrency === 'skipOnConflict' || options.concurrency === 'overwrite') {
-			return `UPDATE SET ${conflictColumnUpdateSql}`;
+			return conflictColumnUpdateSql;
 		}
 		else return '';
 	}
@@ -54,7 +58,10 @@ function insertSql(table, row, options) {
 				addConflictUpdate(column);
 			}
 		}
-		conflictColumnUpdateSql = conflictColumnUpdates.join(',');
+
+		if (conflictColumnUpdates.length > 0)
+			conflictColumnUpdateSql = 'UPDATE SET ' + conflictColumnUpdates.join(',');
+
 
 		function addConflictUpdate(column) {
 			let concurrency = options[column.alias]?.concurrency || options.concurrency;

diff --git a/tests/conflicts.test.js b/tests/conflicts.test.js
@@ -11,10 +11,7 @@ const versionArray = process.version.replace('v', '').split('.');
 const major = parseInt(versionArray[0]);
 const port = 3007;
 
-
-
 beforeAll(async () => {
-
 	await createMs('mssql');
 
 	async function createMs(dbName) {
@@ -29,7 +26,6 @@ beforeAll(async () => {
 });
 
 describe('optimistic fail', () => {
-
 	test('pg', async () => await verify('pg'));
 	test('mssql', async () => await verify('mssql'));
 	if (major > 17)
@@ -113,6 +109,98 @@ describe('insert skipOnConflict with overwrite column', () => {
 	}
 });
 
+describe('savechanges overload overwrite', () => {
+	test('pg', async () => await verify('pg'));
+	test('mssql', async () => await verify('mssql'));
+	if (major > 17)
+		test('mssqlNative', async () => await verify('mssqlNative'));
+	test('mysql', async () => await verify('mysql'));
+	test('sqlite', async () => await verify('sqlite'));
+	test('sap', async () => await verify('sap'));
+
+	async function verify(dbName) {
+		let { db, init } = getDb(dbName);
+		await init(db);
+
+		db = db({
+			vendor: {
+				concurrency: 'skipOnConflict',
+			},
+		});
+
+		const george = await db.vendor.insert({
+			id: 1,
+			name: 'John',
+			balance: 100,
+			isActive: true
+		});
+
+		const george2 = await db.vendor.getById(1);
+
+		george.name = 'John 1';
+		await george.saveChanges();
+
+		george2.name = 'John 2'
+		await george2.saveChanges({ name: { concurrency: 'overwrite' } });
+
+		const expected = {
+			id: 1,
+			name: 'John 2',
+			balance: 100,
+			isActive: true
+		};
+
+		await george.refresh();
+
+		expect(george2).toEqual(expected);
+		expect(george).toEqual(expected);
+	}
+});
+
+describe('savechanges overload optimistic', () => {
+	rdb.on('query', console.dir);
+	test('pg', async () => await verify('pg'));
+	test('mssql', async () => await verify('mssql'));
+	if (major > 17)
+		test('mssqlNative', async () => await verify('mssqlNative'));
+	test('mysql', async () => await verify('mysql'));
+	test('sqlite', async () => await verify('sqlite'));
+	test('sap', async () => await verify('sap'));
+
+	async function verify(dbName) {
+		let { db, init } = getDb(dbName);
+		await init(db);
+
+		db = db({
+			vendor: {
+				concurrency: 'skipOnConflict',
+			},
+		});
+
+		const george = await db.vendor.insert({
+			id: 1,
+			name: 'John',
+			balance: 100,
+			isActive: true
+		});
+
+		const george2 = await db.vendor.getById(1);
+
+		george.name = 'John 1';
+		await george.saveChanges();
+
+		george2.name = 'John 2'
+		let error;
+		try {
+			await george2.saveChanges({ name: { concurrency: 'optimistic' } });
+		}
+		catch (e) {
+			error = e;
+		}
+		expect(error.message).toEqual(`The field name was changed by another user. Expected 'John', but was 'John 1'.`);
+	}
+});
+
 describe('insert empty skipOnConflict', () => {
 	test('pg', async () => await verify('pg'));
 	test('mssql', async () => await verify('mssql'));