search

nautilus / gateway

A federated api gateway for graphql services. https://gateway.nautilus.dev/
MIT License
397 stars 49 forks source link

Allow different type system directive locations, merge built-in directives #171

Closed JohnStarich closed 2 years ago

JohnStarich commented 2 years ago

@AlecAivazis I'm interested in your thoughts on this one, particularly on the merging strategy.

This PR removes a special case when merging built-in directives. Instead, it now attempts to merge directives together.

Directives can be used on execution locations (e.g. a query) or on type system locations (e.g. a deprecated field) (spec). We probably should not merge and share "execution locations" since these may not be supported by the respective service. However, we should merge and share "type system locations" since these should only be defined and used in each respective service's schema. In other words, the services own their usage of those directives.

Some implementations merge all locations irrespective of their kind. This could result in a runtime error or an ignored execution directive instead of an immediate query syntax error. Maybe that's ok after all – this PR leaves us some room to make a decision on flexible executable locations later on, if needed.

JohnStarich commented 2 years ago

Note to self: Switch from isExecutableDirectiveLocation() to isTypeSystemDirectiveLocation() for forward-compatibility.

AlecAivazis commented 2 years ago

I think you're right that it's a dangerous enough situation to warrant something loud. Disabling by default and providing a way to allow it when necessary sounds like a good balance 👍

JohnStarich commented 2 years ago

Thanks @AlecAivazis, much appreciated.

JohnStarich commented 2 years ago

Coveralls is busted temporarily, so merging despite the failed coverage publish & subsequent canceled tests. Tests are passing on all 3 versions as best I can tell.