nuodb / node-nuodb Goto Github PK

TLDR: Don brings up a good point of whether the default case of treating data as a string by default makes sense. The only types not specifically handled in the nodejs driver are the binary types, and the blob/clob types. Binary types should definitely be handled as numbers so that the developer can take advantage of javascript's bitwise operators, and clobs should absolutely be strings.

Copied from JIRA:

The implication is we think all types not called out have a logical VARCHAR translation. Here is the enum, but I am not sure what make sense, for example not all types are specifically handled, but some types are numbers not strings.

enum SqlType

{ NUOSQL_NULL = 0, /< Represents SQL value NULL (0) */ NUOSQL_BIT = -7, /< Type code representing the SQL type BIT (-7) */ NUOSQL_TINYINT = -6, /< Type code representing the SQL type BYTE (-6) */ NUOSQL_SMALLINT = 5, /< Type code representing the SQL type SMALLINT (5) */ NUOSQL_INTEGER = 4, /< Type code representing the SQL type INTEGER (4) */ NUOSQL_BIGINT = -5, /< Type code representing the SQL type BIGINT (-5) */ NUOSQL_FLOAT = 6, /< Type code representing the SQL type FLOAT (6) */ NUOSQL_DOUBLE = 8, /< Type code representing the SQL type DOUBLE (8) */ NUOSQL_CHAR = 1, /< Type code representing the SQL type CHAR (1) */ NUOSQL_VARCHAR = 12, /< Type code representing the SQL type VARCHAR (12) */ NUOSQL_LONGVARCHAR = -1, /< Type code representing the SQL type LONGVARCHAR (-1) */ NUOSQL_DATE = 91, /< Type code representing the SQL type DATE (91) */ NUOSQL_TIME = 92, /< Type code representing the SQL type TIME (92) */ NUOSQL_TIMESTAMP = 93, /< Type code representing the SQL type TIMESTAMP (93) */ NUOSQL_BLOB = 2004, /< Type code representing the SQL type BLOB (2004) */ NUOSQL_CLOB = 2005, /< Type code representing the SQL type CLOB (2005) */ NUOSQL_NUMERIC = 2, /< Type code representing the SQL type NUMERIC (2) */ NUOSQL_DECIMAL = 3, /< Type code representing the SQL type DECIMAL (3) */ NUOSQL_BOOLEAN = 16, /< Type code representing the SQL type BOOLEAN (16) */ NUOSQL_BINARY = -2, /< Type code representing the SQL type BINARY (-2) */ NUOSQL_LONGVARBINARY = -4, /**< Type code representing the SQL type LONGVARBINARY (-4) */ NUOSQL_TYPE_UNUSED = 0 }

Error

 7.14 Testing TIME
      1) 7.14 result set stores TIME correctly


  38 passing (4s)
  1 failing

  1) 7. testing types
       7.14 Testing TIME
         7.14 result set stores TIME correctly:

      AssertionError: expected '23:11:00' == '11:11:00 PM'
      + expected - actual

      -23:11:00
      +11:11:00 PM

      at Object.checkResults (test/typeTestCases.js:247:16)
      at Context.<anonymous> (test/7.typeTests.js:56:28)

Environment

server version: 4.3-3-6390229bd4, server license: Enterprise
server time: 2022-03-28T06:07:11.594, client token: 296fb033d779a34880d49e4ecd07b1560a763b30
Servers:
  [p104] p104:48005 [last_ack = 1.20] ACTIVE (LEADER, Leader=p104, log=0/31/31) Connected *
Databases:
  test [state = RUNNING]
    [SM] p104.bo2.nuodb.com/p104:48006 [start_id = 0] [server_id = p104] [pid = 1088866] [node_id = 1] [last_ack =  1.39] MONITORED:RUNNING
    [SM] p104.bo2.nuodb.com/p104:48007 [start_id = 1] [server_id = p104] [pid = 1088892] [node_id = 2] [last_ack =  1.39] MONITORED:RUNNING
    [TE] p104.bo2.nuodb.com/p104:48008 [start_id = 2] [server_id = p104] [pid = 1089029] [node_id = 3] [last_ack =  1.18] MONITORED:RUNNING
    [TE] p104.bo2.nuodb.com/p104:48009 [start_id = 3] [server_id = p104] [pid = 1089060] [node_id = 4] [last_ack =  0.17] MONITORED:RUNNING

The test cases are using the toLocaleTimeString method of a date object. The 'valid' test case is using the time string expected in the US, so testing outside of the US is not guaranteed to have the result in the expected format. This should be solvable by instead comparing on the result of the toISOString method.

Multiplexer should be an addon on top of the connection pool / driver to make it easier for the developer to manage multiple pools for a sharded database environment. Design documentation exists on our internal confluence pages.

Log tracing reveals errors like the following trying to insert invalid Date/Time into the database....
INSERT INTO CVN_CONVERSATIONS (partition_id, conversation_id, creator, tenant_id, title, private, creation_date, modification_date, type) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?); with values: [2,10002,2,1,"some title",false,"2023-09-16T07:34:55.430Z","2023-09-16T07:34:55.430Z",1]
ð ko: error converting 2023-09-16 07:34:55.430tle to Date/Time

We should develop an example of a code pattern for application transaction retries.

We should test for:

• connection loss
• deadlock
• update conflict

We may be able to trap these errors and throw them as custom error types to make error handling easier at the application level, but this will require some investigation.

Fix this as it's a mish-mash of N-API and Add-On C++ API conventions. And we ought to put the whole work into its own C++ namespace.

Failure

  13. Test Result Set
    1) "before all" hook: open connection, init tables for "13.1 Can get results in chunks"


  8 passing (10s)
  1 failing

  1) 13. Test Result Set
       "before all" hook: open connection, init tables for "13.1 Can get results in chunks":
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/bkelly/testspace/drivertesting/node-nuodb/test/13.resultSet.js)
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7)

Environment

server version: 4.3-3-6390229bd4, server license: Enterprise
server time: 2022-03-28T06:07:11.594, client token: 296fb033d779a34880d49e4ecd07b1560a763b30
Servers:
  [p104] p104:48005 [last_ack = 1.20] ACTIVE (LEADER, Leader=p104, log=0/31/31) Connected *
Databases:
  test [state = RUNNING]
    [SM] p104.bo2.nuodb.com/p104:48006 [start_id = 0] [server_id = p104] [pid = 1088866] [node_id = 1] [last_ack =  1.39] MONITORED:RUNNING
    [SM] p104.bo2.nuodb.com/p104:48007 [start_id = 1] [server_id = p104] [pid = 1088892] [node_id = 2] [last_ack =  1.39] MONITORED:RUNNING
    [TE] p104.bo2.nuodb.com/p104:48008 [start_id = 2] [server_id = p104] [pid = 1089029] [node_id = 3] [last_ack =  1.18] MONITORED:RUNNING
    [TE] p104.bo2.nuodb.com/p104:48009 [start_id = 3] [server_id = p104] [pid = 1089060] [node_id = 4] [last_ack =  0.17] MONITORED:RUNNING

The result set test is failing here in QA. I believe the root cause here is that the QA machine running the test is slower than the machine that the test was developed on. Should be able to resolve this by increasing the timeout for this specific test.

The C++ code here doesn't follow the NuoDB C++ coding standards for formatting, naming, and layout. Is this important? I tend to think so, but maybe we should discuss.

When using the connection pool, when the hard limit is reached an error is generated. Another potential option for this would be implementing a connection request queue. When there are no connections to be distributed, the request would be added to a queue, and a promise for a connection returned. As connections are released back into the pool they would be given to requests in the queue in FIFO order.

When returning rows without closing the result set that the rows came from, a crash occurs.

mux.requestConnection().then(conn) => { return conn.execute(query).then((results) => { const rows = results?.getRows().then(r) => { return results?.close().then(() => r); // without this line a crash occurs }) return rows }) })

I wanted strings externalized, akin to ICU, but w/o using ICU. But in externalizing vsnprintf we lost param checking. Is there a way to retain externalizing strings, but also retain checking by the compiler @madscientist ? I know ICU does it, at least AFAIK it does, having used it at two companies thus far.

Per @madscientist , add license terms to each file. But...

Lets defer to a follow on train. What copyright depends upon the license terms attached to the work. I think this will end up being MIT or Apache 2.0, but lets wait until a conversation is had with PM.

Currently the driver appears to be using a version "3.3". I don't think this is a good idea: we have learned to our great sorrow that versioning your drivers the same way as your server is a very big mistake.

The driver needs to have its own versioning scheme which is completely separate from the server, and updated only when the driver itself is updated. We have discussed using a SemVer numbering scheme which seems like what everyone uses these days for these sorts of things.

The question is what should the initial version be. The last version released for the previous Node.js driver was 1.0.2. We can either say this is a completely different driver and start it over at 1.0.0, or we can say this is a new, incompatible version of the driver and start it at 2.0.0.