-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Inconsistencies between .select and .returning #4471
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
Inconsistencies between .select and .returning #4471
Conversation
7b01267 to
c41fd20
Compare
c41fd20 to
0cf3a56
Compare
ca9843e to
3bb59fd
Compare
4bed4b1 to
8bf2ea8
Compare
8bf2ea8 to
09aea2c
Compare
|
@LucasHFS Do I understand correctly that this is a breaking change which modifies existing behaviour? |
|
@elhigu How should we handle this? Considering that probably it makes sense to make those two paths consistent, should we merge this PR in time for 1.0.0? |
|
Searching for issues to solve, This PR solves #2571 as well |
|
I would really appreciate it if this was merged. As described in #2571, this also prevents returning geometry in a usable format, like with |
|
I'll look into this in the upcoming weeks. Considering that Objection still hasn't updated to the latest semver major, I think there is still space to squeeze in another breaking change. |
|
OK, we are starting to prepare for 1.0.0, so now the window for breaking changes is open. |
|
@OlivierCavadenti Can you take this one? |
# Conflicts: # test/integration/execution/transaction.js # test/integration/migrate/migration-integration-tests.js # test/integration/query/inserts.js # test/integration/query/trigger-inserts.js
|
I have a lot of " The migration directory is corrupt, the following files are missing: [object Object], [object Object], [object Object]" errors, do you have clues about the problem ? @kibertoad |
|
@OlivierCavadenti That happens if you rerun migration after deleting some migration files that were run previously. Consider deleting knex changelog table |
|
[object] thing looks very sus, though, probably we broke logging after changing migration file structure, but maybe it comes from other branch being used before |
|
ah, so ci has it too. guess we broke migrator logic for fetching previously ran migrations |
... in fact I miss to report one test correction with the rebase... now seems good (excepted Lint issue) |
|
@OlivierCavadenti Incredible work, thank you so much! I will fix the linting issue tomorrow myself. |
Sure : knex/documentation#378 |
|
Released in 1.0.1. |
knex v1+ changed the return values when `.returning()` clause is used (affecting postgres), before it returned array of primitive values if only 1 column was selected to be returned (e.g. `[1]` for `.returning('id')`) but now it always returns objects (e.g. `[{ id: 1 }]` for `.returning('id')`).
Slightly reorganized `InsertOperation`, instead of calling `createModels` in `doExecute()` (right after `onRawResult` hook) and having "empty" model instances created there, possibly with undefined / declared default properties which would later overwrite input models in `InsertOperation.onAfter1`, offload the handling to `InsertOperation.onAfter1`, which now uses also `setDatabaseJson`, same as `createModels` function, to be consistent.
Additionally, adjusted `$parseDatabaseJson` in `GraphInsert` integration tests, to only parse boolean values when they are present in `json`.
related change in knex v1.0: knex/knex#4471
* bump typescript, knex and @types/node to get rid of typing errors in test:typings
* adjustments for knex v1+
knex v1+ changed the return values when `.returning()` clause is used (affecting postgres), before it returned array of primitive values if only 1 column was selected to be returned (e.g. `[1]` for `.returning('id')`) but now it always returns objects (e.g. `[{ id: 1 }]` for `.returning('id')`).
Slightly reorganized `InsertOperation`, instead of calling `createModels` in `doExecute()` (right after `onRawResult` hook) and having "empty" model instances created there, possibly with undefined / declared default properties which would later overwrite input models in `InsertOperation.onAfter1`, offload the handling to `InsertOperation.onAfter1`, which now uses also `setDatabaseJson`, same as `createModels` function, to be consistent.
Additionally, adjusted `$parseDatabaseJson` in `GraphInsert` integration tests, to only parse boolean values when they are present in `json`.
related change in knex v1.0: knex/knex#4471
* introduced new (or missing) knex querybuilder's methods
to make shapes of both querybuilders as close to each other as possible.
Most of the methods were introduced in knex v1.
- `fromRaw`
- `whereLike`
- `andWhereLike`
- `orWhereLike`
- `whereILike`
- `andWhereILike`
- `orWhereILike`
- `withMaterialized`
- `withNotMaterialized`
- `jsonExtract`
- `jsonSet`
- `jsonInsert`
- `jsonRemove`
- `whereJsonObject`
- `orWhereJsonObject`
- `andWhereJsonObject`
- `whereNotJsonObject`
- `orWhereNotJsonObject`
- `andWhereNotJsonObject`
- `whereJsonPath`
- `orWhereJsonPath`
- `andWhereJsonPath`
and one new `JoinBuilder` method - `andOnJsonPathEquals` (referencing knex's `JoinClause` method of the same name)
`whereJson(Not)SupersetOf` / `whereJson(Not)SubsetOf` are now supported by knex >= 1.0, but for now
objection handles them differently and only for postgres.
Changing them to utilize knex methods directly may require a major version bump and upgrade guide.
Added a test for checking JoinBuilder methods to be inline with knex's JoinClause.
Fixed incorrect checking QueryBuilder methods if they are inline with knex's QueryBuilder.
Added typings for new methods.
Bumped knex `peerDependency` from `>=0.95.13` to `>=1.0.1` because of new methods.
This is latest Knex version v1.x.x https://github.com/knex/knex/blob/176151d8048b2a7feeb89a3d649a5580786d4f4e/CHANGELOG.md#107---13-april-2022 The only thing from Migration guide (linked above) that looks like it can affect us is: > `RETURNING` operations now always return an object with column names; But looking at PR which added this feature - knex/knex#4471 and PR that updated documentation - knex/documentation#378 we can see that we're not affected, since we always use `returning` as `returning('*')` Changes to type definitions have one important change, since `QueryCallback` was defined like this in `v0.95.15` https://github.com/knex/knex/blob/0.95/types/index.d.ts#L1782 ```ts type QueryCallback<TRecord = any, TResult = unknown[]> = ( ... ) => void; ``` which has changed to this in `v1.0.7` https://github.com/knex/knex/blob/1.0.7/types/index.d.ts#L1842 ```ts type QueryCallback<TRecord extends {} = any, TResult = unknown[]> = ( ... ) => void; ``` We can see that `extends {}` was added to `TRecord`, so we need to make the same adjustment, otherwise we get an error "Type `InstanceData` does not satisfy the constraint `{}`" After this change, we can also reduce type assertions in query building
This is latest Knex version v1.x.x https://github.com/knex/knex/blob/176151d8048b2a7feeb89a3d649a5580786d4f4e/CHANGELOG.md#107---13-april-2022 The only thing from Migration guide (linked above) that looks like it can affect us is: > `RETURNING` operations now always return an object with column names; But looking at PR which added this feature - knex/knex#4471 and PR that updated documentation - knex/documentation#378 we can see that we're not affected, since we always use `returning` as `returning('*')` Changes to type definitions have one important change, since `QueryCallback` was defined like this in `v0.95.15` https://github.com/knex/knex/blob/0.95/types/index.d.ts#L1782 ```ts type QueryCallback<TRecord = any, TResult = unknown[]> = ( ... ) => void; ``` which has changed to this in `v1.0.7` https://github.com/knex/knex/blob/1.0.7/types/index.d.ts#L1842 ```ts type QueryCallback<TRecord extends {} = any, TResult = unknown[]> = ( ... ) => void; ``` We can see that `extends {}` was added to `TRecord`, so we need to make the same adjustment, otherwise we get an error "Type `InstanceData` does not satisfy the constraint `{}`" After this change, we can also reduce type assertions in query building
attempt to solve #2544 and make the #2337 functionality work