fix: handle `Accept` header #1789

webbdays · 2024-04-24T20:30:49Z

Summary:

Accept header handling

Issue Reference(s):
#1766

Build & Testing:

[yes ] I ran cargo test successfully.
I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

I have added relevant unit & integration tests.
I have updated the documentation accordingly.
[ yes] I have performed a self-review of my code.
[ yes] PR follows the naming convention of <type>(<optional scope>): <title>

codecov · 2024-04-24T20:37:57Z

Codecov Report

Attention: Patch coverage is 77.27273% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 86.86%. Comparing base (77508ee) to head (8747347).
Report is 63 commits behind head on main.

Files	Patch %	Lines
src/http/request_handler.rs	76.74%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1789      +/-   ##
==========================================
- Coverage   86.88%   86.86%   -0.03%     
==========================================
  Files         155      155              
  Lines       15581    15625      +44     
==========================================
+ Hits        13538    13572      +34     
- Misses       2043     2053      +10

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

tusharmath · 2024-04-25T04:29:35Z

tests/execution/add-field-modify.md

@@ -43,6 +43,8 @@ type Query {
 ```yml @test
 - method: POST
 url: http://localhost:8080/graphql
+ headers:
+ Accept: application/graphql-response+json


Move this logic into execution spec so that by default we make the request with headers and don't need to modify all the tess.

I accept there are changes in many tests files.
Realistically, headers comes form request which we define in tests files. That should be the source of truth.
Also it will be the problem for us to write tests for non compliance.
the non compliance tests i wrote fails.
@tusharmath

tusharmath · 2024-04-25T13:37:51Z

src/http/request_handler.rs

+ let count = req
+ .headers()
+ .get("Accept")
+ .unwrap()


let's not do unwrap

tusharmath · 2024-04-25T13:38:41Z

src/http/request_handler.rs

+ let error_message = "Unsupported 'Accept' Header value in the request, required 'application/graphql-response+json'";
+
+ // enforce this only on/after 1/1/2025, just warn until then.
+ if NaiveDate::from_ymd_opt(2025, 1, 1) <= Some(Local::now().date_naive()) {


Is the date check required?

According to draft, that requirement is must after that date.
so checking date.
But if we want to enforce that spec right from now, then not needed.
I am checking date to maintain compatibility with legacy applications and give some time for them to adopt.(according to spec.)

tusharmath · 2024-04-25T13:40:22Z

src/http/request_handler.rs

+ }
+ }
+ }
+


Let's put this header check in a separate function.

ok. i also thought of that.
what about creating separate module to house all the logic to check spec.
like module: graphql_over_http ?
@tusharmath

github-actions · 2024-04-27T14:02:26Z