fix(gateway): maintain immutability when merging selection sets

[jakeblaxon]

This PR fixes an issue with my previous PR that was merged in recently (#4064). In the previous PR, the merging method was updating original FieldNode objects that tied back to the schema, corrupting data across requests. This PR fixes that issue, so that the method now creates a copy of the FieldNode object so as to preserve immutability.

[jakeblaxon]

@trevor-scheer Please take a look at this ASAP. This solves a bug in my previous PR that can lead to data corruption. I discovered this bug when I noticed that cached queryplan data was changing, causing subsequent calls of the same query to fail.

I figured this doesn't need a test case since it merely updates the code to follow the design principles. The slight change in the queryplan snapshot also reflects this update, so it implicitly tests that immutability is followed. If you'd like me to write a new explicit test case, however, please let me know. Thanks!

[jakeblaxon]

@trevor-scheer do you have some time to look at this? It's a pretty short change, but it fixes a critical issue where cached data can become corrupted. Please let me know if you have any questions or comments, and I'll be happy to help.

[trevor-scheer]

Hey @jakeblaxon, sorry for the delay on this. A couple questions for you:

1. Can you demonstrate the bug you're experiencing with a failing test? I know you mentioned this is across multiple requests, let me know if you need any pointers in getting this set up.

2. I haven't looked closely yet, but if you can justify / provide reason to the change in the existing test that would be wonderful.

Thanks for your vigilance on this!

[jakeblaxon]

Hey @trevor-scheer, I can justify the change to the test as follows: In the query plan, we first fetch all fields that we'll ever need from service A, which includes the animal and color, so that's correct. However, in the Flatten node, we only need the fields from service A that are required by service B. Since we're only querying for favoriteAnimal from B, we only need the animal field from A, not the color field. Therefore, we can safely remove the color field in this spot in the query plan. It was getting included because the selectionset of the original favorites field node was getting updated, even though it should be treated as immutable.

I can definitely write a test for this! Will it be adequate to just write multiple execute() calls in the same test, or will I need to stand up a server in order to make use of the queryplan cache? On my first try it looks like I may need to stand up a server, but I haven't looked too closely at the details yet.

Also, I noticed that this PR was just closed out. Can I repoen it, or should I open a new one? Thanks for the help!

[abernix]

@jakeblaxon This was closed inadvertently and we intend on re-opening it; See #4304 for details. We're waiting for a response from GitHub Support, but we'll take care of re-opening it if they can't trigger something on their end after diagnosing what happened.

[jakeblaxon]

@trevor-scheer I've added a regression test as you requested. This new test fails for the current main branch but succeeds with this new fix. Please take a look. Thanks!

[trevor-scheer]

This is all wonderful @jakeblaxon. Thank you for additional explanation, and the test case looks great! Would you say this test belongs in the requires test suite? How do you feel about the queryPlanCache test suite instead?

I should be able to land and release this on Monday (tomorrow)! cc @abernix

[jakeblaxon]

@trevor-scheer I put it in the requires test suite because the nested requires logic was the source of the problem, but I'm fine with moving it to the query plan cache as well. I'll let you move to wherever you see fit. Thank you for your help with this!