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.

Sign up for GitHub

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(core): self-referencing relations Not unique table/alias #2740

Merged
merged 4 commits into from
Mar 18, 2024
Merged

fix(core): self-referencing relations Not unique table/alias #2740

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4034445
fix(core): self-referencing relations `Not unique table/alias`
monrostar Mar 15, 2024
10c0798
fix(core): update self-related manual joins with deep eager relations
monrostar Mar 16, 2024
4824d86
chore(core): clean up code
monrostar Mar 16, 2024
5eb6391
fix(core): update test cases for new TestEntities order
monrostar Mar 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 21 additions & 8 deletions packages/core/e2e/fixtures/test-plugins/list-query-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@ export class TestEntity extends VendureEntity implements Translatable, HasCustom

@Column(() => TestEntityCustomFields)
customFields: TestEntityCustomFields;

@ManyToOne(() => TestEntity, (type) => type.parent)
parent: TestEntity | null;

@Column('int', { nullable: true })
parentId: ID | null;
Comment on lines +147 to +152
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I had to update the model a bit to repeat the error and take it into account in the tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

above fix i think need consider more behavior
if we have translations like

  @OneToMany(
    () => LensProcessOptionTranslation,
    (translation) => translation.base,
    { eager: true }
  )
  translations: Array<Translation<LensProcessOption>>;

the parent should be auto load translations otherwise it will result translateEntity failed

function translateLeaf(
    object: { [key: string]: any } | undefined,
    property: string,
    languageCode: LanguageCode | [LanguageCode, ...LanguageCode[]],
): any {
    if (object && object[property]) {
        if (Array.isArray(object[property])) {
            return object[property].map((nested2: any) => translateEntity(nested2, languageCode));
        } else if (object[property]) {
            return translateEntity(object[property], languageCode);
        }
    }
}

if parent existed but it have auto loaded translations it will broken here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see my latest updated repo https://github.com/tianyingchun/next-issue/tree/vendure_issue_2738

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

InternalServerError [GraphQLError]: error.entity-has-no-translation-in-language
    at new I18nError (/Users/tianyingchun/Documents/coding/kzfoo/vendure-issue/node_modules/@vendure/core/dist/i18n/i18n-error.js:21:9)
    at new InternalServerError (/Users/tianyingchun/Documents/coding/kzfoo/vendure-issue/node_modules/@vendure/core/dist/common/error/errors.js:15:9)
    at translateEntity (/Users/tianyingchun/Documents/coding/kzfoo/vendure-issue/node_modules/@vendure/core/dist/service/helpers/utils/translate-entity.js:33:15)
    at translateLeaf (/Users/tianyingchun/Documents/coding/kzfoo/vendure-issue/node_modules/@vendure/core/dist/service/helpers/utils/translate-entity.js:104:20)
    at translateDeep (/Users/tianyingchun/Documents/coding/kzfoo/vendure-issue/node_modules/@vendure/core/dist/service/helpers/utils/translate-entity.js:89:21)
    at TranslatorService.translate (/Users/tianyingchun/Documents/coding/kzfoo/vendure-issue/node_modules/@vendure/core/dist/service/helpers/translator/translator.service.js:54:53)
    at file:///Users/tianyingchun/Documents/coding/kzfoo/vendure-issue/packages/plugin-issue/src/services/menu.service.ts:82:76
    at Array.map (<anonymous>)
    at file:///Users/tianyingchun/Documents/coding/kzfoo/vendure-issue/packages/plugin-issue/src/services/menu.service.ts:82:37
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  path: undefined,
  locations: undefined,
  extensions: { code: 'INTERNAL_SERVER_ERROR' },
  variables: { entityName: 'Menu', languageCode: ',,en' },
  code: 'INTERNAL_SERVER_ERROR',
  logLevel: 0
}

Copy link
Contributor Author

@monrostar monrostar Mar 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I thought I had accounted for translations for all self-related relations. I will look into this and add join for eager relations

above fix i think need consider more behavior if we have translations like

  @OneToMany(
    () => LensProcessOptionTranslation,
    (translation) => translation.base,
    { eager: true }
  )
  translations: Array<Translation<LensProcessOption>>;

the parent should be auto load translations otherwise it will result translateEntity failed

function translateLeaf(
    object: { [key: string]: any } | undefined,
    property: string,
    languageCode: LanguageCode | [LanguageCode, ...LanguageCode[]],
): any {
    if (object && object[property]) {
        if (Array.isArray(object[property])) {
            return object[property].map((nested2: any) => translateEntity(nested2, languageCode));
        } else if (object[property]) {
            return translateEntity(object[property], languageCode);
        }
    }
}

if parent existed but it have auto loaded translations it will broken here

}

@Entity()
Expand Down Expand Up @@ -188,6 +194,7 @@ export class ListQueryResolver {
.build(TestEntity, args.options, {
ctx,
relations: [
'parent',
'orderRelation',
'orderRelation.customer',
'customFields.relation',
Expand Down Expand Up @@ -274,6 +281,7 @@ const apiExtensions = gql`
nullableId: ID
nullableDate: DateTime
customFields: TestEntityCustomFields!
parent: TestEntity
}

type TestEntityList implements PaginatedList {
Expand Down Expand Up @@ -324,9 +332,9 @@ export class ListQueryPlugin implements OnApplicationBootstrap {
) {}

async onApplicationBootstrap() {
const count = await this.connection.getRepository(TestEntity).count();
const count = await this.connection.rawConnection.getRepository(TestEntity).count();
if (count === 0) {
const testEntities = await this.connection.getRepository(TestEntity).save([
const testEntities = await this.connection.rawConnection.getRepository(TestEntity).save([
new TestEntity({
label: 'A',
description: 'Lorem ipsum', // 11
Expand Down Expand Up @@ -392,6 +400,11 @@ export class ListQueryPlugin implements OnApplicationBootstrap {
}),
]);

// test entity with self-referencing relation without tree structure decorator
testEntities[0].parent = testEntities[1];
testEntities[3].parent = testEntities[1];
await this.connection.rawConnection.getRepository(TestEntity).save([testEntities[0], testEntities[3]]);

const translations: any = {
A: { [LanguageCode.en]: 'apple', [LanguageCode.de]: 'apfel' },
B: { [LanguageCode.en]: 'bike', [LanguageCode.de]: 'fahrrad' },
Expand All @@ -408,7 +421,7 @@ export class ListQueryPlugin implements OnApplicationBootstrap {
};

for (const testEntity of testEntities) {
await this.connection.getRepository(TestEntityPrice).save([
await this.connection.rawConnection.getRepository(TestEntityPrice).save([
new TestEntityPrice({
price: testEntity.description.length,
channelId: 1,
Expand All @@ -424,7 +437,7 @@ export class ListQueryPlugin implements OnApplicationBootstrap {
for (const code of [LanguageCode.en, LanguageCode.de]) {
const translation = translations[testEntity.label][code];
if (translation) {
await this.connection.getRepository(TestEntityTranslation).save(
await this.connection.rawConnection.getRepository(TestEntityTranslation).save(
new TestEntityTranslation({
name: translation,
base: testEntity,
Expand All @@ -436,13 +449,13 @@ export class ListQueryPlugin implements OnApplicationBootstrap {

if (nestedData[testEntity.label]) {
for (const nestedContent of nestedData[testEntity.label]) {
await this.connection.getRepository(CustomFieldRelationTestEntity).save(
await this.connection.rawConnection.getRepository(CustomFieldRelationTestEntity).save(
new CustomFieldRelationTestEntity({
parent: testEntity,
data: nestedContent.data,
}),
);
await this.connection.getRepository(CustomFieldOtherRelationTestEntity).save(
await this.connection.rawConnection.getRepository(CustomFieldOtherRelationTestEntity).save(
new CustomFieldOtherRelationTestEntity({
parent: testEntity,
data: nestedContent.data,
Expand All @@ -452,7 +465,7 @@ export class ListQueryPlugin implements OnApplicationBootstrap {
}
}
} else {
const testEntities = await this.connection.getRepository(TestEntity).find();
const testEntities = await this.connection.rawConnection.getRepository(TestEntity).find();
const ctx = await this.requestContextService.create({ apiType: 'admin' });
const customers = await this.connection.rawConnection.getRepository(Customer).find();
let i = 0;
Expand All @@ -463,7 +476,7 @@ export class ListQueryPlugin implements OnApplicationBootstrap {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const order = await this.orderService.create(ctx, customer.user!.id);
testEntity.orderRelation = order;
await this.connection.getRepository(TestEntity).save(testEntity);
await this.connection.rawConnection.getRepository(TestEntity).save(testEntity);
} catch (e: any) {
Logger.error(e);
}
Expand Down
5 changes: 5 additions & 0 deletions packages/core/e2e/list-query-builder.e2e-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,6 +1285,7 @@ describe('ListQueryBuilder', () => {
id: 'T_1',
label: 'A',
name: 'apple',
parent: { id: 'T_2'},
orderRelation: {
customer: {
firstName: 'Hayden',
Expand All @@ -1297,6 +1298,7 @@ describe('ListQueryBuilder', () => {
id: 'T_4',
label: 'D',
name: 'dog',
parent: { id: 'T_2'},
orderRelation: {
customer: {
firstName: 'Hayden',
Expand Down Expand Up @@ -1412,6 +1414,9 @@ const GET_LIST_WITH_ORDERS = gql`
id
label
name
parent {
id
}
orderRelation {
id
customer {
Expand Down
12 changes: 11 additions & 1 deletion packages/core/src/service/helpers/list-query-builder/list-query-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,17 @@ export class ListQueryBuilder implements OnApplicationBootstrap {
currentAlias: string,
) => {
const currentMetadataIsTreeType = metadata.treeType;
if (!currentParentIsTreeType && !currentMetadataIsTreeType) {
let currentMetadataHasOneOrMoreSelfReferencingRelations = false;
// Check if the current entity has one or more self-referencing relations
// to determine if it is a tree type or has tree relations.
for (const relation of currentMetadata.relations) {
if (relation.inverseEntityMetadata === currentMetadata) {
currentMetadataHasOneOrMoreSelfReferencingRelations = true;
break;
}
}

if (!currentParentIsTreeType && !currentMetadataIsTreeType && !currentMetadataHasOneOrMoreSelfReferencingRelations) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most important code. It is enough to determine whether it is necessary to do join manually or leave this job to typeorm


Expand Down
Loading