overblog / GraphQLBundle

This bundle provides tools to build a complete GraphQL API server in your Symfony App.
MIT License
787 stars 222 forks source link

Unable to split query configuration into several files #658

Open kix opened 4 years ago

kix commented 4 years ago
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Version/Branch 0.13.0

Overblog\GraphQLBundle\DependencyInjection\Compiler\ConfigParserPass->checkTypesDuplication() disallows splitting queries and mutations into several files.

Imagine I have quite a lot of queries and mutations. The thing that would quite help me manage that is splitting: queries and mutations for x are in types/x.yaml looking something like:

Query:
    type: object
    config:
        description: "Public queries"
        fields:
            listXs: ...
            showSingleX: ...

However I'm currently unable to do so since the parser pass does not merge the configs (and maybe check for conflicts there), but instead treats overlapping keys as conflicts.

I see #169 has been opened but looks like it has gotten irrelevant to its author.

What I also would like to add is that typically Symfony treats same keys in config files as mergeable. E.g. services_test.yaml does not conflict with services.yaml, it just gets merged.

mcg-web commented 4 years ago

You should use decorators to split your fields here is an example of how this can be done:

Mutation:
    type: object
    inherits: [ProjectMutations, StageMutations]
ProjectMutations:
    decorator: true
    config:
        fields:
            createProject:
#...
StageMutations:
    decorator: true
    config:
        fields:
            createStage:
#...

this will create a type Mutation with two fields createProject and createStage.

kix commented 4 years ago

Oh, wow, thanks. That's an option, although I don't quite see why isn't it easier just to enable merging schemas.

murtukov commented 4 years ago

@kix Why do you need to have multiple Query types?

kix commented 4 years ago

@murtukov, I don't need multiple Query types, I simply want to keep the branches of the query tree in different files, so that I don't have a single long Query.yaml.

murtukov commented 4 years ago

What result would you expect in this scenario, if this feature were implemented?

File Query.yaml:

Query:
    type: object
    config:
        fields:
            getPosts: ...

File QueryOne.yaml:

Query:
    type: object
    config:
        fields:
            getNews: ...
            getTags: ...

File QueryTwo.yaml:

Query:
    type: object
    config:
        fields:
            getNews: ...
            getPosts: ...
            getTags: ...

How would be defined the order of merging? Which getTags will we have at the end? From QueryOne.yaml or from QueryTwo.yaml?

What if QueryOne.yaml and QueryTwo.yaml have children, which are also separated in multiple files? And additionally they are using interfaces and decorators.

kix commented 4 years ago

If there is a conflict, throw an exception, show a notice. If there are no key conflicts, merge the trees.

murtukov commented 4 years ago

@kix Without replacing the keys it would be easy to implement.

murtukov commented 4 years ago

@kix I added a label too work on this feature later.

akomm commented 4 years ago

What do you do if the type is different, or builders are used. Do you merge after builders or before.

murtukov commented 4 years ago

@akomm ah, there are too many nuances. Maybe the best option is to decline this idea.

akomm commented 4 years ago

@murtukov I agree.

Its basically hitting the root issue that is experienced here. The expected behavior here is to narrow and based only on current requirements. The config goes into different components. Some used to generate graphql types, some used to generate validation constraints. And as in the linked issue, the merge behavior expected for validation is different, than for the graphql type merging. And if something new is added, it might generate another requirement.

By going from the inheritance merging to config merging, we move a bit up the arm in regards this merging issue. We make it even less controlled in that case and introduce more possibilities how merging might conflict and not work as expected.

kix commented 4 years ago

Well, using inherits seems to solve the issue, although there certainly should be some emphasis on this feature in the docs. Any decently sized API tends to have a lot of interactions, so splitting those into separate files makes a lot of sense.

For anyone looking for a way to do this, coming from Google perhaps, this is the working and correct way: https://github.com/overblog/GraphQLBundle/issues/658#issuecomment-594571793

Although I didn't in fact use the decorator option. Seems to work fine with just plain object declatarions. @mcg-web, why is it necessary, and is it?

akomm commented 4 years ago

@kix check your generated types. Without decorator option I think the inherited ProjectMutations, etc. will be created as own type, additionally to inheriting the fields later into another type.