feat(ibmi): add database version compatibility check for native boolean + initial unit tests #16909

rcronin · 2023-12-29T05:27:07Z

Pull Request Checklist

Have you added new tests to prevent regressions?
If a documentation update is necessary, have you opened a PR to the documentation repository?
Did you update the typescript typings accordingly (if applicable)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Does the name of your PR follow our conventions?

Description Of Change

Added a check within data-types.js for the IBM i dialect to check the database version. If it's the latest known version, it implements the base boolean support, which handles booleans properly within javascript and back to the database. Also copied the DB2 unit tests as a start for IBM i unit testing.

Todos

…anager

WikiRik · 2023-12-29T19:07:43Z

Don't worry about the failing artifact download actions, that's a known flaky thing in the new version of the action. I can just rerun the checks if needed but you should be good if unit tests pass.

rcronin · 2023-12-29T21:17:54Z

@WikiRik sounds good. This is ready to review

rcronin · 2024-01-16T22:03:12Z

@WikiRik - any update on when this may get reviewed/merged?

WikiRik

Could you add some unit tests that simulate ibmi 7.5.0 (or higher)? You can just do something similar as here;

sequelize/packages/core/test/unit/query-interface/create-table.test.ts

Lines 13 to 66 in 6800fbd

    
           it('supports sql.uuidV4 default values', async () => { 
        
             const localSequelize = dialect.name === 'postgres' 
        
               ? createSequelizeInstance({ 
        
                 databaseVersion: '13.0.0', 
        
               }) 
        
               : sequelize; 
        
             const stub = sinon.stub(localSequelize, 'queryRaw'); 
        
             await localSequelize.queryInterface.createTable('table', { 
        
               id: { 
        
                 type: DataTypes.UUID, 
        
                 primaryKey: true, 
        
                 defaultValue: sql.uuidV4, 
        
               }, 
        
             }); 
        
             expect(stub.callCount).to.eq(1); 
        
             const firstCall = stub.getCall(0); 
        
             expectsql(firstCall.args[0], { 
        
               postgres: 'CREATE TABLE IF NOT EXISTS "table" ("id" UUID DEFAULT gen_random_uuid(), PRIMARY KEY ("id"));', 
        
               'mariadb mysql': 'CREATE TABLE IF NOT EXISTS `table` (`id` CHAR(36) BINARY, PRIMARY KEY (`id`)) ENGINE=InnoDB;', 
        
               mssql: `IF OBJECT_ID(N'[table]', 'U') IS NULL CREATE TABLE [table] ([id] UNIQUEIDENTIFIER DEFAULT NEWID(), PRIMARY KEY ([id]));`, 
        
               sqlite: 'CREATE TABLE IF NOT EXISTS `table` (`id` TEXT PRIMARY KEY);', 
        
               snowflake: 'CREATE TABLE IF NOT EXISTS "table" ("id" VARCHAR(36), PRIMARY KEY ("id"));', 
        
               db2: 'CREATE TABLE IF NOT EXISTS "table" ("id" CHAR(36) FOR BIT DATA NOT NULL, PRIMARY KEY ("id"));', 
        
               ibmi: `BEGIN DECLARE CONTINUE HANDLER FOR SQLSTATE VALUE '42710' BEGIN END; CREATE TABLE "table" ("id" CHAR(36), PRIMARY KEY ("id")); END`, 
        
             }); 
        
           }); 
        
           if (dialect.name === 'postgres') { 
        
             // gen_random_uuid was added in postgres 13 
        
             it('supports sql.uuidV4 default values (postgres < 13)', async () => { 
        
               const localSequelize = createSequelizeInstance({ 
        
                 databaseVersion: '12.0.0', 
        
               }); 
        
               const stub = sinon.stub(localSequelize, 'queryRaw'); 
        
               await localSequelize.queryInterface.createTable('table', { 
        
                 id: { 
        
                   type: DataTypes.UUID, 
        
                   primaryKey: true, 
        
                   defaultValue: sql.uuidV4, 
        
                 }, 
        
               }); 
        
               expect(stub.callCount).to.eq(1); 
        
               const firstCall = stub.getCall(0); 
        
               expectsql(firstCall.args[0], { 
        
                 postgres: 'CREATE TABLE IF NOT EXISTS "table" ("id" UUID DEFAULT uuid_generate_v4(), PRIMARY KEY ("id"));', 
        
               }); 
        
             }); 
        
           }

WikiRik · 2024-01-01T20:56:33Z

packages/core/test/unit/dialects/ibmi/query-generator.test.js

The reason that this file was not added originally is that we're reducing the amount of dialect specific tests and are moving this per function to separate files. But seeing as we're still in the process of doing that I can see some benefit in having this file so I can go either way. @ephys do you have a strong opinion on this?

The functions being tested in this new file already have common test files, so I don't want to approve this file. The new tests should be added to the new files instead, if they're relevant

WikiRik · 2024-01-01T20:57:07Z

packages/core/test/unit/query-generator/bulk-insert-query.test.ts

@@ -20,8 +20,7 @@ describe('QueryGenerator#bulkInsertQuery', () => {
    expectsql(sql, {
      default: `INSERT INTO [Users] ([firstName]) VALUES ('a string');`,
      mssql: `INSERT INTO [Users] ([firstName]) VALUES (N'a string');`,
-      // TODO: ibmi should be the same as `default`, since the 'returning' option is not specified
-      ibmi: `SELECT * FROM FINAL TABLE (INSERT INTO "Users" ("firstName") VALUES ('a string'))`,
+      ibmi: `INSERT INTO "Users" ("firstName") VALUES ('a string')`,


This should work now

Suggested change

ibmi: `INSERT INTO "Users" ("firstName") VALUES ('a string')`,

WikiRik · 2024-01-01T20:57:34Z

packages/core/test/unit/query-interface/bulk-insert.test.ts

@@ -24,7 +24,7 @@ describe('QueryInterface#bulkInsert', () => {

    expectPerDialect(() => firstCall, {
      default: toMatchRegex(/^INSERT INTO (?:`|")Users(?:`|") \((?:`|")firstName(?:`|")\) VALUES (?:\('\w+'\),){999}\('\w+'\);$/),
-      ibmi: toMatchRegex(/^SELECT \* FROM FINAL TABLE \(INSERT INTO "Users" \("firstName"\) VALUES (?:\('\w+'\),){999}\('\w+'\)\)$/),
+      ibmi: toMatchRegex(/^INSERT INTO "Users" \("firstName"\) VALUES (?:\('\w+'\),){999}\('\w+'\)$/),


Suggested change

ibmi: toMatchRegex(/^INSERT INTO "Users" $"firstName"$ VALUES (?:$'\w+'$,){999}$'\w+'$$/),

I believe that the custom expectations in the other changed tests can be removed

WikiRik · 2024-01-31T15:46:51Z

packages/core/src/dialects/ibmi/data-types.ts

+    const databaseVersion
+      = Number.parseFloat(this._getDialect().sequelize.options.databaseVersion || this._getDialect().defaultVersion);
+
+    return databaseVersion >= 7.5;


We can use the gt function from semver to compare this to getDatabaseVersion()

WikiRik · 2024-01-31T15:47:31Z

packages/core/test/config/config.ts

@@ -96,5 +96,6 @@ export const Config: Record<Dialect, Options> = {
    dialectOptions: {
      odbcConnectionString: env.SEQ_IBMI_CONN_STR,
    },
+    databaseVersion: '7.3.0',


Suggested change

databaseVersion: '7.3.0',

ibmi has a defaultVersion of 7.3.0 already

ephys · 2024-02-11T17:09:39Z

packages/core/src/dialects/ibmi/data-types.ts

@@ -8,16 +8,27 @@ export class UUID extends BaseTypes.UUID {
 }

 export class BOOLEAN extends BaseTypes.BOOLEAN {
+  protected supportsNativeBooleans() {


this should be hard private

ephys · 2024-02-11T17:09:48Z

packages/core/src/dialects/ibmi/data-types.ts

@@ -8,16 +8,27 @@ export class UUID extends BaseTypes.UUID {
 }

 export class BOOLEAN extends BaseTypes.BOOLEAN {
+  protected supportsNativeBooleans() {
+    const databaseVersion
+      = Number.parseFloat(this._getDialect().sequelize.options.databaseVersion || this._getDialect().defaultVersion);


Don't get the database version like this, but via sequelize.getDatabaseVersion()

ephys · 2024-02-11T17:11:12Z

packages/core/src/dialects/ibmi/query-generator.js

@@ -356,7 +355,6 @@ export class IBMiQueryGenerator extends IBMiQueryGeneratorTypeScript {
    let query = super.bulkInsertQuery(tableName, fieldValueHashes, options, fieldMappedAttributes);
    if (query.at(-1) === ';') {
      query = query.slice(0, -1);
-      query = `SELECT * FROM FINAL TABLE (${query})`;


why remove the "select from final table"? :o

rcronin · 2024-04-17T18:59:07Z

Finally circling back to this. To implement with the dialect being split out, how should I approach this pull request? Throw out and redo it?

rcronin and others added 17 commits March 24, 2023 08:58

Close IBM i connection after query.

9fedae9

Fix typo

ca126e8

Merge branch 'main' into main

e364224

Undo closing connection on query. Fix connection check in connectionm…

14c38a0

…anager

Merge branch 'main' into main

a551b9f

Allow for reconnect on IPL

14d1629

Merge branch 'main' into main

4ebf2c3

Merge branch 'main' into main

f53e531

Merge branch 'main' into main

14e86a1

Merge branch 'sequelize:main' into main

d821599

Add check in IBM i Boolean data type to conditionally check version

b360e8e

Add IBM i unit tests

dec53c5

Cleanup version checking to be reusable.

520ed09

Add database version to test config for IBM i

fdebe0e

Change to template literal

f2c2370

Enable FinalTable for IBM i + pass unit tests

3f99bcb

Implement parseDatabaseValue to handle odbc boolean

a7c78b9

Merge branch 'main' into main

d33bb0f

Merge branch 'main' into main

05d8407

WikiRik reviewed Jan 31, 2024

View reviewed changes

ephys requested changes Feb 11, 2024

View reviewed changes

rcronin requested a review from a team as a code owner March 18, 2024 23:04

rcronin requested review from ephys and WikiRik and removed request for a team March 18, 2024 23:04

sequelize-bot bot added the conflicted This PR has merge conflicts and will not be present in the list of PRs to review label Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ibmi): add database version compatibility check for native boolean + initial unit tests #16909

feat(ibmi): add database version compatibility check for native boolean + initial unit tests #16909

rcronin commented Dec 29, 2023

WikiRik commented Dec 29, 2023

rcronin commented Dec 29, 2023

rcronin commented Jan 16, 2024

WikiRik left a comment

WikiRik Jan 1, 2024

ephys Feb 11, 2024

WikiRik Jan 1, 2024

WikiRik Jan 1, 2024

WikiRik Jan 31, 2024

WikiRik Jan 31, 2024

WikiRik Jan 31, 2024

ephys Feb 11, 2024

ephys Feb 11, 2024

ephys Feb 11, 2024

rcronin commented Apr 17, 2024

	it('supports sql.uuidV4 default values', async () => {
	const localSequelize = dialect.name === 'postgres'
	? createSequelizeInstance({
	databaseVersion: '13.0.0',
	})
	: sequelize;

	const stub = sinon.stub(localSequelize, 'queryRaw');

	await localSequelize.queryInterface.createTable('table', {
	id: {
	type: DataTypes.UUID,
	primaryKey: true,
	defaultValue: sql.uuidV4,
	},
	});

	expect(stub.callCount).to.eq(1);
	const firstCall = stub.getCall(0);
	expectsql(firstCall.args[0], {
	postgres: 'CREATE TABLE IF NOT EXISTS "table" ("id" UUID DEFAULT gen_random_uuid(), PRIMARY KEY ("id"));',
	'mariadb mysql': 'CREATE TABLE IF NOT EXISTS `table` (`id` CHAR(36) BINARY, PRIMARY KEY (`id`)) ENGINE=InnoDB;',
	mssql: `IF OBJECT_ID(N'[table]', 'U') IS NULL CREATE TABLE [table] ([id] UNIQUEIDENTIFIER DEFAULT NEWID(), PRIMARY KEY ([id]));`,
	sqlite: 'CREATE TABLE IF NOT EXISTS `table` (`id` TEXT PRIMARY KEY);',
	snowflake: 'CREATE TABLE IF NOT EXISTS "table" ("id" VARCHAR(36), PRIMARY KEY ("id"));',
	db2: 'CREATE TABLE IF NOT EXISTS "table" ("id" CHAR(36) FOR BIT DATA NOT NULL, PRIMARY KEY ("id"));',
	ibmi: `BEGIN DECLARE CONTINUE HANDLER FOR SQLSTATE VALUE '42710' BEGIN END; CREATE TABLE "table" ("id" CHAR(36), PRIMARY KEY ("id")); END`,
	});
	});

	if (dialect.name === 'postgres') {
	// gen_random_uuid was added in postgres 13
	it('supports sql.uuidV4 default values (postgres < 13)', async () => {
	const localSequelize = createSequelizeInstance({
	databaseVersion: '12.0.0',
	});

	const stub = sinon.stub(localSequelize, 'queryRaw');

	await localSequelize.queryInterface.createTable('table', {
	id: {
	type: DataTypes.UUID,
	primaryKey: true,
	defaultValue: sql.uuidV4,
	},
	});

	expect(stub.callCount).to.eq(1);
	const firstCall = stub.getCall(0);
	expectsql(firstCall.args[0], {
	postgres: 'CREATE TABLE IF NOT EXISTS "table" ("id" UUID DEFAULT uuid_generate_v4(), PRIMARY KEY ("id"));',
	});
	});
	}

feat(ibmi): add database version compatibility check for native boolean + initial unit tests #16909

Are you sure you want to change the base?

feat(ibmi): add database version compatibility check for native boolean + initial unit tests #16909

Conversation

rcronin commented Dec 29, 2023

Pull Request Checklist

Description Of Change

Todos

WikiRik commented Dec 29, 2023

rcronin commented Dec 29, 2023

rcronin commented Jan 16, 2024

WikiRik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcronin commented Apr 17, 2024