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

fix for WrapMysqlModifySubqueryTransformation when using join builder #2407

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

falkenhawk
Copy link
Contributor

ported from ovos#10

Fixes error where .onIn()/.andOnIn() causes:
TypeError: parentQuery.isUpdate is not a function

This happens when we provide .onIn() with an objection query.

BlogPost.query(db)
  .alias('post')
  .innerJoin('post.comment', (join) => {
    join.onIn(
      'comment.author_id',
      Author.query(db).where('active', true)
    );
  });

Since WrapMysqlModifySubqueryTransformation does not make sense for join query, we simply skip it for JoinBuilder.

Fixes and error where .andOnIn() causes:
"TypeError: parentQuery.isUpdate is not a function"
@falkenhawk
Copy link
Contributor Author

I've added a typings test for join.onIn + subquery case, but then I realized objection's typings currently do not allow that, because Knex.JoinClause is referenced directly

(table: TableRef<QB>, cb: CallbackVoid<Knex.JoinClause>): QB;

and that, in turn, has only those variants listed for onIn methods (allowing any[] | Raw for second argument)

https://github.com/knex/knex/blob/b6d04f75b3c1956be8ffa26740dd1e27909dd25d/types/index.d.ts#L1586-L1591

and I remembered, it just worked for us (joins with onIn + subquery) when we @ts-ignore'd that.
But in objection v3 we started getting those errors mentioned above in the PR description, that's why we did that change to the objection's code, while still having those onIns ts-ignored.

Since those are proven to work just fine, it would be better to extend the JoinClause typings in https://github.com/Vincit/objection.js/blob/23b8ac112ff095b5d6d8ac2f60b9af87d9028a77/typings/objection/index.d.ts and add tests for such join + onIn variants somewhere along those lines

it('.join() with a subquery', () => {

(there aren't many test cases for .*join() methods anyway)

@lehni
Copy link
Collaborator

lehni commented May 19, 2023

@falkenhawk could you look into why this is currently failing on Node 16?

@falkenhawk
Copy link
Contributor Author

They fail because of typing errors coming from tsc,

Error: tests/ts/query-builder-api/join-methods.ts(64,29): error TS2345: Argument of type 'QueryBuilder<Person, Person[]>' is not assignable to parameter of type 'readonly any[] | Raw<any>'.
  Type 'QueryBuilder<Person, Person[]>' is missing the following properties from type 'Raw<any>': wrap, toSQL, queryContext, addListener, and 22 more.

because , as I mentioned in my previous comment, Knex.JoinClause is referenced directly for all join methods, so typings do not accept objection's QueryBuilder to be passed to any join method's argument, but it is handled by objection and works fine.

To make it right, typings need a rewrite on objection's side to allow objection's QueryBuilder in places where knex's QueryBuilder is accepted. Tests are also missing for such cases... I think they were left out to trust knex's test suite that join methods work instead.

@lehni
Copy link
Collaborator

lehni commented Jun 29, 2023

@falkenhawk thank you for the explanation, and apologies for initially not reading the full description thoroughly. Would you be willing to work on the missing types? I'd be OK with the missing tests.

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.

3 participants