Skip to content

Conversation

@falkenhawk
Copy link
Contributor

ported from ovos#15

adjustments for knex v1 and v2

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)

related: knex/knex#4859 knex/knex#5044

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.

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
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.
@kibertoad
Copy link
Collaborator

I'll publish this as a new semver major after releasing minor tomorrow.
Thank you!

@falkenhawk
Copy link
Contributor Author

falkenhawk commented Mar 29, 2023

@kibertoad perhaps for a major version bump it would be worth it to rework whereJson(Not)SupersetOf / whereJson(Not)SubsetOf methods to utilize knex counterparts directly? 🤔 Though that would require changes to function signatures and updates to docs / migration guide.

In this PR the only kind-of-breaking change is minimal knex dependency bump from 0.95 to 1.0.1. While it might still work fine with knex 0.95 unless you start using the new json methods.

@kibertoad kibertoad merged commit 4cd306a into Vincit:master Mar 30, 2023
@kibertoad
Copy link
Collaborator

@falkenhawk I'm open to using this semver major window for addressing that. Could you please send the PR?

@falkenhawk falkenhawk mentioned this pull request Mar 31, 2023
falkenhawk added a commit to ovos/objection.js that referenced this pull request Mar 31, 2023
@falkenhawk
Copy link
Contributor Author

@kibertoad I am not sure if I can make it. Meanwhile I updated knex-related docs in #2375

kibertoad pushed a commit that referenced this pull request Mar 31, 2023
@falkenhawk
Copy link
Contributor Author

rework whereJson(Not)SupersetOf / whereJson(Not)SubsetOf methods to utilize knex counterparts directly? 🤔 Though that would require changes to function signatures and updates to docs / migration guide.

@kibertoad I made an attempt, but as I read through https://vincit.github.io/objection.js/api/query-builder/find-methods.html#wherejsonsupersetof there is an example to reference other json field in a where clause as a value.

const people = await Person.query().whereJsonSupersetOf(
  'additionalData:myDogs',
  'additionalData:dogsAtHome'
);

I am not sure if the same is supported in knex, docs does not say anything about it:
https://knexjs.org/guide/query-builder.html#wherejsonsupersetof

It would be nice to have those json functions available also for other clients, other than postgres, which knex currently supports, but I am afraid of introducing an unnecessary regression, making it a blocker to update to new version of objection.js for postgres users who currently use those functions that way (referencing json fields as values)

Maybe you know more about the internals, or may @OlivierCavadenti (who implemented knex/knex#4859) shed some light here, please?

@falkenhawk
Copy link
Contributor Author

@kibertoad from my investigations it won't be trivial to switch whereJson(Not)SupersetOf / whereJson(Not)SubsetOf directly without limiting current functionality.

Even if "field expressions" - https://github.com/Vincit/objection.js/blob/master/lib/queryBuilder/operations/jsonApi/postgresJsonApi.js#L45-L51 - were translated to combinations of column names + jsonExtract() to reference a field inside json value, knex currently does not support passing querybuilder instances as arguments to whereJson(Not)SupersetOf / whereJson(Not)SubsetOf querybuilder methods, such as .whereJsonSupersetOf('column', q => q.jsonExtract('othercolumn', '$.field.0'))

In my case it is getting stuck in infinite loops, but maybe I tested it wrong. Otherwise changes to knex codebase would be needed to make it work, i.e. allowing to migrate such json queries while keeping all current features on objection's side.

That said, I am not able to dig deeper, as I don't have too much time on my hands currently. I don't want to hold you back there, Feel free to proceed with the release, and in the future, when someone approaches this again, we could have yet another major version bump ;)

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.

2 participants