ucast icon indicating copy to clipboard operation
ucast copied to clipboard

table name "xxx" specified more than once - objection

Open MoJo2600 opened this issue 3 years ago • 8 comments

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" = ?))

MoJo2600 avatar Jan 14 '21 14:01 MoJo2600

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 avatar Jan 20 '21 14:01 stalniy

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

stalniy avatar Jan 20 '21 14:01 stalniy

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.

MoJo2600 avatar Jan 20 '21 14:01 MoJo2600

Yes, report please everything here

stalniy avatar Jan 20 '21 16:01 stalniy

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

stalniy avatar Jan 20 '21 16:01 stalniy

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?

MoJo2600 avatar May 03 '21 08:05 MoJo2600

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

stalniy avatar May 03 '21 13:05 stalniy

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.

MoJo2600 avatar May 04 '21 07:05 MoJo2600