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

Handle fields omitted through @skip or @include #79

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spawnia
Copy link
Owner

@spawnia spawnia commented Nov 10, 2022

  • Added automated tests
  • Documented for all relevant versions
  • Updated the changelog

#77

Changes

When an operation tells the server to omit a field through @skip or @include, the result
will not contain those fields at all. Thus, the generated result must be marked nullable and be
able to handle if those keys are missing.

Breaking changes

I foresee none.

@CRC-Mismatch
Copy link
Contributor

CRC-Mismatch commented Apr 17, 2023

I was checking #77, #78 and this for an upcoming task I'm about to take on later this week or the next.

Is it currently already possible to properly use the directives, or are there still any limitations that this PR is meant to solve?

TL;DR - We need to suppress unneeded queries and results to reduce the performance penalty

We're transitioning back-ends from an close-to-unmaintainable REST-only system to a new back-end designed from scratch with GraphQL-only support in mind. To enable the front-end developers to "chop" the transitioning so we can deliver a production-ready migration without having to delay everything for months (or years), we've developed a BFF that "translates" the most coupled legacy REST contracts and endpoints to the new model using Sailor. I've developed a Symfony bundle wrapper (mismatch/sailor-bundle) to ease its integration in our current projects' architecture.

Problem is, we can't deliver the BFF (and by association the clients' migration) as "production-ready" until we can solve the huge performance penalty due to making one REST request become ~15 GraphQL queries and mutations (most of the queries are already compressed in 3 requests, but the mutations are mostly one-per-request) since when we originally started, we couldn't leverage the @include and @skip directives.

Without them, we'll have to create one variation of each query or mutation for each possible scenario where we don't need something, which could solve the performance problem with some clever coding to decide when to use what; but that would mean having around 4x the number of generated classes, and the same amount of new code-paths (incurring in huge NPath and Cyclomatic complexities, in a code that already suffers from these).

I'd like to know the current status, as when I take on that task, if something is still amiss, I'm probably going to first try and work on this PR, instead of turning our BFF's code into a Hydra right away.

@spawnia spawnia added the bug An error within Sailor label Apr 18, 2023
@spawnia
Copy link
Owner Author

spawnia commented Apr 18, 2023

Is it currently already possible to properly use the directives, or are there still any limitations that this PR is meant to solve?

No, the problems with them are detailed in the description of this pull request. Sailor will currently not handle a field missing due to @skip or @include gracefully. Feel free to fork and propose a solution to this pull request, it is currently only a failing test.

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

Successfully merging this pull request may close these issues.

2 participants