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

Add schema visibility fillters #361

Closed
Ambro17 opened this issue Jun 29, 2020 · 12 comments
Closed

Add schema visibility fillters #361

Ambro17 opened this issue Jun 29, 2020 · 12 comments
Labels
discussion enhancement New feature or request

Comments

@Ambro17
Copy link
Contributor

Ambro17 commented Jun 29, 2020

Ruby has its way of hiding fields
https://graphql-ruby.org/authorization/visibility.html

Java has its way too
https://www.graphql-java.com/documentation/v12/fieldvisibility/

Javascript too
https://www.apollographql.com/docs/apollo-server/features/schema-transforms/

The main goal of this is to hide experimental or in progress types from clients, but to allow to include them for testing or staging for example

There seems to be two valid approaches for visibility filters.

  • Per request filters based on user authorization and other context information. This should account for filtering types from introspection too
  • Per schema filters that build a different schema based on rules upon types registering that don't change after the schema is built.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@patrick91 patrick91 added enhancement New feature or request blocked labels Jun 29, 2020
@patrick91
Copy link
Member

I'll mark this as blocked as I'm doing a rewrite on the internals :)

Should be easier to implement this after that 🤞

@patrick91 patrick91 removed the blocked label Aug 10, 2020
@patrick91
Copy link
Member

@Ambro17 I'm wondering if we should go with something similar to our permission:

import strawberry


class IsPaidAPIUser(BaseVisiblity):
    message = "User is not authenticated"

    def has_permission(self, source, info):
        return False


@strawberry.type
class User:
    name: str
    age: int


@strawberry.type
class Query:
    @strawberry.field(visibility_classes=[IsPaidAPIUser])
    def user(self, info) -> User:
        return User(name="Patrick", age=100)


schema = strawberry.Schema(query=Query)

not really sure about the names to be honest

@Ambro17
Copy link
Contributor Author

Ambro17 commented Aug 10, 2020

Didn't know about strawberry's permissions support!
That would certainly be enough for the use case i was thinking when i opened the issue, i would keep the issue open until we get documentation on usage. Perhaps not only showing its usage as a permission based filter, but as a tool to maintain public and private parts of the schema

@patrick91
Copy link
Member

Yeah, visibility filters would be to hide the fields based on the clients that are requesting the schema.

Permissions don't do anything around that, they only check if you can access a field or not ;)

@Ambro17
Copy link
Contributor Author

Ambro17 commented Aug 10, 2020

Oh i see, they are not read on schema build time then. I guess we would have to offer a similar api then to avoid including them on the schema

@patrick91
Copy link
Member

Yes, but I think everything will happen during the request, as that's the only time when you have access to information about the clients (headers, etc).

Not sure yet how this could be implemented, as I need to check the internals of GraphQL-core, but I think it might be a nice feature :)

@Ambro17
Copy link
Contributor Author

Ambro17 commented Aug 10, 2020

I think we are considering two different use cases of schema visibility.

  1. I'm thinking about filtering schema types on build time based on for example on environment variables.
  2. You are proposing a more flexible approach, to control schema introspection on query time, and allowing new fields if a certain authorization header is passed for example.

I think both use cases would benefit from visibility filters, but i find the second case much harder to implement, at least from what i know now. Because one would not only have to delete the type, but also all references to the type across the graph. Perhaps that's trivial on simple cases, but on deeply nested graphs or with a core entity, that may find rough edge cases.

@patrick91
Copy link
Member

Uhm good point on deleting the types, I was only thinking about hiding the fields at the moment. Will think about this too

@Ambro17
Copy link
Contributor Author

Ambro17 commented Sep 6, 2020

I think this issue is a nice read graphql/graphql-js#113 to have a mental model of the problem and the tradeoff of each approach.
Basically, it seems dynamic visibility filters will never reach graphql as they would increase complexity in testing all possible combinations of the schema. The client would also suffer by having a different schema on different requests so the nice guarantees of typing would be less benefitial.
After thinking about this a bit more deeply, i think that schemaTransforms from graphql-tools offers the best tradeoff.
You still get a different schema based on the rules you define, but they end being static so you avoid ending with N schemas, each with slight differences for which each client must account for.

On the other hand, i'm thinking perhaps this transformation is better suited as an external lib than inside strawberry as it would need to be tightly coupled with graphql-core and AFAIR you made efforts Pattrick to make an eventual replacement of graphql-core easier.
Although i don't know strawberry's codebase enough to conclude if there's a clean way to avoid registering fields for each type..

@patrick91
Copy link
Member

Although i don't know strawberry's codebase enough to conclude if there's a clean way to avoid registering fields for each type..

I don't think there's an easy for this, I'll need to try and see if I can come up with something clever for it.

Basically, it seems dynamic visibility filters will never reach graphql as they would increase complexity in testing all possible combinations of the schema. The client would also suffer by having a different schema on different requests so the nice guarantees of typing would be less benefitial.

Yeah, this makes sense. I guess if you only have two variants it might be ok (internal and external). Maybe I'll try to find someone that has done this in the past and see how they're doing 😊

@Ambro17
Copy link
Contributor Author

Ambro17 commented Oct 11, 2020

I have a new idea that i will test tomorrow to see if we can hide them dynamically

@patrick91
Copy link
Member

@Ambro17 I'll close this in favour of #3343, hope that's ok 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants