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

Support orUpdate for SQLite #4097

Merged
merged 4 commits into from May 10, 2019
Merged

Support orUpdate for SQLite #4097

merged 4 commits into from May 10, 2019

Conversation

emibloque
Copy link
Contributor

Sumary

This PR adds support of QueryBuilder's orUpdate for SQLite.

Issue

This closes issue #4096

Background

In PR #3056, support for ON CONFLICT was added for MySQL and Postgres when using orUpdate, and in this PR #3190 support for Postgres onConflict was added for SQLite as they have the same syntax.

test/github-issues/4096/issue-4096.ts Outdated Show resolved Hide resolved
test/github-issues/4096/issue-4096.ts Outdated Show resolved Hide resolved
@@ -249,7 +249,7 @@ export class InsertQueryBuilder<Entity> extends QueryBuilder<Entity> {
if (statement && statement.overwrite instanceof Array) {
if (this.connection.driver instanceof MysqlDriver) {
this.expressionMap.onUpdate.overwrite = statement.overwrite.map(column => `${column} = VALUES(${column})`).join(", ");
} else if (this.connection.driver instanceof PostgresDriver) {
} else if (this.connection.driver instanceof PostgresDriver || this.connection.driver instanceof AbstractSqliteDriver) {
this.expressionMap.onUpdate.overwrite = statement.overwrite.map(column => `${column} = EXCLUDED.${column}`).join(", ");
Copy link
Member

Choose a reason for hiding this comment

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

hm so confusing, is it onUpdate or orUpdate? Looks like second, not sure who named it onUpdate before then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could take advantage of this PR and change the key within expressionMap to orUpdate if you see it ok.

Copy link
Member

Choose a reason for hiding this comment

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

we can left it as it is right now, and change it later (do it if you want), I just commented on what confused me...

@emibloque
Copy link
Contributor Author

@pleerock I want to mean SQLite doesn't seem to support Typeorm's orUpdate.

@emibloque
Copy link
Contributor Author

emibloque commented May 9, 2019

I'm not sure if the error in CI is related with this PR (the last commit was only to solve the conflict in the Changelog file).
Is there anything I can do?

Error message
persistence > many-to-one uni-directional relation
       "before each" hook for "should work when relation id is directly set into relation (without related object)":
     QueryFailedError: Error: Cannot drop the table '#A0F48D65', because it does not exist or you do not have permission.
      at new QueryFailedError (build/compiled/src/error/QueryFailedError.js:3:308)
      at /home/travis/build/typeorm/typeorm/src/driver/sqlserver/SqlServerQueryRunner.ts:44:5
      at _query (node_modules/mssql/lib/base.js:1291:25)
      at Request.tds.Request.err [as userCallback] (node_modules/mssql/lib/tedious.js:656:15)
      at Request._this.callback (node_modules/tedious/lib/request.js:61:27)
      at Connection.endOfMessageMarkerReceived (node_modules/tedious/lib/connection.js:1944:20)
      at Connection.dispatchEvent (node_modules/tedious/lib/connection.js:1012:38)
      at Parser.<anonymous> (node_modules/tedious/lib/connection.js:812:18)
      at Parser.<anonymous> (node_modules/tedious/lib/token/token-stream-parser.js:54:15)
      at addChunk (node_modules/readable-stream/lib/_stream_readable.js:291:12)
      at readableAddChunk (node_modules/readable-stream/lib/_stream_readable.js:278:11)
      at Parser.Readable.push (node_modules/readable-stream/lib/_stream_readable.js:245:10)
      at Parser.Transform.push (node_modules/readable-stream/lib/_stream_transform.js:148:32)
      at Parser.afterTransform (node_modules/readable-stream/lib/_stream_transform.js:91:10)
      at Parser._transform (node_modules/tedious/lib/token/stream-parser.js:69:9)
      at Parser.Transform._read (node_modules/readable-stream/lib/_stream_transform.js:184:10)
      at Parser.Transform._write (node_modules/readable-stream/lib/_stream_transform.js:172:83)
      at doWrite (node_modules/readable-stream/lib/_stream_writable.js:428:64)
      at writeOrBuffer (node_modules/readable-stream/lib/_stream_writable.js:417:5)
      at Parser.Writable.write (node_modules/readable-stream/lib/_stream_writable.js:334:11)
      at Parser.addEndOfMessageMarker (node_modules/tedious/lib/token/token-stream-parser.js:80:26)
      at Connection.message (node_modules/tedious/lib/connection.js:1935:32)
      at Connection.dispatchEvent (node_modules/tedious/lib/connection.js:1012:38)
      at MessageIO.<anonymous> (node_modules/tedious/lib/connection.js:891:18)
      at Message.<anonymous> (node_modules/tedious/lib/message-io.js:60:15)
      at endReadableNT (node_modules/readable-stream/lib/_stream_readable.js:1010:12)
      at _combinedTickCallback (internal/process/next_tick.js:139:11)
      at process._tickCallback (internal/process/next_tick.js:181:9)

@pleerock
Copy link
Member

pleerock commented May 9, 2019

@emibloque its not your fault sometimes it hangs.

@pleerock
Copy link
Member

pleerock commented May 9, 2019

Looks like okay to merge it @Kononnable

@Kononnable Kononnable merged commit 12cc870 into typeorm:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants