Repository: MichalLytek/type-graphql
Pull request #1284 (2022-05-21 01:14:48)

fix: use schemaName instead of methodName when building field resolve…
Hi! 👋 
      
Firstly, thanks for your work on this project! 🙂

We are doing something more complicated, but I tried to boil it down to the essence of the issue below. Basically we have a mixin that adds very similar functionality that is parameterized by an argument `name`. We overwrite the `schemaName` appropriately by passing `name` to the field resolver, but when building the field resolver metadata, TypeGraphQL thinks that the subsequent field resolvers have already been added because it checks against the definition's `methodName`. I've run the test suite against the proposed change below, 

Simplified breaking code:
```
const withMixin = <TObjectType extends ClassType<{ id: string }>>(
  ObjectType: TObjectType,
  BaseClass: ClassType,
  name: string
) => {
  @Resolver(() => ObjectType)
  class FoobarResolver extends BaseClass {
    @FieldResolver(() => String, { name })
    async foobar(
      @Arg(`${name}Id`, () => ID) foobarId: string
    ): Promise<any> {
       ...Common logic we want to add via the mixin that is parameterized by `name`...
    }
  }
  return FoobarResolver;
};


MichalLytek: @dobrynin Thanks for your contribution 🙌 
In order to merge your PR, we need you to create a failing test case that reproduce the issue and which is then fixed by your changes in the codebase.
This way any other future refactor won't affect your use case.
codecov[bot]: # [Codecov](https://codecov.io/gh/MichalLytek/type-graphql/pull/1284?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek) Report
> Merging [#1284](https://codecov.io/gh/MichalLytek/type-graphql/pull/1284?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek) (dc65f3f) into [master](https://codecov.io/gh/MichalLytek/type-graphql/commit/3013a1083c5b37749c4b23f32c4cc969600b9058?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek) (3013a10) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##           master    #1284   +/-   ##
=======================================
  Coverage   96.05%   96.05%           
=======================================
  Files          79       79           
  Lines        1697     1698    +1     
  Branches      356      356           
=======================================
+ Hits         1630     1631    +1     
  Misses         66       66           
  Partials        1        1           
```


| [Impacted Files](https://codecov.io/gh/MichalLytek/type-graphql/pull/1284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek) | Coverage Δ | |
|---|---|---|
| [src/metadata/metadata-storage.ts](https://codecov.io/gh/MichalLytek/type-graphql/pull/1284/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek#diff-c3JjL21ldGFkYXRhL21ldGFkYXRhLXN0b3JhZ2UudHM=) | `99.48% <100.00%> (+<0.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/MichalLytek/type-graphql/pull/1284?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/MichalLytek/type-graphql/pull/1284?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek). Last update [3013a10...dc65f3f](https://codecov.io/gh/MichalLytek/type-graphql/pull/1284?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Micha%C5%82+Lytek).

dobrynin: @MichalLytek test added!
dobrynin: Hi @MichalLytek , is it possible to get an estimate for when this change can be merged + released?
MichalLytek: Please don't test metadata storage. It's implementation detail.
Your test cases should describe the real usage and real world assertion, so like "when I create this resolvers with this mixin, it should use this name, so that this field has this name and calls my field resolver properly".
dobrynin: Okay @MichalLytek , I've refactored the test to stop relying on metadata storage and moved the test to the `resolvers.ts` file. Please take a look when you have a chance!
dobrynin: Don't mean to nag you @MichalLytek, but please take a look at this one when you've had a chance. We'd like to stop using our fork of type-graphql :)
itpropro: I have a similar scenario currently, any news when this is integrated into the main branch @MichalLytek ?
MichalLytek: @itpropro Sorry to keep you waiting so long for this simple fix, I constantly forgot to merge after rebase 😛 
Too much opened PRs and issues, too few time for all of that 😞 

@dobrynin Can you check if your branch or PR still have enabled "allow edits from maintainers" setting?
I can't push changes anymore and I wanted to fix the rebase issues in order to merge this PR finally 🙏 

dobrynin: @MichalLytek I don't see any option to do that, I think it may not be possible unfortunately because the fork lives under an organization: https://github.com/orgs/community/discussions/5634
MichalLytek: @dobrynin So could you rebase your branch and fix the lint issue (missing `!` in test sample filed)? 🙏 
dobrynin: @MichalLytek done

Patch:
diff --git a/src/metadata/metadata-storage.ts b/src/metadata/metadata-storage.ts
index 8715e714..da8afa4a 100644
--- a/src/metadata/metadata-storage.ts
+++ b/src/metadata/metadata-storage.ts
@@ -291,7 +291,9 @@ export class MetadataStorage {
           );
         }
 
-        const typeField = typeMetadata.fields!.find(fieldDef => fieldDef.name === def.methodName)!;
+        const typeField = typeMetadata.fields!.find(
+          fieldDef => fieldDef.schemaName === def.schemaName,
+        )!;
         if (!typeField) {
           const shouldCollectFieldMetadata =
             !options.resolvers ||


Test patch:
diff --git a/tests/functional/resolvers.ts b/tests/functional/resolvers.ts
index c1ab50ea..b8cebc22 100644
--- a/tests/functional/resolvers.ts
+++ b/tests/functional/resolvers.ts
@@ -2479,5 +2479,70 @@ describe("Resolvers", () => {
 
       expect(self).toBeInstanceOf(childResolver);
     });
+
+    it("should allow duplicate fieldResolver methods with different schema names for inherited resolvers", async () => {
+      getMetadataStorage().clear();
+      const INHERITED_DYNAMIC_FIELD_NAME_1 = "dynamicallyNamedMethod1";
+      const INHERITED_DYNAMIC_FIELD_NAME_2 = "dynamicallyNamedMethod2";
+
+      const withDynamicallyNamedFieldResolver = (
+        classType: ClassType,
+        BaseResolverClass: ClassType,
+        name: string,
+      ) => {
+        @Resolver(() => classType)
+        class DynamicallyNamedFieldResolver extends BaseResolverClass {
+          @FieldResolver({ name })
+          dynamicallyNamedField(): boolean {
+            return true;
+          }
+        }
+        return DynamicallyNamedFieldResolver;
+      };
+
+      @ObjectType()
+      class SampleObject {
+        @Field()
+        sampleField!: string;
+      }
+
+      @Resolver()
+      class SampleResolver {
+        @Query(() => SampleObject)
+        sampleObject(): SampleObject {
+          return { sampleField: "sampleText" };
+        }
+      }
+
+      const DynamicallyNamedFieldResolver1 = withDynamicallyNamedFieldResolver(
+        SampleObject,
+        SampleResolver,
+        INHERITED_DYNAMIC_FIELD_NAME_1,
+      );
+      const DynamicallyNamedFieldResolver2 = withDynamicallyNamedFieldResolver(
+        SampleObject,
+        DynamicallyNamedFieldResolver1,
+        INHERITED_DYNAMIC_FIELD_NAME_2,
+      );
+
+      const schemaInfo = await getSchemaInfo({
+        resolvers: [DynamicallyNamedFieldResolver2],
+      });
+      schemaIntrospection = schemaInfo.schemaIntrospection;
+      const sampleObjectType = schemaIntrospection.types.find(
+        type => type.name === "SampleObject",
+      ) as IntrospectionObjectType;
+
+      const dynamicField1 = sampleObjectType.fields.find(
+        field => field.name === INHERITED_DYNAMIC_FIELD_NAME_1,
+      )!;
+
+      const dynamicField2 = sampleObjectType.fields.find(
+        field => field.name === INHERITED_DYNAMIC_FIELD_NAME_2,
+      )!;
+
+      expect(dynamicField1).toBeDefined();
+      expect(dynamicField2).toBeDefined();
+    });
   });
 });