Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(oracle): add oracle dialect support #14638

Merged
merged 20 commits into from Sep 15, 2022
Merged

feat(oracle): add oracle dialect support #14638

merged 20 commits into from Sep 15, 2022

Conversation

sudarshan12s
Copy link

@sudarshan12s sudarshan12s commented Jun 14, 2022

  • feat(oracle): add oracle dialect support

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • 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?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

Adding Oracle dialect support to Sequelize. Integration and Unit tests have been run on the new dialect. A yaml file for automating tests is included in the PR. The Github workflow up to the point of Oracle software installation has been verified.

We acknowledge and thank contributors to previous Oracle support efforts. Significant improvements have been made over these PRs:

Sequelize documentation changes are done in a separate PR at sequelize/website#187

This PR is for the V6 branch, which is in wide use. We will work on V7 once this V6 MR is OK'd by the community.

Minimum Supported version of dependency

  1. Node-oracledb 5.4.0
  2. Oracle Database 18.4

Todos

* feat(oracle): add oracle dialect support
ephys
ephys previously requested changes Jun 19, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall really good work.
I have many notes due to the sheer size of the PR, but this is impressive :)

strategy:
fail-fast: false
matrix:
node-version: [10, 16]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can bump it to 18 now, we still need to update the v6 ci :)

Suggested change
node-version: [10, 16]
node-version: [10, 18]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ephys should I update the CI to be similar to main where we have the separate steps?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have updated the node version to 18 for the Oracle dialect

Comment on lines 66 to 67
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also something we have neglected to update 😅

Suggested change
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
- uses: actions/checkout@v3
- uses: actions/setup-node@v3

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

- name: Unit Tests
run: yarn test-unit
- name: Integration Tests
run: yarn test-integration-oracle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the current oldest version that is not EOL? Can you add a test against that one too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Sequelize, we are planning to support DB version 18.4 and above. currently, we have test (start.sh) for 21.3 DB version and folder is named as latest to fetch latest version always. But as you mentioned to use fixed versions, I shall rename this to 21c. You wanted to add test case for 18.4 too ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dev/oracle/21-slim/ folder is added to indicate 21c DB version is used as a Oracle docker image.

services:
oraclexedb:
container_name: oraclexedb
image: gvenzl/oracle-xe:latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use :latest anymore (though the v6 branch is a bit outdated), we'll make renovate watch this file so it can open a PR when a new major is released :)

Suggested change
image: gvenzl/oracle-xe:latest
image: gvenzl/oracle-xe:21.3.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have to tell renovate to also check the v6 branch though, but that's something we as maintainers will do.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# Granting all the needed privileges to sequelizetest user
docker exec -t oraclexedb sqlplus system/password@XEPDB1 @privileges.sql

# Setting up Oracle instant client for oracledb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed to run our tests? :o

Is ~/oracle the recommended folder? I would prefer to not add any file outside of the sequelize directory. Can we move it to a gitignored folder called .oracle at the project root?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is needed, is there also a version for Windows that should be added here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we need to install the Instant client for oracle dialect to work. The privileges are explicitly provided, so that the tests dont just run under higher privileges (like SYSDBA) but under the required privileges. I have moved the installation to Project root/ .oracle folder. I added the .oracle to the .gitignore list. Windows support is added .

sqlite: 'CHAR BINARY(12)',
postgres: 'BYTEA'
});

testsql('CHAR.BINARY', DataTypes.CHAR.BINARY, {
default: 'CHAR(255) BINARY',
oracle: 'RAW(255)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is RAW a fixed-length type? If not, this should log a warning (something postgres doesn't do but that is on the "to fix" backlog)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a warning 'Oracle CHAR.BINARY datatype is not of Fixed Length.'

@@ -91,6 +94,7 @@ describe(Support.getTestDialectTeaser('SQL'), () => {
mysql: ['2015-01-20 01:00:00'],
snowflake: ['2015-01-20 01:00:00'],
mariadb: ['2015-01-20 01:00:00.000'],
oracle: [new Date(Date.UTC(2015, 0, 20))],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this equivalent?

Suggested change
oracle: [new Date(Date.UTC(2015, 0, 20))],
oracle: [new Date(2015, 0, 20)],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time can vary based on timezone so we are considering the UTC convention

default: `SELECT [user].* FROM (${
[
`SELECT * FROM (SELECT [user].[id_user] AS [id], [user].[last_name] AS [subquery_order_0], [project_users].[user_id] AS [project_users.userId], [project_users].[project_id] AS [project_users.projectId] FROM [users] AS [user] INNER JOIN [project_users] AS [project_users] ON [user].[id_user] = [project_users].[user_id] AND [project_users].[project_id] = 1 AND [project_users].[status] = 1 ORDER BY [subquery_order_0] ASC${ current.dialect.name === 'mssql' ? ', [user].[id_user]' : ''}${sql.addLimitAndOffset({ limit: 3, order: ['last_name', 'ASC'] })}) AS sub`,
`SELECT * FROM (SELECT [user].[id_user] AS [id], [user].[last_name] AS [subquery_order_0], [project_users].[user_id] AS [project_users.userId], [project_users].[project_id] AS [project_users.projectId] FROM [users] AS [user] INNER JOIN [project_users] AS [project_users] ON [user].[id_user] = [project_users].[user_id] AND [project_users].[project_id] = 5 AND [project_users].[status] = 1 ORDER BY [subquery_order_0] ASC${ current.dialect.name === 'mssql' ? ', [user].[id_user]' : ''}${sql.addLimitAndOffset({ limit: 3, order: ['last_name', 'ASC'] })}) AS sub`
].join(current.dialect.supports['UNION ALL'] ? ' UNION ALL ' : ' UNION ')
}) AS [user] ORDER BY [subquery_order_0] ASC;`
}) AS [user] ORDER BY [subquery_order_0] ASC;`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}) AS [user] ORDER BY [subquery_order_0] ASC;`
}) AS [user] ORDER BY [subquery_order_0] ASC;`

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

default: 'SELECT [User].[name], [User].[age], [Posts].[id] AS [Posts.id], [Posts].[* FROM User; DELETE FROM User;SELECT id] AS [Posts.* FROM User; DELETE FROM User;SELECT id] FROM [User] AS [User] LEFT OUTER JOIN [Post] AS [Posts] ON [User].[id] = [Posts].[user_id];'
});
oracle: 'SELECT "User"."name", "User"."age", "Posts"."id" AS "Posts.id", "Posts"."* FROM User; DELETE FROM User;SELECT id" AS "Posts.* FROM User; DELETE FROM User;SELECT id" FROM "User" "User" LEFT OUTER JOIN "Post" "Posts" ON "User"."id" = "Posts"."user_id";',
default: 'SELECT [User].[name], [User].[age], [Posts].[id] AS [Posts.id], [Posts].[* FROM User; DELETE FROM User;SELECT id] AS [Posts.* FROM User; DELETE FROM User;SELECT id] FROM [User] AS [User] LEFT OUTER JOIN [Post] AS [Posts] ON [User].[id] = [Posts].[user_id];' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most tests have default first, then dialect-specific versions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -495,6 +500,7 @@ describe(Support.getTestDialectTeaser('SQL'), () => {
[Op.between]: ['2013-01-01', '2013-01-11']
}, {
default: "[date] BETWEEN '2013-01-01' AND '2013-01-11'",
oracle: '"date" BETWEEN \'2013-01-01\' AND \'2013-01-11\'',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the same as default

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes used default.

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't fully checked out the query-generator yet, but I agree with @ephys that it's a high quality PR. Looking forward to seeing the changes implemented and then moving over to a PR for the main branch

services:
oraclexedb:
container_name: oraclexedb
image: gvenzl/oracle-xe:latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have to tell renovate to also check the v6 branch though, but that's something we as maintainers will do.

strategy:
fail-fast: false
matrix:
node-version: [10, 16]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ephys should I update the CI to be similar to main where we have the separate steps?

- name: Unit Tests
run: yarn test-unit
- name: Integration Tests
run: yarn test-integration-oracle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
run: yarn test-integration-oracle
run: yarn test-integration

I think that if we specify LD_LIBRARY_PATH in the env variables above this should also work, just for consistency

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I have set the LD_LIBRARY_PATH and it is getting populated in test-integration

# Granting all the needed privileges to sequelizetest user
docker exec -t oraclexedb sqlplus system/password@XEPDB1 @privileges.sql

# Setting up Oracle instant client for oracledb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is needed, is there also a version for Windows that should be added here?

package.json Show resolved Hide resolved
default: 'SELECT [User].[name], [User].[age], [Posts].[id] AS [Posts.id], [Posts].[* FROM User; DELETE FROM User;SELECT id] AS [Posts.* FROM User; DELETE FROM User;SELECT id] FROM [User] AS [User] LEFT OUTER JOIN [Post] AS [Posts] ON [User].[id] = [Posts].[user_id];'
});
oracle: 'SELECT "User"."name", "User"."age", "Posts"."id" AS "Posts.id", "Posts"."* FROM User; DELETE FROM User;SELECT id" AS "Posts.* FROM User; DELETE FROM User;SELECT id" FROM "User" "User" LEFT OUTER JOIN "Post" "Posts" ON "User"."id" = "Posts"."user_id";',
default: 'SELECT [User].[name], [User].[age], [Posts].[id] AS [Posts.id], [Posts].[* FROM User; DELETE FROM User;SELECT id] AS [Posts.* FROM User; DELETE FROM User;SELECT id] FROM [User] AS [User] LEFT OUTER JOIN [Post] AS [Posts] ON [User].[id] = [Posts].[user_id];' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
default: 'SELECT [User].[name], [User].[age], [Posts].[id] AS [Posts.id], [Posts].[* FROM User; DELETE FROM User;SELECT id] AS [Posts.* FROM User; DELETE FROM User;SELECT id] FROM [User] AS [User] LEFT OUTER JOIN [Post] AS [Posts] ON [User].[id] = [Posts].[user_id];' });
default: 'SELECT [User].[name], [User].[age], [Posts].[id] AS [Posts.id], [Posts].[* FROM User; DELETE FROM User;SELECT id] AS [Posts.* FROM User; DELETE FROM User;SELECT id] FROM [User] AS [User] LEFT OUTER JOIN [Post] AS [Posts] ON [User].[id] = [Posts].[user_id];'});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 121 to 126
// const dialectOptions = config.dialectOptions;

// //If stmtCacheSize is defined, we set it
// if (dialectOptions && 'stmtCacheSize' in dialectOptions) {
// connectionConfig.stmtCacheSize = dialectOptions.stmtCacheSize;
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// const dialectOptions = config.dialectOptions;
// //If stmtCacheSize is defined, we set it
// if (dialectOptions && 'stmtCacheSize' in dialectOptions) {
// connectionConfig.stmtCacheSize = dialectOptions.stmtCacheSize;
// }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

throw new SequelizeErrors.HostNotReachableError(err); //ORA-12154: TNS:could not resolve the connect identifier specified
case 'ORA-12514': // ORA-12514: TNS:listener does not currently know of service requested in connect descriptor
throw new SequelizeErrors.HostNotFoundError(err);
// case 'ORA-12541': // ORA-12541: TNS:No listener
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have addressed it.

package.json Outdated
@@ -261,13 +267,15 @@
"test-integration-mssql": "cross-env DIALECT=mssql npm run test-integration",
"test-integration-db2": "cross-env DIALECT=db2 npm run test-integration",
"test-integration-snowflake": "cross-env DIALECT=snowflake npm run test-integration",
"test-integration-oracle": "cross-env LD_LIBRARY_PATH=$HOME/oracle/instantclient/ DIALECT=oracle UV_THREADPOOL_SIZE=128 npm run test-integration",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using GitHub Codespaces, running Node 16 and starting Oracle seems to work fine but I can't run integration tests.

 SequelizeConnectionError: DPI-1047: Cannot locate a 64-bit Oracle Client library: "libaio.so.1: cannot open shared object file: No such file or directory". See https://oracle.github.io/node-oracledb/INSTALL.html for help
Node-oracledb installation instructions: https://oracle.github.io/node-oracledb/INSTALL.html
You must have 64-bit Oracle Client libraries configured with ldconfig, or in LD_LIBRARY_PATH.
If you do not have Oracle Database on this computer, then install the Instant Client Basic or Basic Light package from 
https://www.oracle.com/database/technologies/instant-client/linux-x86-64-downloads.html

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The libaio package needs to be added manually and is not part of the instant client.
It looks like its not installed in VM/container?
The instructions details are here :
https://oracle.github.io/node-oracledb/INSTALL.html (grep for libaio )


const debug = logger.debugContext('sql:oracle');

class Query extends AbstractQuery {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially at @ephys do we want to name this OracleQuery?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed it but yes :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used OracleQuery

src/dialects/oracle/query-generator.js Outdated Show resolved Hide resolved
src/dialects/oracle/connection-manager.js Outdated Show resolved Hide resolved
src/dialects/oracle/data-types.js Outdated Show resolved Hide resolved
src/dialects/oracle/data-types.js Outdated Show resolved Hide resolved
src/dialects/oracle/query-generator.js Show resolved Hide resolved
src/dialects/oracle/query-generator.js Show resolved Hide resolved
src/dialects/oracle/query-generator.js Outdated Show resolved Hide resolved
@WikiRik WikiRik mentioned this pull request Jul 24, 2022
8 tasks
@WikiRik
Copy link
Member

WikiRik commented Aug 8, 2022

@sudarshan12s sorry for the long wait, but I hope to be able to take another look at this later this week

@Abdel-Monaam-Aouini
Copy link

@sudarshan12s sorry for the long wait, but I hope to be able to take another look at this later this week

nice :D

@cjbj
Copy link

cjbj commented Aug 22, 2022

@Monaam12 @WikiRik another ping to make sure we can land this PR soon.

@14gasher
Copy link

Any updates on this perchance?

package.json Show resolved Hide resolved
@@ -239,17 +244,20 @@
"start-postgres": "bash dev/postgres/10/start.sh",
"start-mssql": "bash dev/mssql/2019/start.sh",
"start-db2": "bash dev/db2/11.5/start.sh",
"start-oracle": "bash dev/oracle/21-slim/start.sh",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to work on Apple Silicone?

host logs

[v6][~/Projects/sequelize]$ yarn start-oracle
yarn run v1.22.15
$ bash dev/oracle/21-slim/start.sh
++ dirname -- dev/oracle/21-slim/start.sh
+ cd -P -- dev/oracle/21-slim
+ docker-compose -p oraclexedb down --remove-orphans
Removing oraclexedb ... done
Removing network sequelize-oraclexedb-network
+ docker-compose -p oraclexedb up -d
Creating network "sequelize-oraclexedb-network" with the default driver
Creating oraclexedb ... done
+ ./wait-until-healthy.sh oraclexedb
oraclexedb is healthy!
+ docker cp privileges.sql oraclexedb:/opt/oracle/.
+ docker exec -t oraclexedb sqlplus system/password@XEPDB1 @privileges.sql
Error response from daemon: Container 76313f78c153efd601426ac8534d2ac45d2666fc200b0c2a78b0e212e7fc1111 is not running
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

container logs

The command completed successfully
ERROR:
ORA-12547: TNS:lost contact


SP2-0306: Invalid option.
Usage: CONN[ECT] [{logon|/|proxy} [AS {SYSDBA|SYSOPER|SYSASM|SYSBACKUP|SYSDG|SYSKM|SYSRAC}] [edition=value]]
where <logon> ::= <username>[/<password>][@<connect_identifier>]
      <proxy> ::= <proxyuser>[<username>][/<password>][@<connect_identifier>]
SP2-0306: Invalid option.
Usage: CONN[ECT] [{logon|/|proxy} [AS {SYSDBA|SYSOPER|SYSASM|SYSBACKUP|SYSDG|SYSKM|SYSRAC}] [edition=value]]
where <logon> ::= <username>[/<password>][@<connect_identifier>]
      <proxy> ::= <proxyuser>[<username>][/<password>][@<connect_identifier>]
SP2-0157: unable to CONNECT to ORACLE after 3 attempts, exiting SQL*Plus

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would only work on Apple Silicon under Rosetta because the Oracle Instant Client is not available on Apple Silicon due to some 3rd party libraries not being available.

query = `${replacements.attributes.length ? valueQuery : emptyQuery};`;
if (this._dialect.supports.returnIntoValues && options.returning) {
// Populating the returnAttributes array and performing operations needed for output binds of insertQuery
this.getInsertQueryReturnIntoBinds(returnAttributes, bind.length, returningModelAttributes, returnTypes, options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. What is this doing? As far as I can tell, we never override getInsertQueryReturnIntoBinds anywhere and the defined abstract function is a noop?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the Oracle dialect we generate sql statement like these - INSERT INTO "Campuses" ("id","name","address","description","createdAt","updatedAt") VALUES (DEFAULT,:1,:2,:3,:4,:5) RETURNING "id","name","address","description","createdAt","updatedAt" INTO :6,:7,:8,:9,:10,:11;

getInsertQueryReturnIntoBinds function generates the bind values for outBinds (:6,:7,:8,:9,:10,:11 in the above sql) , generates the bindDefs for the out binds and also populates the options.outBindAttributes field.

Yes, this function is only used by the Oracle dialect, for now, we have defined an abstract function with a noop so that other dialects can override it if required.

src/dialects/abstract/query-generator.js Show resolved Hide resolved
dev/oracle/21-slim/wait-until-healthy.sh Show resolved Hide resolved
dev/oracle/21-slim/wait-until-healthy.sh Outdated Show resolved Hide resolved
src/dialects/abstract/query-generator.js Show resolved Hide resolved
src/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
src/dialects/oracle/connection-manager.js Show resolved Hide resolved
GEOMETRY: false
});

OracleDialect.prototype.defaultVersion = '18.4.0';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this implementation supports all oracle versions starting from 18.4? Do we test against that version on CI?

src/model.js Outdated
@@ -2645,7 +2645,7 @@ class Model {
}
}

if (options.ignoreDuplicates && ['mssql', 'db2'].includes(dialect)) {
if (options.ignoreDuplicates && ['mssql', 'db2', 'oracle'].includes(dialect)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have access to the dialect.supports.ignoreDuplicates here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes its accessable. I see it can be added like this by checking if these properties are left default.

if (options.ignoreDuplicates && this.sequelize.dialect.supports.inserts.ignoreDuplicates === '' &&
       this.sequelize.dialect.supports.inserts.onConflictDoNothing === '') {

@@ -1417,7 +1417,7 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), () => {

describe('hasAssociations with binary key', () => {
beforeEach(function() {
const keyDataType = ['mysql', 'mariadb', 'db2'].includes(dialect) ? 'BINARY(255)' : DataTypes.BLOB('tiny');
const keyDataType = ['mysql', 'mariadb', 'db2'].includes(dialect) ? 'BINARY(255)' : dialect === 'oracle' ? DataTypes.STRING(255, true) : DataTypes.BLOB('tiny');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refactor this to avoid the nested ternary operator.

@@ -13,7 +14,7 @@ describe('QueryInterface#bulkInsert', () => {
});

// you'll find more replacement tests in query-generator tests
it('does not parse replacements outside of raw sql', async () => {
(dialect !== 'oracle' ? it : it.skip)('does not parse replacements outside of raw sql', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you going to work on this?

* fix: incorporated review comments
src/model.js Outdated
@@ -2644,8 +2644,8 @@ class Model {
options.returning = true;
}
}

if (options.ignoreDuplicates && ['mssql', 'db2', 'oracle'].includes(dialect)) {
if (options.ignoreDuplicates && this.sequelize.dialect.supports.inserts.ignoreDuplicates === '' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy mother of Jesus, I didn't expect that thing to be a string. Since empty strings are false, I wonder if we should go with

options.ignoreDuplicates && !this.sequelize.dialect.supports.inserts.ignoreDuplicates

Why do we consider onConflictDoNothing suddenly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without adding onConflictDoNothing, I see this test case for postgresql was failing.

test/integration/model/bulk-create.test.js
 1) should support the ignoreDuplicates option

The test case enables ignoreDuplicates option under a check if either of property; ignoreDuplicates or onConflictDoNothing is non-null. Hence validating against only supports.inserts.ignoreDuplicates throws an error.

Error: postgres does not support the ignoreDuplicates option.

I modified to use ! for string empty check.
on the CI using latest version, can we add the lower version 18.4 in a separate PR?

@sdepold
Copy link
Member

sdepold commented Sep 14, 2022

I think the code is in a decent shape now. There is this one open question about onConflictDoNothing but other than that I'm fine. Since I cannot find my own comment on this anymore, I think it would make sense to test for the latest and the lowest supported version on CI

@sudarshan12s
Copy link
Author

I updated my understanding onConflictDoNothing property.
Can we add the lower version 18.4 in a separate PR?

@WikiRik
Copy link
Member

WikiRik commented Sep 15, 2022

I'm fine with adding 18.4 CI testing in a separate PR. I think that with these latest change we should be good to go. @sdepold agree?

@sdepold
Copy link
Member

sdepold commented Sep 15, 2022

Let's do it!

@sdepold sdepold merged commit c230d80 into sequelize:v6 Sep 15, 2022
@sdepold
Copy link
Member

sdepold commented Sep 15, 2022

Huge kudos to you and your team for the great contribution :)

@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.22.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Abdel-Monaam-Aouini
Copy link

@sdepold sequelize/website#210 related to this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants