Fix building of GraphQL queries arguments hash

[y9v]

Fixes #3874

What does this PR do?
This PR changes the way we extract arguments from GraphQL queries.
It fixes following things:

• handling of fragments
• handling for field aliases
• handling of default values
• handling of mutation input object

Motivation:
This issue

Additional Notes:
This PR also changes the keys for arguments, they were query names before, but according to our RFC it should be resolver names.

How to test the change?
Unit tests are included. I also set up a local test GraphQL application for testing which I can share.

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-09-11 15:37:47

Comparing candidate commit 0259cdb in PR branch appsec-fix-graphql-query-args with baseline commit bc84181 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 2 unstable metrics.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 98.63946% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.85%. Comparing base (bc84181) to head (0259cdb).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
.../datadog/appsec/contrib/graphql/gateway/watcher.rb	84.61%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3887   +/-   ##
=======================================
  Coverage   97.84%   97.85%           
=======================================
  Files        1279     1280    +1     
  Lines       76530    76621   +91     
  Branches     3752     3753    +1     
=======================================
+ Hits        74883    74976   +93     
+ Misses       1647     1645    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[anmarchenko]

Thanks for tackling this, it looks cleaner and I like the additional tests. Left a few comments on some quick performance wins.

[anmarchenko]

Looks good, thank you 👍🏻

[TonyCTHsu]

We've talked about being slightly more defensive:

1. Rescuing unexpected error and return a benign value
2. Validate the presence of constant which the implementation depends on before applying the patch

[y9v]

@TonyCTHsu as discussed, I removed direct usage of graphql-ruby constants here and added a compatibility check based on whether they are defined or not.

And here I added rescuing of unexpected errors during AppSec instrumentation.

[TonyCTHsu]

Approve!

[TonyCTHsu]

Adding a couple of test cases

1. Test positive with our graphql appraisal groups.
2. Test negative with hide_const

[y9v]

added here: 20a00b8

[vpellan]

LGTM!

[Strech]

Can't comment much on the correctness of the code, but here are my 50c with the tests.

[Strech]

Just mine 50c since I can't add more to the actual code 😄

Suggested change

	expect(described_class.ast_node_classes_defined?).to eq(true)
	expect(described_class.ast_node_classes_defined?).to be(true)

Explanation: https://stackoverflow.com/questions/26779937/what-is-the-difference-between-be-true-and-be-true-in-rspec

[anmarchenko]

hm, I still don't see in the link above what is the difference in this case between be and eq: I always use eq for booleans as it reads similar to == true or == false

[y9v]

it is the same actually, I just thought that be(true) would be a bit more readable

[p-datadog]

I personally type expect(xxx).to be true because it saves me two shift presses with pinky but as far as reading the code I don't see any difference between be true and eq(true). Semantically I believe the two are identical.

[Strech]

Suggested change

	expect(described_class.ast_node_classes_defined?).to eq(false)
	expect(described_class.ast_node_classes_defined?).to be(false)

[Strech]

Just a suggestion over the simplicity, I think the queries var doesn't bring much value (hiding setup complexity) nor explaining the value. Because of that I think it's easier just to use the real expectation value

Suggested change

	let(:queries) { [query] }
	it 'returns queries' do
	expect(dd_multiplex.queries).to eq(queries)
	it 'returns queries' do
	expect(dd_multiplex.queries).to eq([query])

[y9v]

thanks! fixed in 242eb7c

[lloeki]

Approved, although. I can't say much about the more intricate technical details here.

[p-datadog]

The GraphQL logic itself is above my pay grade I'm afraid.

[y9v]

Merging this, the failing system test will be fixed by DataDog/system-tests#2988