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

Decorator allowing field metadata annotations #124

Closed
chrisdevereux opened this issue Aug 11, 2018 · 23 comments
Closed

Decorator allowing field metadata annotations #124

chrisdevereux opened this issue Aug 11, 2018 · 23 comments
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Solved ✔️ The issue has been solved
Milestone

Comments

@chrisdevereux
Copy link

chrisdevereux commented Aug 11, 2018

It is generally useful to be able to annotate fields with custom metadata for use by middleware and validators. See: #77 and #36 (my personal use case)

Since there currently isn't support for directives in GraphQL-js, an alternative might be for TypeGraphQL to add a new decorator that annotates the fiend config object passed into the GraphQLObjectType constructor with additional user-defined metadata fields.

I'd imagine the API looking something like this:

@ObjectType()
class MyObjectType {
  @Metadata({ isUseful: false })
  @Field()
  myAnnotatedField() {
    return "12"
  }
}

This relatively simple change would make it possible to write middleware libraries wrapping tools such as graphql-query-complexity.

Owner note:
Read more about how to implement here: #124 (comment)

@MichalLytek
Copy link
Owner

MichalLytek commented Aug 11, 2018

To access a data from decorator in middleware, you can create custom decorators that uses middlewares under the hood:

function Metadata(args: { isUseful: boolean}) {
  return UseMiddleware(({}, next) => {
	if (!args.isUseful) {
	  return null; // or other logic	
    }
    return next();
  });
}

If you need to access graphql-related metadata in the middleware, you might be interested in #123.

If you think about #36 and the need to emit some data into new ObjectType constructor, it's a separate thing. I think i it may be implemented only with the plugin API, as I need to call it on schema creation - it can't be done by a middleware.

@chrisdevereux
Copy link
Author

chrisdevereux commented Aug 12, 2018

Yeah, I'm thinking this would be applied at schema creation, then a global middleware would have access to the annotated schema and would be able to work it's magic.

My suggestion here is that @metadata would be a primitive exposed by type-graphql, which defines additional properties to be passed into a field at schema creation time.

What's the plugin API you're referring to? I can't see documentation or an open issue for it.

@MichalLytek
Copy link
Owner

So you also need graphql-js related metadata like GraphQLInputObjectTypeConfig data? For #36 you only need to inject the data into the config on build types time.

Sorry but I don't understand the "middleware would have access to the annotated schema" part 😕

@chrisdevereux
Copy link
Author

chrisdevereux commented Aug 12, 2018

If you think about #36 and the need to emit some data into new ObjectType constructor, it's a separate thing. I think i it may be implemented only with the plugin API, as I need to call it on schema creation - it can't be done by a middleware.

Yes, the proposed feature here is for an API to do this. How would you feel about integrating this feature in the core since there isn't a plugin API yet?

Sorry but I don't understand the "middleware would have access to the annotated schema" part 😕

It would have access to it via the info.schema property. But having checked the documentation, you're right to be confused by me talking about middleware in relation to #36 (for some reason I thought global middlewares were applied once to the whole query, rather than on every resolver 😕).

This would be used top-level validation rule, possibly applied by the graphql server before the schema is even evaluated.

@MichalLytek
Copy link
Owner

How would you feel about integrating this feature in the core since there isn't a plugin API yet?

For #36 I would have to add a field to decorator config, store it in MedatataStorage and manually place in ObjectType config object on schema generation.
But I don't like this tight coupling, I like the idea of microservices and I would prefer to go with multi packages monorepo. To do this, I need to develop a plugin API, so @typegraphql/query-complexity would be installed by user, registered in buildSchema plugins array and extend the @Field decorator with complexity settings.
So basically with plugin API you could be able to do the integration by yourself, e.g. for other graphql package not listed in roadmap here.

for some reason I thought global middlewares were applied once to the whole query, rather than on every resolver 😕

They are global global 😆 But basically, scoping type of global middlewares makes sense 🙄
I need to think how to declare the middleware type resolverOnly|fieldOnly|all.
For classes it might be a static prop:

export class GlobalQueryMiddleware implements MiddlewareInterface<Context> {
  static type = "resolverOnly";

  async use({ context, info }: ResolverData<Context>, next: NextFn) {
    return next();
  }
}

But for middleware functions it would have to be like the defaultProps for react function components:

export const GlobalQueryMiddleware: MiddlewareFn = async ({ info }, next) => {
  await next();
};
GlobalQueryMiddleware.type = "resolverOnly";

For now nobody complained about the global middlewares scoping, seems I should make it more flexible in the future 😉

@chrisdevereux
Copy link
Author

chrisdevereux commented Aug 12, 2018

But I don't like this tight coupling, I like the idea of microservices and I would prefer to go with multi packages monorepo. To do this, I need to develop a plugin API, so @typegraphql/query-complexity would be installed by user, registered in buildSchema plugins array and extend the @field decorator with complexity settings.

I absolutely agree with this, a plugin system is definitely the right way to go. But it's a big problem exposing a public GraphQL API in production without proper query complexity validation as it basically makes it trivial to DOS your server by crafting a malicious query.

So I'm wondering if a nice compromise between those two concerns would be for an API that:

  1. Allows TypeGraphQL to be used in production now (for me, it's the only blocker -- everything else is there)
  2. Is mostly forwards compatible with a future migration to a plugin-based architecture. Users can keep their declarative complexity annotations, then just change the decorator factory to call into the plugin API when the migration to a plugin architecture happens.

They are global global 😆 But basically, scoping type of global middlewares makes sense roll_eyes

That would be nice, but at least for #36, probably isn't required as I can just validate the query before evaluating it against the schema.

@chrisdevereux
Copy link
Author

Also, worth mentioning.... I'd be happy to have a go at implementing it.... assuming you were ok with the general approach 🙂

@MichalLytek
Copy link
Owner

Sure, it's quite easy do implement it:

  • add the complexity config property to @Field, @Query, etc. decorators options interface
  • -//- to the field, query, etc. metadata interface
  • in decorators put the complexity config into proper metadata configs
  • in schema generator, just put the config data in ObjectType and others constructors
  • you may need to extend graphql-js typings to make TS not to complain about graphql-query-complexity related fields

And remember to create a test cases for this feature and add info about it in docs.
Please stick to the commit message guidelines that I use (see commit history).

In the future I would extract it to the separate plugin package but the API will be the same. Users only need to install the package and register the plugin to make it works again.

@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community labels Aug 14, 2018
@chrisdevereux
Copy link
Author

Awesome. Thanks for the pointers, I'll take a look over the next few days.

@MichalLytek
Copy link
Owner

@chrisdevereux
#36 has been implemented, now TypeGraphQL supports query complexity analysis 🎉

@wesselvdv
Copy link

Is it possible to add custom metadata now? We're using join monster, and that requires us to add additional metadata to GraphQL objects.

@MichalLytek MichalLytek added Help Wanted 🆘 Extra attention is needed Good First Issue 👋 You can start contributing here Hacktoberfest labels Oct 4, 2018
@MichalLytek
Copy link
Owner

MichalLytek commented Oct 4, 2018

@wesselvdv
Not yet but I think it's easy to implement even by beginners 😉

API usage - @Metadata:

@ObjectType()
class MyObjectType {
  @Metadata({ isUseful: false })
  @Field()
  myAnnotatedField() {
    return "12"
  }
}

Steps to do:

  • create Metadata decorator that accepts object
  • modify field/query/mutation/subscription/input metadata interfaces in storage
  • create a method in metadata storage that will be called by Metadata to place the metadata object in storage
  • use that metadata object during schema construction in all object types, input, handlers, etc.:
fieldsMap[field.schemaName] = {
  // ...
  deprecationReason: field.deprecationReason,
  ...(field.metadata || {}),
};
  • of course test this feature well and create documentation for it 😉

@MichalLytek
Copy link
Owner

Currently this is blocked by graphql/graphql-js#1527 (comment). The GraphQL team will remove the current hackish workaround and introduce a special property in type config for placing custom metadata, so we have to wait for 14.2.0 release 😉

@MichalLytek MichalLytek added the Blocked 🚫 Resolving this issue is blocked by other issue or 3rd party stuffs label Jan 27, 2019
@MichalLytek MichalLytek removed the Help Wanted 🆘 Extra attention is needed label Jan 27, 2019
@teoxoy
Copy link

teoxoy commented Aug 20, 2019

@19majkel94 graphql/graphql-js#1527 has been implemented

@MichalLytek MichalLytek added Help Wanted 🆘 Extra attention is needed and removed Blocked 🚫 Resolving this issue is blocked by other issue or 3rd party stuffs labels Aug 20, 2019
@MichalLytek
Copy link
Owner

We should rename this decorator to @Extensions.

@palaparthi
Copy link

palaparthi commented Nov 12, 2019

@MichalLytek I would like to give this a go

@MichalLytek
Copy link
Owner

Already taken by my friend @VeryVicious, will let you know @palaparthi if something changes.

@glen-84
Copy link
Contributor

glen-84 commented Dec 28, 2019

Is @VeryVicious still working on this?

@MichalLytek
Copy link
Owner

@glen-84 I'm afraid not, you can take try to tackle it down if @palaparthi is not interested anymore 😉

@hihuz
Copy link
Contributor

hihuz commented Jan 8, 2020

Hello 👋

We are also very interested in this feature for our project 🙂If neither of them are interested anymore I can give it a try as well 🙂

@glen-84
Copy link
Contributor

glen-84 commented Jan 10, 2020

@hihuz,

Maybe extensions should be read-only (Readonly<Record<string, any>>)? I think it is in graphql-js.

@hihuz
Copy link
Contributor

hihuz commented Jan 10, 2020

Thank you for the suggestion @glen-84 🙂I agree, and have updated my branch to reflect it.

I have now opened a PR for review and I hope we can make some progress on it.

@MichalLytek
Copy link
Owner

MichalLytek commented Jan 29, 2020

Closed by #521 🔒

Released in v0.18.0-beta.10 🎉

@MichalLytek MichalLytek added Solved ✔️ The issue has been solved and removed Good First Issue 👋 You can start contributing here Help Wanted 🆘 Extra attention is needed labels Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Solved ✔️ The issue has been solved
Projects
None yet
Development

No branches or pull requests

7 participants