RFC: Officially recommend against using GraphQL Stitching in Gravity about readme HOT 19 CLOSED

@kajatiger @mzikherman it's very clear though that the average developer just doesn't know what's best for the overall schema:

• The pervasiveness of nullable fields
• The inability to implement pagination correctly/completely
• Not knowing what objects to return in mutations
• Not knowing when to implement common interfaces (dates, images, markdown)

from readme.

Closing this RFC as accepted! The support seems fairly enthusiastic.

As a follow-up I'll open another RFC to provide stronger guidance around stitching in general, across all of our systems. While there isn't much we can do about those services that are already stitched in, providing clear guidance around future systems will be useful. In short: Avoid GraphQL stitching unless service has already been stitched in and lacks a REST layer. Once that is accepted I will update docs and corresponding playbooks.

from readme.

FWIW, here's a recent stitched-mutation PR between Exchange and Gravity: artsy/metaphysics#3968. Note how nothing more is needed, other than work defined in Exchange.

from readme.

I think it's important to recognize that this RFC is to recommend against adding to Gravity's GraphQL endpoint. There may still be exceptions, and I feel that's totally reasonable if the dev(s) making them are aware of the tradeoffs and have a reason for choosing GraphQL over REST.

There's a real advantage to having a clear recommendation: less mental overhead, less time spent wondering "does this make more sense in GraphQL or REST?" Instead, it's REST unless a tech plan or conversation surfaces a really good reason it should happen in GraphQL.

I agree with a lot of the points made in this thread. We have less GraphQL experience, and I think more importantly less enthusiasm for complex GraphQL work, than we have in the past when people like Eloy, Justin, and Orta were on the team. That's not to say we're not capable of solving GraphQL problems or reinvesting in our GQL infrastructure, but more that doing so would require us to either hire people with deep GQL experience, or for existing Artsy engineers to want to build that level of experience.

IMO there are three big things to consider with a change like this:

• Performance
  • AFAIK there's not a large/obvious benefit to either side (GQL/REST)
• Dev experience
  • This is core to this RFC. Chris' argument is that it's a better dev experience to use REST with dataloaders instead of GQL, and after reading and reflecting, I agree
• Number of different tools/patterns to maintain
  • Also core to this RFC. While we're not necessarily talking about having less tools/patterns overall, we are talking about simplifying and clarifying what people should do when faced with a choice between GQL and REST in Gravity, so effectively cutting down on our total maintenance burden (mentally, at least)

Thanks for starting this conversation @damassi!

from readme.

Thank you for opening this RFC @damassi !

I agree with the recommendation, mostly because in my experience, stitching works until it doesn't (which can be unpredictable!). And when it doesn't, it can sink tons of developer time and morale (for all the reasons stated above).

Quick aside

I also agree with @joeyAghion that perhaps we can make the recommendation stronger. Reason being: if possible, having a single recommendation across our codebases reduces the mental overhead even further, which would be awesome!

Here's my understanding/potential recommendation:

• Clients prefer to consume (and mutate) data using GraphQL, via Metaphysics
  • Assume "client" here means an app that is primarily responsible for providing a view on data to end-users. Includes things like Force, Eigen, Forque, Volt, and Pulse.
  • I assume (?) this is non-controversial, but I'm not sure if it's written down anywhere.
• Metaphysics prefers to fetch data from backends via REST, but it can also be stitched in, in special cases. Metaphysics should not fetch data from backends using GraphQL but no stitching.
  • Assume the special cases are of course the stitching we've already implemented.
  • Assume "special cases" are cases where we are already stitching, or if it's straightforward (<- could be expanded upon) and we're not encountering anything new (<- dangerous, but I could see someone making the point).
  • The second point, about not fetching data via GraphQL without stitching is a reference to the "fake stitching" we did when we first built Exchange. See the PR that removed all this code here, and a slack thread that discusses the brittleness of the setup.

I think this roughly matches what's above, but the point I want to make here: Gravity is an ideal case to suggest REST over stitching, because it already has a robust REST API and for devs adding new functionality, we can make it easy to choose one over the other.

It is less straightforward if a backend app (Exchange, for example) only has a GraphQL API, and so it might be unnecessary overhead to start supporting a REST API, given we're all-in on GraphQL. However, in this case, I would prefer stitching over the "fake stitching" that hurt us in the past.

It's way out of scope of this RFC, but I'm still not sold on backend APIs communicating with each other via GraphQL (it's fine, but in my very limited experience the tooling hasn't seemed great for consuming those APIs). One reason we got into this situation we're in is because in the past, our recommendation for people starting new apps was to prefer GraphQL APIs over all else, assuming we'd be stitching. I'd be curious if that recommendation should change as well, especially given what we know now about stitching. (This could be a separate RFC 🙃).

from readme.

My understanding is that stitching also makes certain things difficult or impossible with regards to typings and fragments. For instance each one of our services has their own PageCursors type:

from readme.

We also don't stitch Metaphysics and other services back into Gravity. Which further makes other things difficult or impossible. Gravity fields don't implement images correctly AFAIK. If it used markdown or date formatting those implementations would drift, etc.

I think going with the REST pattern is the way to go for now but I'd like to recommend we explore federation in the future as well.

from readme.

Just a thought but another, maybe more specific way to phrase this could be:

Officially recommend against adding operations to the GravitySchema:

https://github.com/artsy/gravity/blob/main/app/graphql/gravity_schema.rb#L1

from readme.

Seconding @dzucconi around confusion exploring our schema, developers unfamiliar with the stitching paradigm open up the Metaphysics GraphiQL docs and see all kinds of things like

What does this even mean? (For devs familiar with stitching, what does this even mean?) It's unfortunate noise related to types and clashing -- the very things stitching is supposed to resolve, but in practice it creates a by-product in the form of this kind of output, permanently muddying the understandability of our GraphQL layer.

@jonallured

Just a thought but another, maybe more specific way to phrase this could be Officially recommend against adding operations to the GravitySchema

I am being more specific here! Because in the end, arguing against stitching is not the same as arguing against a microservice having a GraphQL layer. GraphQL is fine. The argument is against using stitching if the desired end result is consuming said data from Metaphysics -- something REST + a dataloader is perfectly suited to.

from readme.

It's worth giving another look in Exchange's stitching.ts file in order to reflect on the degree of complexity therein. Exchange deals with our $$. If it was simply an old fashioned REST app connected to Metaphysics via a DataLoader, would any of this have been necessary? Somehow we've even found ourselves within the realms of AST manipulation. Why? Working around the hidden costs of stitching within a system that never needed to scale.

from readme.

It's worth giving another look in Exchange's stitching.ts file in order to reflect on the degree of complexity therein. Exchange deals with our $$. If it was simply an old fashioned REST app connected to Metaphysics via a DataLoader, would any of this have been necessary? Somehow we've even found ourselves within the realms of AST manipulation. Why? Working around the hidden costs of stitching within a system that never needed to scale.

I agree, I had to create a new mutation using Stitching in this file was kinda fuzz to handle de AST to get the right property and keep the work. 😵

So what's the best solution in this Exchange/Gravity case, dataLoaders + REST (sounds redundant but wanted to make sure we're still fine with these situations, I heard when I came that we move away from REST as much as possible but I guess had a different context that phrase)?
FYI, in that case, we reused all types from Exchange's mutation so was supposed to keep in sync, but I assume that's not usually the case

from readme.

Rather than officially recommend against stitching, could we instead promote the validity of REST and data loaders alongside stitching, and leave it up to the developer?

There are good vanilla-ish examples of merging in a schema with no stitching, or minimal stitching (like including an artwork connection within them). Everything mentioned in this thread are real examples of stitching complexity, I just think we can officially recommend REST and data loaders as well (I thought this was the status quo, that both GraphQL w/ stitching and REST/data loaders were recommended and supported).

from readme.

I thought this was the status quo, that both GraphQL w/ stitching and REST/data loaders were recommended and supported

As mentioned in the cited thread the whole point of this RFC is reduce future complexity / maintainability burdens. To say that a developer should use what they feel is best ignores the thesis of this RFC, that there are serious complexity costs around expanding / comprehending our stitching-based codebase, compared to just using REST.

If a developer can achieve a fully functional connection by using a REST endpoint + paginate_and_sort + paginationResolver in metaphysics and be done with it -- three very low-complexity operations requiring very little understanding of any of the three technologies -- then why would we recommend that they freely choose between REST or stitching, where for the latter they'd need to have a fairly advanced understanding of 5+ technological domains just to feel confident enough to contribute and debug should something go wrong? The point of all of this is to clarify the status quo for the team by reevaluating past decisions.

Now imagine a future where there are no more stitching-proficient devs on the team (right now there are what, maybe 2-3? already danger territory), would you rather leave a legacy that provides simple/clear instructions around REST + Gravity v1, or a detailed document that links out to the stitching docs? Because one could clearly document how to add a rest endpoint and use a dataloader in MP, but under no circumstances could we internally explain to a future engineer how to stitch in anything of any complexity without ensuring that they have sufficient understanding of numerous advanced concepts, and the confidence to execute on them. The time required to release a feature instantly skyrockets due to learning alone, independent of any actual coding work. And then throw in debugging time, which is a whole different matter.

from readme.

Could you clarify:

• What part of this proposal is specific to Gravity? Some of the specific complaints would apply equally to any ruby back-end, or even to any back-end. Is the difference that Gravity has a well-established REST API, or that Gravity's schema tends to require more of the complex stitching work, or something else? Without a good reason, I'd hate to recommend one approach for most other services and then have a big exception for Gravity.
• What about mutations and schema properties that can be straightforwardly "merged" into Metaphysics' schema without stitching complexities (e.g., a widgetsConnection and a createWidget mutation)? Are those acceptable or do you propose REST throughout?

from readme.

What about mutations and schema properties that can be straightforwardly "merged" into Metaphysics' schema without stitching complexities (e.g., a widgetsConnection and a createWidget mutation)? Are those acceptable or do you propose REST throughout?

That's the thing, they aren't straight forward due to the differences in Pagination-related types. Like if we define a single pagination fragment for use in a pagination component in Force, it wouldn't be useable in stitched pagination fields. (I may be missing something here because this seems so obviously a problem that there has to be a reasonable solution?)

from readme.

I completely agree that stitching adds a lot of problems and complexity in most cases and we should favor MP+Loaders. But looking at cases where stitching makes things easier I wonder if we should allow (or even encourage) the usage of stitching in those cases (e.g. simple mutations, services like Convection with independent types).

What do you think about restricting the usage of stitching to a list of well defined cases and recommend MP+Loader in all other cases?

from readme.

Mutations are rarely going to be simple because they necessitate the logged in user and the return field should always include 'me' which is in Metaphysics

from readme.

I am with @mzikherman on this and would leave this up to the developer.

from readme.

@olerichter00 - this is really the dilemma here. And to @joeyAghion's point, I was originally thinking that this RFC would be about recommending against all stitching, but then I started thinking back to how undeniably well it worked with Convection and decided to hold back. (By "undeniably well" I mean: it worked really well for someone already fluent in stitching techniques.)

But even then, @dzucconi is also right. It might seem simple, but compared to basic CRUD operations performed via dataloaders, its still more complicated because in the end mutations are meant to return data, and once you start crossing schema boundaries then one is mixing mental models and we're back in the realm of a) required specialized knowledge; and b) unnecessary complexity.

Do you have any thoughts on how that list of allowed operations might look?

from readme.