Skip to content

Conversation

@LucasHFS
Copy link
Contributor

@LucasHFS LucasHFS commented May 6, 2021

attempt to solve #2544 and make the #2337 functionality work

@LucasHFS LucasHFS force-pushed the 2544_inconsistences_select_returning branch 3 times, most recently from 7b01267 to c41fd20 Compare May 7, 2021 04:10
@LucasHFS LucasHFS force-pushed the 2544_inconsistences_select_returning branch from c41fd20 to 0cf3a56 Compare May 7, 2021 04:17
@LucasHFS LucasHFS force-pushed the 2544_inconsistences_select_returning branch from ca9843e to 3bb59fd Compare May 7, 2021 13:12
@LucasHFS LucasHFS force-pushed the 2544_inconsistences_select_returning branch 2 times, most recently from 4bed4b1 to 8bf2ea8 Compare May 7, 2021 13:41
@kibertoad
Copy link
Collaborator

@LucasHFS Do I understand correctly that this is a breaking change which modifies existing behaviour?

@LucasHFS
Copy link
Contributor Author

Yes, I was searching for something to contribute with. I started by this #2544. Then I saw #2337 after a few days that this PR covers.

@kibertoad
Copy link
Collaborator

kibertoad commented May 11, 2021

@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?

@LucasHFS
Copy link
Contributor Author

Searching for issues to solve, This PR solves #2571 as well

@fdevibe
Copy link

fdevibe commented Jun 20, 2021

I would really appreciate it if this was merged. As described in #2571, this also prevents returning geometry in a usable format, like with ST_AsGeoJSON() (AFAICT).

@kibertoad
Copy link
Collaborator

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.

@kibertoad
Copy link
Collaborator

OK, we are starting to prepare for 1.0.0, so now the window for breaking changes is open.
@LucasHFS Could you please rebase this PR?

@kibertoad
Copy link
Collaborator

@OlivierCavadenti Can you take this one?

@OlivierCavadenti OlivierCavadenti self-assigned this Jan 6, 2022
@kibertoad kibertoad mentioned this pull request Jan 12, 2022
# Conflicts:
#	test/integration/execution/transaction.js
#	test/integration/migrate/migration-integration-tests.js
#	test/integration/query/inserts.js
#	test/integration/query/trigger-inserts.js
@OlivierCavadenti
Copy link
Collaborator

OlivierCavadenti commented Jan 13, 2022

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

@kibertoad
Copy link
Collaborator

@OlivierCavadenti That happens if you rerun migration after deleting some migration files that were run previously. Consider deleting knex changelog table

@kibertoad
Copy link
Collaborator

[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

@kibertoad
Copy link
Collaborator

ah, so ci has it too. guess we broke migrator logic for fetching previously ran migrations

@OlivierCavadenti
Copy link
Collaborator

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)

@kibertoad kibertoad merged commit b42bc46 into knex:master Jan 13, 2022
@kibertoad
Copy link
Collaborator

@OlivierCavadenti Incredible work, thank you so much! I will fix the linting issue tomorrow myself.
Do we need to update documentation after this change?

@OlivierCavadenti
Copy link
Collaborator

@OlivierCavadenti Incredible work, thank you so much! I will fix the linting issue tomorrow myself. Do we need to update documentation after this change?

Sure : knex/documentation#378

@kibertoad
Copy link
Collaborator

Released in 1.0.1.

falkenhawk added a commit to ovos/objection.js that referenced this pull request Mar 27, 2023
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
kibertoad pushed a commit to Vincit/objection.js that referenced this pull request Mar 30, 2023
* 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.
Pl217 added a commit to UN-OCHA/hpc-api-core that referenced this pull request Aug 23, 2024
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
Pl217 added a commit to UN-OCHA/hpc-api-core that referenced this pull request Aug 23, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants