ardatan / graphql-tools

:wrench: Utility library for GraphQL to build, stitch and mock GraphQL schemas in the SDL-first approach
https://www.graphql-tools.com
MIT License
5.34k stars 810 forks source link

Stitched schema does return "null" for aliased fields in union types #4394

Open xddq opened 2 years ago

xddq commented 2 years ago

Issue workflow progress

Progress of the issue based on the Contributor Workflow


Describe the bug

To Reproduce Steps to reproduce the behavior:

Expected behavior


** Actual Behaviour **
- result contains "null" for aliased fields

{ "data": { "articles": [ { "title": { "text": "hello world" } }, { "title": { "renamedText": null } }, { "title": { "renamedText": null } }, { "title": { "text": "bye" } } ] } }



**Environment:**

- OS: ubuntu 18.04
- "@graphql-tools/load": "^7.5.8",
- "@graphql-tools/schema": "^8.3.8",
- "@graphql-tools/stitch": "^8.6.6",
- "@graphql-tools/url-loader": "^7.9.11",
- "@graphql-tools/wrap": "^8.4.13",
- NodeJS: v14.19.1
yaacovCR commented 2 years ago

Thanks for submitting this!!!

yaacovCR commented 2 years ago

Are you able to post what query hits the remote schema?

xddq commented 2 years ago

Hey @yaacovCR , thanks for the fast response.

I did add query logging and the logging results into reproduction repo

The copy pasted QueryLog.md file:

When querying the schema directly on localhost:4001/graphql

query {articles {title {
  ... on TitleOne {text}
  ... on TitleTwo {renamedText: text}
}}}
remote-1 query:  {
  articles {
    title {
      ... on TitleOne {
        text
      }
      ... on TitleTwo {
        renamedText: text
      }
    }
  }
}

remote-1 variables:  {}

When querying the stitched schema on localhost:4444/graphql

query {articles {title {
  ... on TitleOne {text}
  ... on TitleTwo {renamedText: text}
}}}
remote-1 query:  {
  articles {
    title {
      ... on TitleOne {
        text
      }
      ... on TitleTwo {
        renamedText: text
      }
      __typename
      __typename
    }
  }
}
remote-1 variables:  {}

stitched query:  {
  articles {
    title {
      ... on TitleOne {
        text
      }
      ... on TitleTwo {
        renamedText: text
      }
    }
  }
}

stitched variables:  {}
yaacovCR commented 2 years ago

i tried to reproduce this with additional test and could not get it to fail.

perhaps the problem is the subschema implementation being used -- i am not familiar with it and used a local schema.

see https://github.com/ardatan/graphql-tools/pull/4396

yaacovCR commented 2 years ago

The queries look fine what about the remote response?

xddq commented 2 years ago

Mhhh, that's strange. I mean you did use subschemas as well here

I am currently trying to figure out how to log the response from the remote-1 server. Will post it once I figured it out.

Do you see any issue with the reproduction repo?

xddq commented 2 years ago

Log for the request-response cycle. Starting from stitched and starting from non-stitched.

query {
  articles {
    title {
      ... on TitleOne {
        text
      }
      ... on TitleTwo {
        renamedText: text
      }
    }
  }
}

log when coming from localhost:4444/graphql (stitched)

[stitched] stitched request:  {
[stitched]   articles {
[stitched]     title {
[stitched]       ... on TitleOne {
[stitched]         text
[stitched]       }
[stitched]       ... on TitleTwo {
[stitched]         renamedText: text
[stitched]       }
[stitched]     }
[stitched]   }
[stitched] }
[stitched]
[stitched] stitched variables: {}
[remote-*1] remote-1 request:  {
[remote-*1]   articles {
[remote-*1]     title {
[remote-*1]       ... on TitleOne {
[remote-*1]         text
[remote-*1]       }
[remote-*1]       ... on TitleTwo {
[remote-*1]         renamedText: text
[remote-*1]       }
[remote-*1]       __typename
[remote-*1]       __typename
[remote-*1]     }
[remote-*1]   }
[remote-*1] }
[remote-*1] remote-1 variables: {}
[remote-*1] remote-1 response: {"articles":[{"title":{"text":"hello world","__typename":"TitleOne"}},{"title":{"renamedText":1,"__typename":"TitleTwo"}},{"title":{"renamedText":2,"__typename":"TitleTwo"}},{"title":{"text":"bye","__typename":"TitleOne"}}]}
[stitched] stitched response: {"articles":[{"title":{"text":"hello world"}},{"title":{"renamedText":null}},{"title":{"renamedText":null}},{"title":{"text":"bye"}}]}

log when coming from localhost:4001/graphql (non-stitched)

[remote-*1] remote-1 request:  {
[remote-*1]   articles {
[remote-*1]     title {
[remote-*1]       ... on TitleOne {
[remote-*1]         text
[remote-*1]       }
[remote-*1]       ... on TitleTwo {
[remote-*1]         renamedText: text
[remote-*1]       }
[remote-*1]     }
[remote-*1]   }
[remote-*1] }
[remote-*1]
[remote-*1] remote-1 variables: {}
[remote-*1] remote-1 response: {"articles":[{"title":{"text":"hello world"}},{"title":{"renamedText":1}},{"title":{"renamedText":2}},{"title":{"text":"bye"}}]}
yaacovCR commented 2 years ago

Mhhh, that's strange. I mean you did use subschemas as well here

I used local subschemas, i mean, not local schemas. so the executor is the default executor not whatever you set up.

Do you see any issue with the reproduction repo?

Not from the brief glance, but I just looked at it enough to make a minimal reproduction. We will know more when you get the response from the remote subschema to show up.

In theory, if it can't be reproduced with local schemas, it's not a bug in stitching, it's a bug in the executor you are using => but that's just in theory. If you find anything I can fix, I would be very happy.

xddq commented 2 years ago

Okay so actually all that was needed was to add an executor to the subschema config. I am not yet sure how the executor works and why it is needed (since it worked without specifying one, besides the aliases on union types issue?)

I did upload the "corrected" version in the repro repo. I am not sure if this is still worth while to take a look into it for you.

Thanks a LOT for your time and your quick responses!

Urigo commented 2 years ago

@xddq @yaacovCR I'm not adding anything here but just trying to label it correctly. So we couldn't yet reproduce it with a failing test but we do have a reproduction on a repo right?

xddq commented 2 years ago

@Urigo We have a reproduction repo, yes.

But I am not sure whether this is worth to get into since I did NOT specify an exectuor for my stitched schema while the documentation clearly states that an executor is needed. here, cited 2022-04-22-20:00:00

The only real bug (in my opinion) is that it did work without me ever specifiying an executor. It only failed while trying to rename attributes inside a union type.

I think it should error out when trying to create a subschema without specifying an executor, if one is required.