From 807739ea7a60d8fdb25588874ac19d1f0135cf2a Mon Sep 17 00:00:00 2001 From: GermanBluefox Date: Sat, 3 Oct 2026 07:27:45 +0300 Subject: [PATCH] Lock in the duplicate (id, ts) guard (#304) #304 is a different bug from #294 despite the comment there: not two statements in one query, but two rows with the same (id, ts) in one INSERT - the same datapoint logged from two sources that landed on the same millisecond - which aborted the whole batch with a primary key violation. It is already fixed. MySQL carries `ON DUPLICATE KEY UPDATE id=id`, PostgreSQL and SQLite `ON CONFLICT DO NOTHING`. MS SQL has nothing, and that is correct: init() creates a plain `CREATE INDEX i_id on ...(id, ts)` rather than a primary key, so no uniqueness conflict can arise there. Nothing tested any of it. test/testInsertStatements.js now asserts the guard per dialect, including the deliberate absence for MS SQL, so that someone levelling the dialects does not add one there. It also pins DO NOTHING against DO UPDATE: the duplicate sits *inside* one statement, which DO NOTHING handles but DO UPDATE answers with "cannot affect row a second time" - the obvious way to make the last value win would bring the bug straight back. Since the issue was reported on MariaDB, that dialect gets an executable check rather than a string match. test/testMySQLInsertDuplicates.js talks to the server directly - no js-controller, no adapter instance - creates its own throwaway database, and covers three things: the reported batch inserts cleanly, the same batch without the guard still fails with ER_DUP_ENTRY '4-1681102802872', and a genuinely broken statement still raises ER_BAD_FIELD_ERROR. That last one is why the guard is not INSERT IGNORE, as proposed in #407: IGNORE would swallow it. It is picked up by the MySQL job's existing `test/testMySQL*.js` glob and sorts after testMySQLExistingNoNulls.js, so the documented order of the existing-data tests is unaffected. Without a reachable server the cases skip with the reason, so the glob stays usable locally. Verified against MariaDB 10.11.14: 3 passing with a server, 3 pending without. Co-Authored-By: Claude Opus 5 (1M context) --- test/testInsertStatements.js | 82 ++++++++++++++++++++ test/testMySQLInsertDuplicates.js | 125 ++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+) create mode 100644 test/testMySQLInsertDuplicates.js diff --git a/test/testInsertStatements.js b/test/testInsertStatements.js index b68e7e6..0912c21 100644 --- a/test/testInsertStatements.js +++ b/test/testInsertStatements.js @@ -81,3 +81,85 @@ describe('Test insert() never concatenates statements', function () { }); } }); + +// Issue #304: two rows with the same (id, ts) in one batch - the same datapoint logged from two +// sources that happened to land on the same millisecond - used to abort the whole INSERT with a +// primary key violation. +const DUPLICATE_GUARD = { + mysql: /ON DUPLICATE KEY UPDATE/i, + postgresql: /ON CONFLICT DO NOTHING/i, + sqlite: /ON CONFLICT DO NOTHING/i, + // No guard, and that is correct: init() creates `CREATE INDEX i_id on ...(id, ts)`, a plain + // non-unique index rather than a primary key, so no uniqueness conflict can arise. Adding one + // here to make the dialects look alike would be wrong. + mssql: null, +}; + +/** The batch from issue #304: ts 1681102802872 appears twice for id 4, from two different sources */ +function duplicateBatch() { + return [ + { table: 'ts_number', state: { val: 54.7, ts: 1681102502871, ack: true }, from: 2 }, + { table: 'ts_number', state: { val: 54.7, ts: 1681102802872, ack: true }, from: 2 }, + { table: 'ts_number', state: { val: 54.7, ts: 1681102502853, ack: true }, from: 3 }, + { table: 'ts_number', state: { val: 54.9, ts: 1681102802872, ack: true }, from: 3 }, + ]; +} + +describe('Test insert() suppresses duplicate (id, ts) rows (#304)', function () { + for (const [dialect, sql] of Object.entries(DIALECTS)) { + const guard = DUPLICATE_GUARD[dialect]; + + it(`${dialect}: ${guard ? 'guards the keyed tables' : 'needs no guard, its index is not unique'}`, function () { + for (const table of ['ts_number', 'ts_string', 'ts_bool']) { + const [query] = sql.insert('iobroker', 4, [ + { table, state: { val: table === 'ts_string' ? 'a' : 1, ts: 100, ack: true }, from: 2 }, + ]); + + if (guard) { + assert.ok(guard.test(query), `${table} is unguarded: ${query}`); + } else { + assert.ok( + !/ON DUPLICATE KEY|ON CONFLICT/i.test(query), + `${table} grew a guard; check whether init() still creates a non-unique index: ${query}`, + ); + } + } + }); + + if (dialect === 'postgresql' || dialect === 'sqlite') { + it(`${dialect}: uses DO NOTHING, not DO UPDATE`, function () { + const [query] = sql.insert('iobroker', 4, duplicateBatch()); + + // The duplicate sits *inside* one statement. DO NOTHING copes with that; turning it + // into DO UPDATE - the obvious way to let the last value win - makes PostgreSQL + // raise "cannot affect row a second time" and brings the bug straight back. + assert.ok(!/DO UPDATE/i.test(query), `DO UPDATE reintroduces #304: ${query}`); + }); + } + } + + it('sqlite: the reported batch really inserts, against a live schema', function (done) { + const sqlite3 = require('sqlite3'); + const db = new sqlite3.Database(':memory:'); + + db.serialize(() => { + // the real DDL from init() + db.run( + 'CREATE TABLE ts_number (id INTEGER, ts BIGINT, val REAL, ack BOOLEAN, _from INTEGER, q INTEGER, PRIMARY KEY(id, ts))', + ); + + const [query] = DIALECTS.sqlite.insert('ignored', 4, duplicateBatch()); + db.run(query, function (err) { + assert.ifError(err); + + db.all('SELECT ts, val FROM ts_number ORDER BY ts', function (err, rows) { + assert.ifError(err); + // four input rows, three distinct timestamps - the duplicate is dropped + assert.strictEqual(rows.length, 3, JSON.stringify(rows)); + assert.strictEqual(rows[2].val, 54.7, 'the first value for a timestamp wins'); + db.close(done); + }); + }); + }); + }); +}); diff --git a/test/testMySQLInsertDuplicates.js b/test/testMySQLInsertDuplicates.js new file mode 100644 index 0000000..4f770e5 --- /dev/null +++ b/test/testMySQLInsertDuplicates.js @@ -0,0 +1,125 @@ +const assert = require('node:assert'); + +// Issue #304 reported a primary key violation on MariaDB/MySQL, so the dialect it was reported on +// gets an executable check rather than a string assertion. This talks to the server directly - no +// js-controller, no adapter instance - so it belongs in the MySQL job's `test/testMySQL*.js` glob +// and costs a second. +const MySQL = require('../build/lib/mysql'); + +const HOST = '127.0.0.1'; +const USER = process.env.SQL_USER || 'root'; +const PASS = process.env.SQL_PASS || 'root'; +// Never the real `iobroker` database: this one is created and dropped by the test itself. +const DB = '__iob_insert_duplicates__'; + +/** The batch from the issue: ts 1681102802872 appears twice for id 4, logged from two sources */ +function duplicateBatch() { + return [ + { table: 'ts_number', state: { val: 54.7, ts: 1681102502871, ack: true }, from: 2 }, + { table: 'ts_number', state: { val: 54.7, ts: 1681102802872, ack: true }, from: 2 }, + { table: 'ts_number', state: { val: 54.7, ts: 1681102502853, ack: true }, from: 3 }, + { table: 'ts_number', state: { val: 54.9, ts: 1681102802872, ack: true }, from: 3 }, + ]; +} + +describe('Test insert() against a live MySQL (#304)', function () { + this.timeout(30000); + + let connection = null; + let unreachable = ''; + + before(async function () { + let mysql; + try { + mysql = require('mysql2/promise'); + } catch (e) { + unreachable = `mysql2 is an optionalDependency and is not installed: ${e.message}`; + return; + } + + try { + connection = await mysql.createConnection({ + host: HOST, + user: USER, + password: PASS, + connectTimeout: 5000, + // left off on purpose: the adapter never sends batches either, and the test would + // stop covering what it is meant to cover + multipleStatements: false, + }); + await connection.query(`DROP DATABASE IF EXISTS \`${DB}\``); + await connection.query(`CREATE DATABASE \`${DB}\``); + // the real DDL from init(), PRIMARY KEY(id, ts) and all + await connection.query( + `CREATE TABLE \`${DB}\`.ts_number (id INTEGER, ts BIGINT, val REAL, ack BOOLEAN, _from INTEGER, q INTEGER, PRIMARY KEY(id, ts));`, + ); + } catch (e) { + unreachable = `${e.code || ''} ${e.message}`.trim(); + connection = null; + } + }); + + after(async function () { + if (connection) { + await connection.query(`DROP DATABASE IF EXISTS \`${DB}\``); + await connection.end(); + } + }); + + beforeEach(function () { + // Skipping rather than failing keeps `npx mocha test/testMySQL*.js` usable on a machine + // without a server. The MySQL CI job always has one, so there it really runs. + if (!connection) { + console.log(`Skipped: no MySQL at ${HOST} (${unreachable})`); + this.skip(); + } + }); + + it('stores a batch that contains the same (id, ts) twice', async function () { + await connection.query(`TRUNCATE \`${DB}\`.ts_number`); + + const [query] = MySQL.insert(DB, 4, duplicateBatch()); + await connection.query(query); // used to throw ER_DUP_ENTRY + + const [rows] = await connection.query(`SELECT ts, val, _from FROM \`${DB}\`.ts_number ORDER BY ts`); + assert.strictEqual(rows.length, 3, `four rows, three distinct timestamps: ${JSON.stringify(rows)}`); + assert.strictEqual(Number(rows[2].ts), 1681102802872); + assert.strictEqual(rows[2].val, 54.7, 'the first value for a timestamp wins, the duplicate is dropped'); + }); + + it('would fail without the guard - this is the bug that was reported', async function () { + await connection.query(`TRUNCATE \`${DB}\`.ts_number`); + + const [query] = MySQL.insert(DB, 4, duplicateBatch()); + const unguarded = query.replace(/ ON DUPLICATE KEY UPDATE id=id/i, ''); + assert.notStrictEqual(unguarded, query, 'precondition: the guard was in the statement'); + + await assert.rejects( + () => connection.query(unguarded), + err => { + // exactly what issue #304 shows: Duplicate entry '-' for key 'PRIMARY' + assert.strictEqual(err.code, 'ER_DUP_ENTRY', err.message); + assert.ok(err.message.includes('4-1681102802872'), err.message); + return true; + }, + ); + }); + + it('still reports a real error instead of swallowing it', async function () { + await connection.query(`TRUNCATE \`${DB}\`.ts_number`); + + // The guard is `ON DUPLICATE KEY UPDATE id=id`, not `INSERT IGNORE`, so that only the + // uniqueness conflict is suppressed - see the comment in src/lib/mysql.ts. An INSERT naming + // a column that does not exist has to keep failing. + await assert.rejects( + () => + connection.query( + `INSERT INTO \`${DB}\`.ts_number (id, ts, nope) VALUES (1, 2, 3) ON DUPLICATE KEY UPDATE id=id;`, + ), + err => { + assert.strictEqual(err.code, 'ER_BAD_FIELD_ERROR', err.message); + return true; + }, + ); + }); +});