Crud: eager join to relation table with composite key fails with column naming assumption, producing incorrect SQL.

[mkelandis]

Hi - First off, thank you for all the hard work, nestjs looks like it's going to be a good fit for our next project.

while evaluating I'm seeing the following issue:
typeorm crud service, when joining using querybuilder (setJoin), assumes that the relation table (rightside) has a column with the same name matching the leftside table’s referenced column, and it includes it in the query which then proceeds to fail since the column doesn’t exist. This breaks implementations like mine where the relation entity models a relationship with a composite key, with no single “id” column.

Here is the error I'm seeing:

query failed: SELECT "Applicant"."id" AS "Applicant_id", "Applicant"."status" AS "Applicant_status", "Applicant"."deleted" AS "Applicant_deleted", "Applicant"."dateCreated" AS "Applicant_dateCreated", "Applicant"."dateUpdated" AS "Applicant_dateUpdated", "Applicant"."version" AS "Applicant_version", "applicantSkills"."applicantId" AS "applicantSkills_applicantId", "applicantSkills"."skillId" AS "applicantSkills_skillId", "applicantSkills"."experience" AS "applicantSkills_experience", applicantSkills.id FROM "applicant" "Applicant" LEFT JOIN "applicant_skill" "applicantSkills" ON "applicantSkills"."applicantId"="Applicant"."id"
error: { error: missing FROM-clause entry for table "applicantskills"
    at Connection.parseE (/Users/mlandis/repos/git/bqp/node_modules/pg/lib/connection.js:604:11)
    at Connection.parseMessage (/Users/mlandis/repos/git/bqp/node_modules/pg/lib/connection.js:401:19)
    at Socket.<anonymous> (/Users/mlandis/repos/git/bqp/node_modules/pg/lib/connection.js:121:22)
    at Socket.emit (events.js:198:13)
    at addChunk (_stream_readable.js:288:12)
    at readableAddChunk (_stream_readable.js:269:11)
    at Socket.Readable.push (_stream_readable.js:224:10)
    at TCP.onStreamRead [as onread] (internal/stream_base_commons.js:94:17)
  name: 'error',
  length: 127,
  severity: 'ERROR',
  code: '42P01',
  detail: undefined,
  hint: undefined,
  position: '476',
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  schema: undefined,
  table: undefined,
  column: undefined,
  dataType: undefined,
  constraint: undefined,
  file: 'parse_relation.c',
  line: '3239',
  routine: 'errorMissingRTE' }

Consider the following three tables:

applicant (left side)
------------
id PK
name

skill
-----
id PK
name

applicant_skill (right side, composite key)
------------------
applicantId PK, FK (applicant.id)
skillId PK, FK (skill.id)
experience

Where the relationships look like:
applicant ← one-to-many → applicant_skill ← many-to-one → skill
“one applicant has many applicant skills”
“one skill has many applicant skills”

In my applicant crud controller, I would like to eagerly return the list of applicant skills when the applicants are queried. It’s decorated like:

@Crud({
  model: {
    type: Applicant,
  },
  query: {
    join: {
      applicantSkills: {
        eager: true
      }
    }
  }
})
@ApiUseTags('applicants')
@Controller('applicants')
export class ApplicantController implements CrudController<Applicant> {
  constructor(public service: ApplicantService) {}
}

After debugging the issue, the problem arises where relation.referencedColumn is automatically added to the select (~line 322, locally). relation = ‘applicant_skill’, referencedColumn = ‘id’, ← which doesn’t exist. If I comment out this line, the query is successful and the data is returned, including the eager loading on the joined table. What is the use-case where this line is needed? See below for commented out line:

.../@nestjsx/crud-typeorm/lib/typeorm-crud.service.js

 setJoin(cond, joinOptions, builder) {
        if (this.entityRelationsHash[cond.field] === undefined && cond.field.includes('.')) {
            const curr = this.getRelationMetadata(cond.field);
            if (!curr) {
                this.entityRelationsHash[cond.field] = null;
                return true;
            }
            this.entityRelationsHash[cond.field] = {
                name: curr.propertyName,
                type: this.getJoinType(curr.relationType),
                columns: curr.inverseEntityMetadata.columns.map((col) => col.propertyName),
                referencedColumn: (curr.joinColumns.length
                    ? curr.joinColumns[0]
                    : curr.inverseRelation.joinColumns[0]).referencedColumn.propertyName,
                nestedRelation: curr.nestedRelation,
            };
        }
        if (cond.field && this.entityRelationsHash[cond.field] && joinOptions[cond.field]) {
            const relation = this.entityRelationsHash[cond.field];
            const options = joinOptions[cond.field];
            const allowed = this.getAllowedColumns(relation.columns, options);
            if (!allowed.length) {
                return true;
            }
            const columns = !cond.select || !cond.select.length
                ? allowed
                : cond.select.filter((col) => allowed.some((a) => a === col));
            const select = [
                // relation.referencedColumn,
                ...(options.persist && options.persist.length ? options.persist : []),
                ...columns,
            ].map((col) => `${relation.name}.${col}`);
            const relationPath = relation.nestedRelation || `${this.alias}.${relation.name}`;
            builder[relation.type](relationPath, relation.name);
            builder.addSelect(select);
        }
        return true;
    }

[mkelandis]

...quick update... I'm trying to understand this a little better so I'm forking and working on a PR, but removing this line breaks a test. I'll take this up and update shortly or close it out.

[mkelandis]

Here is PR for consideration...
https://github.com/nestjsx/crud/pull/248/files

...I have a test locally that sets up a fixture with the relationship and exercises this change. I can't push the commit yet because it had a side effect that somehow impacts should return joined entity, 2

[mkelandis]

The PR has been updated with the new test, and one additional code change to address the broken one. I'm happy to make changes based on the review or to conform to your dev procedures. -Mike

[agarbund]

I've recently encountered the same issue with the composite primary key and was really happy to see it's been already fixed. Thanks for that @mkelandis !

@zMotivat0r any idea when it'll be released?