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

print_schema doesn't support user-defined directives #133

Closed
barbieri opened this issue May 25, 2021 · 7 comments
Closed

print_schema doesn't support user-defined directives #133

barbieri opened this issue May 25, 2021 · 7 comments

Comments

@barbieri
Copy link

https://github.com/graphql-python/graphql-core/blob/main/src/graphql/utilities/print_schema.py#L208 is made in a way that just supports @deprecated, however fields may get other directives, such as validation, permissions or display hints.

This is also the case with print_input_value that only handles deprecated directive.

@Cito
Copy link
Member

Cito commented May 25, 2021

print_schema should also support printing custom directives, see this test.

@barbieri
Copy link
Author

it does print the directive declaration like you pointed, but doesn't handle the usage ("on locations"). Such as types, fields, arguments, input fields...

I saw another bug like this one closed because "you just use this in your resolver", saying that the actual validation/permission checks and all should be done in field resolver -- which I totally agree. But the issue is if you parse or generate the GraphQL types and want to publish your schema by dumping it with print_schema, it won't work.

@Cito
Copy link
Member

Cito commented May 25, 2021

Are you talking about issue #90 (which is not a "bug")?

@barbieri
Copy link
Author

barbieri commented May 25, 2021

yes, exactly. You can call it "not a bug", but not a feature either :-D

Any reasons why this directive serialization is not a standard visitor with serialization? I understand in some cases you may want custom behavior, such as omitting the deprecated reason if it's the default reason, but at least a "fallback" to handle other cases:

# pseudo code:
serializer = directive_serializers.get(directive.name, default_directive_serializer) # you can provide one for deprecated here
serializer(directive)

def default_directive_serializer(directive):
    s = f'@{directive.name}'
    if directive.arguments:
        s += '(' + ', '.join(f'{a.name}: {serialize_value(a.value)}' for a in directive.arguments) + ')'
   return s

I'm not familiar with all the printers already there, eventually this inner loop for arguments already exist because of the field arguments, just need to take care to serialize the quotes properly.

@Cito
Copy link
Member

Cito commented May 26, 2021

I think I explained it in #90. The declared goal and scope of this project is to be a 1:1 port of GraphQL.js and its API to Python, even down to the level of implementation details and tests, and to keep it in sync with the latest development in GraphQL.js, where they are continually moving forward: fixing bugs, adding functionality and keeping in sync with changes and additions to the GraphQL spec.

I am so adamantly sticking to this scope and trying to not extend it, because it's the only modus operandi that allows me to maintain this project in my limited spare time (I'm not paid for this work) and to keep it up to date and in sync with the functionality of GraphQL.js in the long run. The more GraphQL-core deviates from GraphQL.js, the more difficult it will become to stay up to date. I have already seen this with GraphQL-core 2, which added functionality, but at one point in time stopped keeping in sync with GraphQL.js (I think it was at version 0.6.0) because they already deviated too much, after which it fell behind and quickly became outdated. That's why I created GraphQL-core 3 with the declared goal of just being a port of GraphQL.js, not more and not less. That's also why a "bug" in the context of this project must be measured only against the goal of this project being a functional port of GraphQL.js or not.

If some functionality of GraphQL.js is missing or considered a bug, the way to proceed is to submit an issue on the GraphQL.js issue tracker and convince the GraphQL.js folks to add the functionality or to fix the bug. They have more competent people and resources, and more expertise to decide whether and how some functionality or API should be changed, and they are very approachable and usually eager and quick to fix bugs. Then, after it is implemented in GraphQL.js, it will be also ported to GraphQL-core, and everyone benefits, both the users of GraphQL-core and the users of GraphQL.js. Another way to add functionality would be to create a supplementary library or tool that uses GraphQL-core. The reason this library is called "-core" is because it provides just the core functionality that other libs (like Graphene) can build upon. A third option would be to fork or create a new library that has a "higher goal" than GraphQL-core, and would include more functionality, and maybe offer a more Pythonic API or better performance due to an implementation that is more optimized for how Python works. I am totally not against this. But inside this project, I want to stick to the given scope.

I'm sorry if this is not the response you had hoped for, but I hope you understand my reasoning.

@barbieri
Copy link
Author

@Cito ok, I even checked GraphQL.js to see if that was really the case but yeah https://github.com/graphql/graphql-js/blob/c589c3d285cb1ec44b09bf0b50ec041ec083760c/src/utilities/printSchema.ts

I see your point, specially regarding time, I also maintain https://github.com/profusion/sgqlc and it's hard to keep up with the demand :-)

I'll fill a bug in there, meanwhile we keep going with a monkey patch print_fields() in our project. There is no need to fork the whole project because of a visitor, even if we couldn't monkey patch, it would be possible to just implement this visitor manually.

@barbieri
Copy link
Author

And I saw couple of tickets, one still open, that would address that: graphql/graphql-js#2020

If that ever gets into the GraphQL.JS, I'll ping here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants