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

table name "xxx" specified more than once - objection #17

Open
MoJo2600 opened this issue Jan 14, 2021 · 8 comments · May be fixed by #18
Open

table name "xxx" specified more than once - objection #17

MoJo2600 opened this issue Jan 14, 2021 · 8 comments · May be fixed by #18
Labels
bug Something isn't working ucast/sql

Comments

@MoJo2600
Copy link

I'm using you gist code to check casl rights directly at the database. But when i do a eager or withGraphJoined the interpret function will add a second join for the relation, leading to the error table name "xxx" specified more than once - objection. My ability checks if the user id is either TechnicalContact or AccountOwner.

I see there is a sample which seems to prevent adding a new relation join, but this is not available with objection.

My code:

import {interpret} from '@ucast/sql/objection';

toObjectionQuery<M extends Model>(
  ability: AnyAbility,
  action: string,
  query: QueryBuilder<M, M[]>
): QueryBuilder<M, M[]> {
  const q =
    rulesToQuery(ability, action, query.modelClass(), rule => {
      if (!rule.ast) {
        throw new Error('Unable to create Objection.Query without AST');
      }
      return rule.ast;
    }) || {};

  if (q) {
    const {$and = [], $or = []} = q;
    const condition = new CompoundCondition('and', [
      ...$and,
      new CompoundCondition('or', [...$or] as CompoundCondition[]),
    ] as CompoundCondition[]);

    return interpret(
      condition,
      query as QueryBuilder<Model, Model[]>
    ) as QueryBuilder<M, M[]>;
  }
  return query;
}

const ability = fastify.userAbilities;

// Create query that takes abilities into account
const accountQuery = this.toObjectionQuery<AwsAccountModel>(
  ability,
  'read',
  AwsAccountModel.query().withGraphJoined(
    '[technicalContact,accountOwner,costcenterData]'
  )
);

Error:
Results in an sql query like this: table name "technical_contact" specified more than once

and the sql looks somewhat like this:

...
INNER JOIN "users" AS "technical_contact"
ON "technical_contact"."id" = CAST("acc"."data"#>>'{TechnicalContact}' AS text)
LEFT JOIN "users" AS "technical_contact"
ON "technical_contact"."id" = CAST("acc"."data"#>>'{TechnicalContact}' AS text)
...
WHERE (("technical_contact"."id" = ? or "account_owner"."id" = ?))
@stalniy
Copy link
Owner

stalniy commented Jan 20, 2021

Sorry for the long response. @ucast/sql is still in alpha phase. I didn't have much time to experiment with different configurations, so thank you very much for the use case. I'll take a look at it when finish vue3 support for casl

@stalniy
Copy link
Owner

stalniy commented Jan 20, 2021

Ah, the fix should be trivial if objection provides an api to check whether the table was joined before and retrieve its alias

@stalniy stalniy added bug Something isn't working ucast/sql labels Jan 20, 2021
@MoJo2600
Copy link
Author

No worries. I just do some testing and experimenting. If it is ok for you, i'll report my experiences here. If you give me a hint on how you would solve this issue, i can try to create a pull request.

@stalniy
Copy link
Owner

stalniy commented Jan 20, 2021

Yes, report please everything here

@stalniy
Copy link
Owner

stalniy commented Jan 20, 2021

I think at this point we need to add additional check that ensures relation was not joined before

MoJo2600 pushed a commit to MoJo2600/ucast that referenced this issue Apr 30, 2021
When a relation was already joined with `withGraphJoined`, ucast was adding the relation
a second time. Now a check for the relation is performed.

fixes stalniy#17
@MoJo2600
Copy link
Author

MoJo2600 commented May 3, 2021

Hey, one small issue. The existing tests do all work, but to test withGraphJoined we would need a database connection as withGraphJoined needs metadata to be loaded (See how it is done in the objection repository: https://github.com/Vincit/objection.js/blob/260b284a1cbfb044991894c5a3cf3dedc8ce7267/tests/integration/toKnexQuery.js)

The error is: Error: table metadata has not been fetched. Are you trying to call toKnexQuery() for a withGraphJoined query? To make sure the table metadata is fetched see the objection.initialize function.. Calling initialize will try to connect to the database.

I'm not sure how to continue from here. Not sure if it makes sense to add integration tests to ucast/sql? What would you propose?

@stalniy
Copy link
Owner

stalniy commented May 3, 2021

Can we check existence of joined relation without waiting for metadata fetching? Maybe on knex level? Just don’t understand why we need this

@MoJo2600
Copy link
Author

MoJo2600 commented May 4, 2021

Sorry, I was a bit in a hurry yesterday. I should have added more information and not waste your time. Here is everything i figured out so far:

I created a test like this:

  it('doest not join relation again when condition is set on relation field and already joined', () => {
    const condition = new FieldCondition('eq', 'projects.name', 'test')
    const userQuery = User.query().withGraphJoined('[projects]')
    const query = interpret(condition, userQuery)

    expect(query.toKnexQuery().toString()).to.equal(linearize`
      select "users".*
      from "users"
        inner join "projects" on "projects"."user_id" = "users"."id"
      where "projects"."name" = 'test'
    `.trim())
  })

This results in the error: Error: table metadata has not been fetched. Are you trying to call toKnexQuery() for a withGraphJoined query? To make sure the table metadata is fetched see the objection.initialize function.

If I add the initialize method of objection.js like this:

const knex = Knex({ client: 'pg' })
initialize(knex, [User, Project]).then()

it results in an timeout, because the initialize method tries to connect to the database to retrieve the metadata.

So I'm not sure how to properly test the resulting sql from an objection.js query with withGraphJoined as it seems to require a database connection. Objection.js itself is using an integration test with a mock database to test withGraphJoined queries. I'm not a expert in objection.js, but maybe there is another way or I'm on the wrong track on how to add a test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ucast/sql
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants