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

Using @Extensions and Object Inheritance problem #776

Closed
phal0r opened this issue Jan 31, 2021 · 9 comments
Closed

Using @Extensions and Object Inheritance problem #776

phal0r opened this issue Jan 31, 2021 · 9 comments
Assignees
Labels
Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community Solved ✔️ The issue has been solved
Milestone

Comments

@phal0r
Copy link

phal0r commented Jan 31, 2021

Describe the Bug
I use @Extensions annotation on field resolver to define custom tracing instructions as I only want to trace some field resolver executions. I added the annotation to several field resolvers and some are working as expected and some are failing.

I could identify a pattern, where the extensions are passed properly on fields, which are defined in the base class, but not on fields, that are defined in the inherited class.

To Reproduce

@ObjectType()
class BaseClass {
  @Field()
  baseField: number
}

@ObjectType()
class ExtendedClass extends BaseClass {
  @Field()
  extendedField: number
}

@Resolver((of) => ExtendedClass)
class Resolver {
  @Extensions({ trace: true })
  @FieldResolver((returns) => Int)
  async baseField() {
    ...
  }

  @Extensions({ trace: true })
  @FieldResolver((returns) => Int)
  async extendedField() {
    ...
  }
}

Expected Behavior
Accessing the Metadata in my plugin via

info.parentType.getFields()[info.fieldName].extensions?.trace

should return true for both field resolvers, but it only does for baseField

Logs
Not applicable

Environment (please complete the following information):

  • OS: node:14 base docker image
  • Node 14
  • Package version 1.1.1
  • TypeScript version 4.1.3

Additional Context

@MichalLytek
Copy link
Owner

MichalLytek commented Jan 31, 2021

@Resolver((of) => ExtendedClass)

have you tried @Resolver((of) => BaseClass)?

@MichalLytek MichalLytek added Need More Info 🤷‍♂️ Further information is requested Question ❔ Not future request, proposal or bug issue labels Jan 31, 2021
@phal0r
Copy link
Author

phal0r commented Jan 31, 2021

Thanks for the answer. If I switch to BaseClass, it says, that extendedField cannot be null (because the field is not being resolved at all).

@MichalLytek
Copy link
Owner

@Resolver((of) => BaseClass)
class BaseResolver {
  @Extensions({ trace: true })
  @FieldResolver((returns) => Int)
  async baseField() {
    ...
  }
}

@Resolver((of) => ExtendedClass)
class ExtendedResolver {
  @Extensions({ trace: true })
  @FieldResolver((returns) => Int)
  async extendedField() {
    ...
  }
}

Looks like you need to read all the docs again cause you still don't know what the @Resolver((of) => is about and how inheritance and interfaces works.

@phal0r
Copy link
Author

phal0r commented Feb 1, 2021

I do know how inheritance works and when to use it. Maybe a little bit more about my use case:

I use BaseClass to define some basic fields, that are used in 3 different ExtendedClasses. I have a resolver for each of the ExtendedClasses, where my field resolvers are defined. It is important to mention. that all fields like baseField have different field resolvers in the 3 different resolvers of the ExtendedClasses and this is intentional as they need to pass different business logic. Also I don't want to resolve BaseClass at all as it is not used in the schema. It is just to have the common field of the 3 ExtendedClasses in one place. Resolving those field works without any problem since the beginning.

I took the example for extending classes with ObjectType annotation from the docs here: https://typegraphql.com/docs/inheritance.html

I thought this is the way to define the base fields in a reusable manner. So, resolving works properly, only when I introduced extension params via @Extensions, I could see from the output, that only baseField extensions are set properly. Since you are the author of the project, you definitely have more insights about why it might fail and if you think, this is the wrong approach to define reusable properties, I would take gladly any advice on how to circumvent this behaviour.

The only other way to approach this use case, could be https://typegraphql.com/docs/interfaces.html. Is this what you would suggest?

@MichalLytek
Copy link
Owner

MichalLytek commented Feb 1, 2021

I still don't understand your use case neither what you're trying to achieve.

If you define extensions on child class field resolver, it won't automagically register for base class field.

You can put @Extensions({ trace: true }) on the base @Field field, no need to use @FieldResolver.

@phal0r
Copy link
Author

phal0r commented Feb 1, 2021

I still don't understand your use case neither what you're trying to achieve.

Ok, let's make it more specific. I have 3 user models, which I want to use in my schema: ActiveUser, User and AdminUser. They are defined (simplified) like this:

@ObjectType()
class BaseUser {
  @Field()
  pictureCount: number
  @Field()
  pictureCount: Picture
}

@ObjectType()
class User extends BaseUser {
  @Field()
  isFavorite: boolean
}

@ObjectType()
class ActiveUser extends BaseUser {
  @Field()
  isOnboarding: boolean
}

@ObjectType()
class AdminUser extends BaseUser {
  @Field()
  lastLogin: number
}

BaseUser should just define the common properties of the 3 other classes, but is not part of the schema itself and does not have a corresponding resolver.

Then I have created the resolvers for AdminUser, User and ActiveUser. Simplified example for User:

@Resolver((of) => User)
class UserResolver {
  @Extensions({ trace: true })
  @FieldResolver((returns) => Int)
  async pictureCount() {
    //fetch pictureCount from DB
  }

  @Extensions({ trace: true })
  @FieldResolver((returns) => Int)
  async isFavorite() {
    // fetch favorite relation from DB
  }
}

I want to control, which field resolver executions are being traced and which are not for AdminUser, User and ActiveUser. I want to be able to trace the execution of User.pictureCount, but to disable tracing for ActiveUser.pictureCount. This is why I added the information to the field resolver, which seems to be the correct place to do so, also it increases readability as I mostly only want to trace async field resolvers, which is easy to spot like so.

If you define extensions on child class field resolver, it won't automagically register for base class field.

Actually it works for the inherited fields, but not for the fields defined in the child class. In this example, the trace flag is correctly applied to the metainformation of pictureCount, but not to isFavorite.

You can put @extensions({ trace: true }) on the base @field field, no need to use @FieldResolver.

I want to be able to make the decision for each child class seperately, this is why I don't want to define it on the base class, also it increases readability, as I mostly only want to trace async field resolvers.

Does it make more sense now?

@MichalLytek
Copy link
Owner

MichalLytek commented Feb 1, 2021

Ok, now it makes sense and now I get it. I was able to reproduce the issue:

describe.only("Fields with field resolvers", () => {
  beforeAll(async () => {
    getMetadataStorage().clear();

    @ObjectType()
    @Extensions({ parentClass: true })
    class Parent {
      @Field()
      @Extensions({ parentField: true })
      parentField!: string;
    }
    @Extensions({ childClass: true })
    @ObjectType()
    class Child extends Parent {
      @Field()
      @Extensions({ childField: true })
      childField!: string;
    }
    @Resolver(of => Child)
    class ChildResolver {
      @Query()
      sampleQuery(): Child {
        return {} as Child;
      }

      @Extensions({ childFieldResolver: true })
      @FieldResolver()
      childField(): string {
        return "childField";
      }
    }

    schema = await buildSchema({
      resolvers: [ChildResolver],
      orphanedTypes: [Parent],
    });
  });

  it("should merge field level with field resolver level extensions", () => {
    const childObjectType = schema.getType("Child") as GraphQLObjectType;

    expect(childObjectType.getFields().childField.extensions).toEqual({
      childField: true,
      childFieldResolver: true,
    });
  });
});

Thanks for the report, gonna fix that soon 😉

@MichalLytek MichalLytek added Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community and removed Need More Info 🤷‍♂️ Further information is requested Question ❔ Not future request, proposal or bug issue labels Feb 1, 2021
@MichalLytek MichalLytek added this to the 1.x versions milestone Feb 1, 2021
@MichalLytek MichalLytek self-assigned this Feb 1, 2021
@MichalLytek
Copy link
Owner

Fixed by 73736e2 🔒

@MichalLytek MichalLytek added the Solved ✔️ The issue has been solved label Feb 1, 2021
@phal0r
Copy link
Author

phal0r commented Feb 1, 2021

Awesome! Thanks and btw. this is an awesome project, I really like to use it in my projects and appreciate your effort.

@MichalLytek MichalLytek modified the milestones: 1.x versions, 1.2 Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community Solved ✔️ The issue has been solved
Projects
None yet
Development

No branches or pull requests

2 participants