-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
* feat(oracle): add oracle dialect support
There was a problem hiding this 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 :)
.github/workflows/ci.yml
Outdated
strategy: | ||
fail-fast: false | ||
matrix: | ||
node-version: [10, 16] |
There was a problem hiding this comment.
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 :)
node-version: [10, 16] | |
node-version: [10, 18] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.github/workflows/ci.yml
Outdated
- uses: actions/checkout@v2 | ||
- uses: actions/setup-node@v1 |
There was a problem hiding this comment.
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 😅
- uses: actions/checkout@v2 | |
- uses: actions/setup-node@v1 | |
- uses: actions/checkout@v3 | |
- uses: actions/setup-node@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.github/workflows/ci.yml
Outdated
- name: Unit Tests | ||
run: yarn test-unit | ||
- name: Integration Tests | ||
run: yarn test-integration-oracle |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
dev/oracle/latest/docker-compose.yml
Outdated
services: | ||
oraclexedb: | ||
container_name: oraclexedb | ||
image: gvenzl/oracle-xe:latest |
There was a problem hiding this comment.
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 :)
image: gvenzl/oracle-xe:latest | |
image: gvenzl/oracle-xe:21.3.0 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
dev/oracle/latest/start.sh
Outdated
# 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)', |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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))], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this equivalent?
oracle: [new Date(Date.UTC(2015, 0, 20))], | |
oracle: [new Date(2015, 0, 20)], |
There was a problem hiding this comment.
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
test/unit/sql/select.test.js
Outdated
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;` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}) AS [user] ORDER BY [subquery_order_0] ASC;` | |
}) AS [user] ORDER BY [subquery_order_0] ASC;` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
test/unit/sql/select.test.js
Outdated
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];' }); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
test/unit/sql/where.test.js
Outdated
@@ -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\'', |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes used default.
There was a problem hiding this 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
dev/oracle/latest/docker-compose.yml
Outdated
services: | ||
oraclexedb: | ||
container_name: oraclexedb | ||
image: gvenzl/oracle-xe:latest |
There was a problem hiding this comment.
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.
.github/workflows/ci.yml
Outdated
strategy: | ||
fail-fast: false | ||
matrix: | ||
node-version: [10, 16] |
There was a problem hiding this comment.
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?
.github/workflows/ci.yml
Outdated
- name: Unit Tests | ||
run: yarn test-unit | ||
- name: Integration Tests | ||
run: yarn test-integration-oracle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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
dev/oracle/latest/start.sh
Outdated
# 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 |
There was a problem hiding this comment.
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?
test/unit/sql/select.test.js
Outdated
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];' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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];'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// const dialectOptions = config.dialectOptions; | ||
|
||
// //If stmtCacheSize is defined, we set it | ||
// if (dialectOptions && 'stmtCacheSize' in dialectOptions) { | ||
// connectionConfig.stmtCacheSize = dialectOptions.stmtCacheSize; | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// const dialectOptions = config.dialectOptions; | |
// //If stmtCacheSize is defined, we set it | |
// if (dialectOptions && 'stmtCacheSize' in dialectOptions) { | |
// connectionConfig.stmtCacheSize = dialectOptions.stmtCacheSize; | |
// } |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 )
src/dialects/oracle/query.js
Outdated
|
||
const debug = logger.debugContext('sql:oracle'); | ||
|
||
class Query extends AbstractQuery { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used OracleQuery
* fix: addressing review comments
* fix: minor fixes to the review comments
* fix: doc gen is fixed
* fix: autogenerate the primary constraint name
@sudarshan12s sorry for the long wait, but I hope to be able to take another look at this later this week |
nice :D |
@Monaam12 @WikiRik another ping to make sure we can land this PR soon. |
Any updates on this perchance? |
@@ -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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
GEOMETRY: false | ||
}); | ||
|
||
OracleDialect.prototype.defaultVersion = '18.4.0'; |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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 === '' && |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
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 |
I updated my understanding onConflictDoNothing property. |
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? |
Let's do it! |
Huge kudos to you and your team for the great contribution :) |
🎉 This PR is included in version 6.22.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@sdepold sequelize/website#210 related to this PR |
Pull Request Checklist
Please make sure to review and check all of these items:
yarn test
oryarn test-DIALECT
pass with this change (including linting)?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:
Feature/oracle support #12862
Oracle support #6903
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
Todos