Skip to content

refactor: Query builders, drivers, entity/column/relation metadata, decorators#7058

Closed
nebkat wants to merge 36 commits intotypeorm:masterfrom
nebkat:query-builder-refactor
Closed

refactor: Query builders, drivers, entity/column/relation metadata, decorators#7058
nebkat wants to merge 36 commits intotypeorm:masterfrom
nebkat:query-builder-refactor

Conversation

@nebkat
Copy link
Copy Markdown
Contributor

@nebkat nebkat commented Nov 11, 2020

Description of change

Major refactor of query builders and other parts of code, big improvements in readability and lots of duplicate code removed.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@nebkat nebkat force-pushed the query-builder-refactor branch from 84d649a to 9bbdaca Compare November 11, 2020 23:18
if (this.connection.driver instanceof MysqlDriver || this.connection.driver instanceof AuroraDataApiDriver) {
return " LIMIT " + limit;
} else {
throw new LimitOnUpdateNotSupportedError();
Copy link
Copy Markdown
Contributor Author

@nebkat nebkat Nov 12, 2020

Choose a reason for hiding this comment

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

Rename to LimitNotSupportedError?

@nebkat nebkat force-pushed the query-builder-refactor branch from 9bbdaca to db7a5d5 Compare November 12, 2020 00:54
Comment thread src/query-builder/builder/SoftDeleteQueryBuilder.ts
@nebkat nebkat force-pushed the query-builder-refactor branch 3 times, most recently from 7d04672 to 0c742f6 Compare November 12, 2020 01:29
@nebkat nebkat marked this pull request as ready for review November 12, 2020 02:21
@nebkat nebkat force-pushed the query-builder-refactor branch 10 times, most recently from 418c9c5 to c5ce7f7 Compare November 16, 2020 01:45
@pleerock
Copy link
Copy Markdown
Member

I'm not sure how to review this PR since change diff isn't clear

@nebkat nebkat force-pushed the query-builder-refactor branch 3 times, most recently from c36d4f0 to f41aae2 Compare November 16, 2020 14:34
@nebkat
Copy link
Copy Markdown
Contributor Author

nebkat commented Nov 16, 2020

@pleerock I've broken some of the commits up into smaller parts and added extra descriptions to the commit messages. It should now be easier to follow if you review commits one by one. Let me know if you need any further clarifications, am free to chat if necessary.

@nebkat nebkat changed the title refactor: Query builders refactor: Query builders and entity/column/relation metadata Nov 16, 2020
@nebkat nebkat force-pushed the query-builder-refactor branch from c5efbe1 to b96a0ba Compare November 17, 2020 22:05
@nebkat nebkat force-pushed the query-builder-refactor branch 3 times, most recently from 70e25b4 to f3e4f7d Compare November 19, 2020 00:44
Copy link
Copy Markdown
Contributor Author

@nebkat nebkat left a comment

Choose a reason for hiding this comment

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

Ignore this comment, can't delete

…vers

Some drivers require functions such as ST_AsGeoJSON when persisting or selecting columns, this
moves the wrapping of the persist/select expressions from the query builder to the driver.
SAP HANA doesn't support null values as parameters so they are converted to the
raw NULL value. This moves that conversion from query builder to SapDriver.
Makes Driver.parametrizeValue() an optional function, currently implemented
only by SqlServer. Other drivers can now also add additional information to
their parameters if necessary.
…yBuilder

Moves all functionality from SoftDeleteQueryBuilder to UpdateQueryBuilder except the
"from()" function used to specific "soft delete FROM table", because a similar method
does not currently exist for UpdateQueryBuilder.
In preparation for "virtual"/computed columns, call internally generated columns "internal"
rather than "virtual" to avoid confusion.
…ction)

All options in @column() define a real column except for embedded. This adds an additional
@Embedded decorator for more explicitly defining embeddeds while still maintaining the
option in @column().
* Introduces new type to describe the fields parameter of an @Index or @unique.
* Allows (type) => [type.column] syntax inside embedded
…yMetadatas()

* Removes unnecessary array pushing/slicing
* Matches buildMigrations()/buildSubscribers() more closely
…elete

* Rename ModificationQueryBuilder to AbstractModifyQueryBuilder (UPDATE/DELETE)

* New AbstractPersistQueryBuilder for INSERT/UPDATE/DELETE
    * Common code for RETURNING expressions
    * Common code for INSERT/UPDATE value expressions

* Move UpdateDateColumn, VersionDateColumn, etc to value expression calculation
    * Matches InsertQueryBuilder behavior
refactor: driver: Move database specific EntityManagers to driver

Accidentally ammended during rebase
…lags and generators

Introduces `DriverConfig` and `DriverQueryGenerators` interfaces that include various
flags that drivers can use to customize behavior of the built in functionality. This
decouples these built in features from the drivers themselves, moving towards the
possibility of having each driver in a separate package.
@nebkat nebkat force-pushed the query-builder-refactor branch from a128d91 to 9181163 Compare February 26, 2021 00:51
@nebkat nebkat force-pushed the query-builder-refactor branch from 103b570 to 335aff3 Compare February 26, 2021 02:36
@nebkat
Copy link
Copy Markdown
Contributor Author

nebkat commented Feb 26, 2021

@pleerock I've refactored this to keep up with the latest changes but you can imagine it's quite a complicated one since I have modified so many files, any chance you could take a look at it now? It is becoming rather complicated to keep up with it when working on my other PRs (expression builder and upsert). I know there it is a very large diff but on the individual commit level it should actually be quite easy to review.

There are some nice benefits especially on the driver abstraction front - the number of occurrences of driver instanceof goes down from ~165 to ~70 and most of the remaining ones are Mongo + the complicated Oracle/SqlServer stuff. Also generally a lot of duplicate code is removed.

I also see you have upgraded TypeScript and were working on the merge next into master, could you let me know what the plan is for the near future so I can adjust the other PRs if necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants