Name the datapoint in the counter type error (#320)

README.md

@@ -619,6 +619,9 @@ sendTo('sql.0', 'getEnabledDPs', {}, function (result) {
 -->
 
 ## Changelog
+### **WORK IN PROGRESS**
+* (@GermanBluefox) The `Counter must have type "number"` error now names the datapoint and is logged once instead of for every value (#320)
+
 ### **WORK IN PROGRESS**
 * (@DutchmanNL) PostgreSQL: "do not create database" now connects straight to the configured database instead of opening the maintenance database `postgres` first, so roles without `CONNECT` on it can be used
 * (@GermanBluefox) MySQL can now connect through a unix socket instead of host and port (#104)

src/lib/errors.ts

@@ -131,3 +131,22 @@ function describeNested(err: unknown, depth: number): string {
     }
     return formatError(err, depth);
 }
+
+/**
+ * The message for a datapoint that has the counter option on but is not stored as a number.
+ *
+ * Built here rather than inline so it can be unit tested, and because the point of
+ * https://github.com/ioBroker/ioBroker.sql/issues/320 is the wording: the old text was
+ * `Counter must have type "number"!` with no indication of *which* datapoint, which left people
+ * searching their configuration for a year. The ID and the actual type are what make it
+ * actionable, and the sentence says what to change.
+ *
+ * @param id the state ID of the misconfigured datapoint
+ * @param storageType how the datapoint is stored, e.g. 'String' - may be a raw number if unmapped
+ */
+export function counterTypeMismatch(id: string, storageType: string | number | undefined): string {
+    return (
+        `Counter must have type "number", but "${id}" is stored as "${storageType ?? 'unknown'}". ` +
+        `Change the storage type of this datapoint to "Number", or switch the counter option off.`
+    );
+}

src/main.ts

@@ -27,7 +27,7 @@ import { SQLite3ClientPool, SQLite3Client, type SQLite3Options } from './lib/sql
 import type { SQLClientPool, PoolConfig } from './lib/sql-client-pool';
 import type SQLClient from './lib/sql-client';
 import type { IobDataEntry } from './lib/types';
-import { formatError } from './lib/errors';
+import { counterTypeMismatch, formatError } from './lib/errors';
 
 export interface IobDataEntryEx extends Omit<IobDataEntry, 'val'> {
     val: string | boolean | number | null;
@@ -252,6 +252,8 @@ type SQLPointConfig = {
     list: { state: IobDataEntryEx; from: number; table: TableName }[];
     inFlight: { [inFlightId: string]: { state: IobDataEntryEx; from: number; table: TableName }[] };
     isRunning?: { id: string; state: IobDataEntryEx; isCounter: boolean; cb?: (err?: Error | null) => void }[];
+    /** set once the counter/type mismatch has been reported, so it is not repeated per value */
+    counterTypeReported?: boolean;
 };
 
 function sortByTs(
@@ -1724,7 +1726,16 @@ export class SqlAdapter extends Adapter {
 
             if (settings.counter && this.sqlDPs[id].state) {
                 if (this.sqlDPs[id].type !== types.number) {
-                    this.log.error('Counter must have type "number"!');
+                    // Without the ID this is unsearchable: the message fires for every value of
+                    // the misconfigured datapoint and says nothing about which one it is. Reported
+                    // once per datapoint rather than once per value, because the repetition was
+                    // half of the problem. See https://github.com/ioBroker/ioBroker.sql/issues/320
+                    if (!this.sqlDPs[id].counterTypeReported) {
+                        this.sqlDPs[id].counterTypeReported = true;
+                        this.log.error(
+                            counterTypeMismatch(id, storageTypes[this.sqlDPs[id].type] ?? this.sqlDPs[id].type),
+                        );
+                    }
                 } else if (
                     state.val === null ||
                     this.sqlDPs[id].state.val === null ||
@@ -2386,7 +2397,7 @@ export class SqlAdapter extends Adapter {
 
         // Check SQL connection
         if (!this.clientPool) {
-            this.log.warn('No Connection to database');
+            this.log.warn(`No connection to the database, cannot store the value of "${id}"`);
             if (cb) {
                 setImmediate(() => cb(new Error('No Connection to database')));
             }
@@ -3514,7 +3525,7 @@ export class SqlAdapter extends Adapter {
      */
     #readIdIndexAndType(id: string, cb: (err: Error | null, index?: number, type?: 0 | 1 | 2) => void): void {
         if (!this.clientPool) {
-            this.log.warn('No Connection to database');
+            this.log.warn(`No connection to the database, cannot look up "${id}"`);
             setImmediate(() => cb(new Error('No Connection to database')));
             return;
         }

test/testErrors.js

@@ -1,5 +1,5 @@
 const assert = require('node:assert');
-const { formatError } = require('../build/lib/errors');
+const { formatError, counterTypeMismatch } = require('../build/lib/errors');
 
 describe('Test formatError', function () {
     it('unwraps an AggregateError', function () {
@@ -69,3 +69,23 @@ describe('Test formatError', function () {
         assert.ok(formatError(circular).length > 0);
     });
 });
+
+describe('Test counterTypeMismatch', function () {
+    it('names the datapoint, which the old message did not', function () {
+        const text = counterTypeMismatch('modbus.0.holdingRegisters.1234_Zaehler', 'String');
+
+        // the whole point of issue #320: the old text was 'Counter must have type "number"!'
+        // and left people searching their configuration for which datapoint it meant
+        assert.ok(text.includes('modbus.0.holdingRegisters.1234_Zaehler'), text);
+        assert.ok(text.includes('String'), 'says how it is stored now');
+        assert.ok(text.includes('Number'), 'says what it has to be');
+        assert.ok(text.includes('counter option'), 'offers the other way out');
+        assert.ok(!text.includes('\n'), 'must stay on one line');
+    });
+
+    it('survives an unmapped storage type', function () {
+        // storageTypes[] has no entry for every value the datapoints table could hold
+        assert.ok(counterTypeMismatch('sql.0.x', 7).includes('"7"'));
+        assert.ok(counterTypeMismatch('sql.0.x', undefined).includes('unknown'));
+    });
+});