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 future API #32

Open
wants to merge 63 commits into
base: main
Choose a base branch
from
Open

Add future API #32

wants to merge 63 commits into from

Conversation

rafalp
Copy link
Contributor

@rafalp rafalp commented Nov 21, 2023

This PR adds next package to ariadne_graphql_modules containing future API that supports both schema first and code first approaches to development

TODO

GraphQL features

  • enum
  • input
  • interface
  • scalar
  • type
  • union
  • Subscription

@DamianCzajkowski
Copy link

Hi @rafalp,

Could you please take a look at this PR? I’d love to hear your thoughts on it.

I’ve made some changes to the code structure to improve the deployment process. Inspired by Pydantic, I’ve moved the old version into a v1 folder and made the v2 version the official one.

Additionally, I’ve introduced the ability to inherit interface objects using (object, interface). This idea comes from the Mirumee team, allowing us to avoid code duplication when defining the schema. Here’s an example:

    class BaseEntityInterface(GraphQLInterface):
        id: GraphQLID

    class UserInterface(BaseEntityInterface):
        username: str

    class UserType(GraphQLObject, UserInterface):
        name: str

    class SuperUserType(UserType):
        is_super_user: bool

This will generate the following GraphQL schema:

        interface UserInterface implements BaseEntityInterface {
         id: ID!
         username: String!
       }

       interface BaseEntityInterface {
         id: ID!
       }

       type User implements BaseEntityInterface & UserInterface {
         id: ID!
         username: String!
         name: String!
       }

       type SuperUser implements BaseEntityInterface & UserInterface {
         id: ID!
         username: String!
         name: String!
         isSuperUser: Boolean!
       }

runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
python-version: ["3.8", "3.9", "3.10", "3.11"]
python-version: ["3.9", "3.10", "3.11", "3.12"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ariadne in it's readme states 3.7+, should this follow? Even if you are dropping 3.8 here it should be loudly stated in the release notes.

Copy link
Contributor Author

@rafalp rafalp Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ariadne GraphQL Modules never supported 3.7 due to difference in bound methods handling in Python itself that landed in py3.8.

Nevermind. That change happened in Py 3.7 minor release. And we've had two different lists of supported versions, one in pyproject.toml and other in setup.py. One going as far as Py 3.6 and other for Py 3.7. I think its safe to drop 3.7 and 3.8 both here and in other Ariadne libs, but make it first item in release notes.

@DamianCzajkowski
Copy link

I know 301 changed files looks intimidating, but most of these are snapshot files (snapshots/*.yml). These are automatically generated by pytest-regression to store test outputs for regression testing. Since they’re auto-generated and just for verifying code correctness, there’s no need to review them closely.

As for the V1 module, it’s the same as before with only minor type-related tweaks to ensure backward compatibility with V2. The functionality hasn’t changed; it’s just been relocated in the project structure.

Examples of how to use the API will be included in the documentation that's currently being developed. For now, you can find usage examples in the tests.

@rafalp
Copy link
Contributor Author

rafalp commented Aug 26, 2024

My only worry is that v1 module change will be a nasty surprise to everyone who just updates GraphQL Modules to new version and expects it to work.

@DamianCzajkowski
Copy link

My only worry is that v1 module change will be a nasty surprise to everyone who just updates GraphQL Modules to new version and expects it to work.

I understand your concern, but the good news is that if I properly highlight this change in the release notes, the transition shouldn't be too much of an issue. It's actually a better solution than having everyone manually remove .next from their imports after maintaining the old version for a year. This way, we can ensure a smoother upgrade process for everyone.

@DamianCzajkowski DamianCzajkowski marked this pull request as ready for review September 9, 2024 09:51
@rafalp
Copy link
Contributor Author

rafalp commented Oct 10, 2024

I've been working on and off and on again using code from this PR to implement a toy GraphQL API and I've been liking it. I am impressed that my code validated in MyPy and Pylint (with caveats), and correctness checks implemented are quite tight. Being able to work with GraphQLInput instance in resolver that has type checking is chef's kiss.

The repo is available here: https://github.com/rafalp/ariadne-graphql-modules-v2-example/

In readme I've left some notes, but I'll copy-paste them here:

Problems found

list[SchemaType] type in make_executable_schema

list is invariant in mypy. A list of type[GraphQLObject], like one from the example.queries.__init__ produces an error because list[type[GraphQLObject]] is incompatible with list[type[GraphQLType]].

Replacing list with Iterable may be enough to fix this.

Resolvers need to be decorated with @classmethod or @staticmethod to keep linters happy

Linters will scream that resolver method decorated with @ObectType.resolver is missing self first attribute.

This may require mypy plugin to fix, but its good idea to look how Graphene and Strawberry are solving this.

Missing dedicated Mutation type

We could have a mutation type like this:

class ConcatMutation(GraphQLMutation):
    __graphql_name__: str = "concat"

    def mutate(info: GraphQLResolveInfo, *, arg1: str, arg2: str) -> str:
        return arg1 + arg2

This would be more intuitive way to define mutations than current approach of having MutationType(GraphQLObject) with multiple methods.

Object type docs don't document interface usage for objects with schema

According to interface docs, this is valid:

class PostType(GraphQLObject, SearchResultInterface):
    __schema__ = gql(
        """
        type Post {
            id: ID!
            content: String!
            category: Category
            poster: User
        }
        """
    )

But it will fail to validate with following error:

ValueError: Class 'PostType' defines '__schema__' attribute with declaration for an invalid GraphQL type. ('ObjectTypeDefinitionNode' != 'InterfaceTypeDefinitionNode')

This points to SearchResultInterface validation logic overriding GraphQLObject.

Docs also need to be updated to show example for GraphQLObject with interface.

Interface's docs are also mentioning subscription in its parts which needs to be edited out.

Interface's docs should also have example of usage with GraphQLObject, even if only as "See the GraphQLObject docs for usage example." link somewhere in it.

Interfaces need to be explicitly passed to make_executable_schema

Given GraphQL object definition:

class PostType(GraphQLObject, SearchResultInterface):
    ...

make_executable_schema will fail with:

TypeError: Unknown type 'SearchResultInterface'.

GraphQLObject's model creation could see if parent types include subclasses of GraphQLInterface and include them.

GraphQLUnion docs should have example of union type implementing resolve_type

We do this already for GraphQLInterface, it would help if GraphQLUnion also did it.

Other take aways

Documentation is important. Somebody technical should proof-read it to verify that things make sense and code examples are correct. This should be done before release (but shouldn't block merge IMO).

Other ideas for future improvements:

I am not sure if idea of "argument": GraphQLObject.argument(graphql_type=SomeScalar) will work long term, but until we come with better idea, its okay to ship this.

Maybe we could define Python type on GraphQLScalar? Eg. setting __python_type__ = datetime on the DateTimeScalar(GraphQLScalar) would automatically use it for all args and fields that are of type datetime. Maybe it should be a dict with mapping passed to make_executable_schema, eg.: global_scalars={"datetime": datetime}.

Maybe we could also have a type validation for args and resolvers using scalars to see if their types match scalar's serialize and parse return values?

Let resolvers for fields return GraphQLObject instance (like we already do for GraphQLInput) as representation of an object? This approach kinda breaks the flow where data is resolved from Python obj. when its needed, but it would be an escape hatch for those who need this extra type checking safety while returning data from resolvers.

I also have review underway for this PR but I'll likely only finish it tomorrow or over the weekend.

Copy link
Contributor Author

@rafalp rafalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some notes, but nothing blocking.

Also, absolute imports? Rest of Ariadne's ecosystem uses relative ones. You'll do what you want there, but I've always preferred relatives because renaming package doesn't change its contents and relative imports reveal oddities in the structure unlike absolute ones, forcing mostly flat and wide layout of code, which IMO is better from having deep structures. And, in flat structure those imports are shorter.

I don't know if docs were formatted with some sort of markdown formatter, but it removed double blank line from before # headers, removing visual separation from those section when reading MD and making them hard to read for me.

if name:
return name

name_mappings = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name doesn't describe its use, perhaps replace_suffixes would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every of types on this list should also have it's documentation updated to explain how its GraphQL name is created from Python type's name if __graphql_name__ is not defined.

field_args: dict[str, GraphQLObjectFieldArg] = {}
field_args_start = 0

# Fist pass: (arg, *_, something, something) or (arg, *, something, something):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Fist pass: (arg, *_, something, something) or (arg, *, something, something):
# Find index of first GraphQL arg

field_args: dict[str, GraphQLObjectFieldArg] = {}
field_args_start = 0

# Fist pass: (arg, *_, something, something) or (arg, *, something, something):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Fist pass: (arg, *_, something, something) or (arg, *, something, something):
# Find index of first GraphQL arg

raise ValueError(f"Unsupported base_type {cls.__base_type__}")

@classmethod
def construct_object_model(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why construct_ instead of get or create?


@classmethod
def construct_object_model(
cls, base_type: type[Union[ObjectType, MutationType]]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base_type -> legacy_type

@@ -0,0 +1,13 @@
def convert_python_name_to_graphql(python_name: str) -> str:
components = python_name.split("_")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

components -> words

Create a DeferredTypeData object from a given module path.

If the module path is relative (starts with '.'),
resolve it based on the caller's package context.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resolve it based on the caller's package context.
resolve it based on the caller's package context.
Use this util together with 'typing.Annotate':
field: Annotate["LazyType", deferred(".lazytype")]

)

if not type_node:
raise ValueError(f"Can't create a GraphQL return type for '{type_hint}'.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In testing I've found that those errors can be very unspecific. get_type_node could accept additional kwarg like location: str that would go to this error, so we could make it "Can't create a GraphQL return type for type_hint in SomethingSomethingType".


### `resolver`

Defines a resolver for a subscription. The resolver processes the data provided by the source before sending it to the client.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parts of this doc were copied from subscription doc but weren't updated after that.

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

Successfully merging this pull request may close these issues.

4 participants