Tartiflette 0.12 compatibility

[florimondmanca]

Problem description

Note: tartiflette-starlette is currently pinned to tartiflette<0.11.

I noticed that Tartiflette 0.11 was out, and there are a few changes.

The major one I see is that engine creation is now asynchronous and must be done via create_engine().

This means that the current engine parameter to TartifletteApp() isn't appropriate anymore, because we can't create a raw Engine anymore (at least not in a sync context which is typically where the app is registered).

Alternatives

First, options previously delegated to Engine must now be handled by TartifletteApp. This can be done in two ways:

1. Create new keyword parameters on TartifletteApp (similar to what tartiflette-aiohttp currently does things), store them and then pass them to create_engine().
2. Provide an Engine helper and pass its kwargs down to create_engine().

Supported keyword arguments should be at least sdl, schema_name and modules (IMO). Supporting all those supported by create_engine() would be best.

I personally prefer 2) because it keeps things organized and is more explicit/IDE-friendly.

Next, we're going to have to defer engine creation/"cooking" to when the app starts up. This can be implemented with Starlette's Lifespan helper.

Additional context

• 0.11 (Changelog)
• https://tartiflette.io/docs/api/engine

[florimondmanca]

Hi @abusi, can I ask why it is now impossible to create an engine synchronously — or rather, what makes creating an engine asynchronously necessary? Thanks!

Edit: okay, I think I've found the answer in the engine docs, actually.

My question is then, why do we need to pass parameters to .cook() instead of passing them to the Engine constructor? I think it would be possible to do it this way, and then use self.xxx inside .cook() to build the engine. (It would also be more intuitive to me — define the Engine with all the ingredients it needs, and then just .cook() it).

This would allow this package (and others?) to reuse the Engine API instead of having to re-implement the .cook() interface at the constructor level. For example, to add 0.11 support with parameters at the Engine level, I'd just have to call .cook() in an app startup event handler.

[abusi]

Hi @florimondmanca I was in the process of making starlette compatible with 0.11 by passing a partial as a startup hanlder and then do the create_engine in it.

We removed the parameters from the engine cause they are not needed by the engine itself but by subcomponent, it was bothering me to add things to self that weren't usefull for self. They are all transitive parameters that's why I moved them to the .cook method.

I understand your frustration here and I didn't think it would be such a big deal.
In the future we'll try to discuss these kind of thing with you before doing them. That's also why we are not in v1.0.0 it's because things are moving fast.

I'll propose a PR to you with tartiflette 0.11 compatibility, I didn't had the time to finish it, but I'll do it as soon as I can (in the coming days).

[florimondmanca]

Thanks for your answer!

That's also why we are not in v1.0.0 it's because things are moving fast.

Yup, and you're 100% right!

I understand your frustration here and I didn't think it would be such a big deal.

I'm not angry at all about the change, just trying to understand the decision. :-) Sorry if it came across as rough!

In the future we'll try to discuss these kind of thing with you before doing them.

Thanks! Just a heads up in the associated Tartiflette issue/PR would be enough, I wouldn't want to slow things down.

I was in the process of making starlette compatible with 0.11 by passing a partial as a startup hanlder and then do the create_engine in it.

Sounds good! FYI, I pushed a complete overhaul of the internals of TartifletteApp yesterday, which allow us to leverage Starlette's components much more easily.

Here's my idea for an updated TartifletteApp:

from starlette.routing import Lifespan

class TartifletteApp:
    def __init__(
        self,
        sdl: str,
        *,
        graphiql: typing.Union[bool, GraphiQL] = True,
        path: str = "/",
        schema_name: str = "default",
        modules: typing.List = None,
    ):
        if graphiql is True:
            graphiql = GraphiQL()

        routes = []

        if graphiql and graphiql.path is not None:
            routes.append(Route(path=graphiql.path, endpoint=GraphiQLEndpoint))

        graphql_route = Route(path=path, endpoint=GraphQLEndpoint)
        routes.append(graphql_route)

        self.engine = Engine()

        self.app = Lifespan(GraphQLMiddleware(
            Router(routes=routes),
            engine=self.engine,
            graphiql=graphiql,
            graphql_path=graphql_route.path,
        ))

        @self.app.on_event("startup")
        async def prepare_engine():
            await self.engine.cook(
                sdl=sdl, modules=modules, schema_name=schema_name
            )

    async def __call__(self, scope: Scope, receive: Receive, send: Send):
        await self.app(scope, receive, send)

[florimondmanca]

By the way, note that the sdl, schema_name, and modules parameters are just used by .cook().

If we were able to pass an Engine with those parameters stored in its self, it would lead to more decoupled code because tartiflette-starlette wouldn't need to care about the cook interface at all — the Engine would.

Here's how it could simplify TartifletteApp.__init__():

    def __init__(
        self,
        engine: Engine
        *,
        graphiql: typing.Union[bool, GraphiQL] = True,
        path: str = "/",
    ):
        if graphiql is True:
            graphiql = GraphiQL()

        routes = []

        if graphiql and graphiql.path is not None:
            routes.append(Route(path=graphiql.path, endpoint=GraphiQLEndpoint))

        graphql_route = Route(path=path, endpoint=GraphQLEndpoint)
        routes.append(graphql_route)

        self.engine = engine

        self.app = Lifespan(GraphQLMiddleware(
            Router(routes=routes),
            engine=self.engine,
            graphiql=graphiql,
            graphql_path=graphql_route.path,
        ))

        self.app.add_event_handler("startup", self.engine.cook)

I insist because while I can see how storing data in self is not directly useful to the Engine, it would turn the Engine into an object that can be passed around, ready for cooking by third-party clients such as tartiflette-starlette. Starlette uses this compositional approach a lot (components rely on ASGI as their primary interface), and I think it makes the API extremely pleasant and explicit IMO.

Is it too late to consider a switch?

[abusi]

You're making good points with this, let me think about it a little bit more. I'll get back to you during the day.

[abusi]

tartiflette/tartiflette#244 @florimondmanca You were right, it gives us more flexibility.
Thank you for your feedback.

[abusi]

@florimondmanca Do you still need help with the 0.11.x migration ? I don't have time to finish my work on it, I see that you already have ideas for it. If you think it can wait a bit, i'll do it next week.

[florimondmanca]

I think I can take it over. Feel free to push your work to a branch and I'll be able to take it from there this week/this weekend. :-) Thanks!