feat: support custom-headers by wrath-of-god · Pull Request #455 · tailcallhq/tailcall

wrath-of-god · 2023-10-09T17:11:01Z

/claim #377
fixes #377
Build & Testing:

I ran cargo test successfully.
I have run ./lint.sh to address and fix linting issues.

Checklist:

I have added relevant unit & integration tests.
I have updated the documentation accordingly (if applicable).
I have performed a self-review of my own code.

codecov · 2023-10-09T17:13:41Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (0450061) 84.20% compared to head (2ee7e4d) 85.94%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
+ Coverage   84.20%   85.94%   +1.74%     
==========================================
  Files          50       50              
  Lines        4532     4554      +22     
==========================================
+ Hits         3816     3914      +98     
+ Misses        716      640      -76

Files	Coverage Δ
src/blueprint/from_config.rs	`96.60% <100.00%> (+0.06%)`	⬆️
src/config/server.rs	`80.00% <ø> (+12.00%)`	⬆️
src/http/server.rs	`73.01% <100.00%> (+73.01%)`	⬆️
src/http/server_context.rs	`92.30% <100.00%> (+92.30%)`	⬆️

... and 4 files with indirect coverage changes

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

wrath-of-god · 2023-10-09T17:14:20Z

@tusharmath @amitksingh1490 do not able to test this? any recommendation/direction?

tusharmath · 2023-10-10T04:27:39Z

We will need tests over here as coverage drops quite significantly. I think a little bit of refactoring of the functions can make this piece more testable without requiring mocks.

Test Specifications:

We should be able to view all errors with every invalid header name or value.
Invalid header error should be composed with Blueprint generation errors.

tusharmath · 2023-10-10T10:51:16Z

Let me know if there are any other questions @wrath-of-god Feel free to hop on our discord to get more help if needed.

examples/jsonplaceholder.graphql

tusharmath · 2023-10-11T11:46:30Z

I think this is great! We could clean this up a little bit though.

Create a blueprint::Server that contains a processed version of config::Server
This way Blueprint contains processed a validated HeaderMap which can be passed around everywhere.

tusharmath · 2023-10-11T11:49:25Z

tests/graphql/errors/test-set-headers.graphql

@@ -0,0 +1,17 @@
+#> server-sdl
+schema @server(baseURL: "https://jsonplaceholder.typicode.com", setHeaders: [{key: "🤣", value: "a"}]) {


Suggested change

schema @server(baseURL: "https://jsonplaceholder.typicode.com", setHeaders: [{key: "🤣", value: "a"}]) {

schema @server(baseURL: "https://jsonplaceholder.typicode.com", responseHeaders: [{key: "🤣", value: "a"}]) {

How about calling it responseHeaders ?

tusharmath · 2023-10-11T11:53:16Z

tests/graphql/errors/test-set-headers.graphql

+
+#> client-sdl
+type Failure
+  @error(message: "Parsing failed because of invalid HTTP header name", trace: ["schema", "@server", "setHeaders"])


Add tests for invalid Http HeaderValue

Add tests for multiple failures ie, if multiple headers are invalid, we should be able to show all of them in errors.

tusharmath · 2023-10-13T11:30:16Z

src/blueprint/blueprint.rs

+pub struct Server {
+  pub allowed_headers: Option<HashSet<String>>,
+  pub base_url: Option<String>,
+  pub enable_apollo_tracing: Option<bool>,
+  pub enable_cache_control_header: Option<bool>,
+  pub enable_graphiql: Option<String>,
+  pub enable_http_cache: Option<bool>,
+  pub enable_introspection: Option<bool>,
+  pub enable_query_validation: Option<bool>,
+  pub enable_response_validation: Option<bool>,
+  pub global_response_timeout: Option<i64>,
+  pub port: Option<u16>,
+  pub upstream: Upstream,
+  pub vars: BTreeMap<String, String>,
+  pub batch: Option<crate::config::Batch>,
+  pub response_headers: HeaderMap,
 }


We can drop Option The Server in blueprint should contain the final values. Which means basically no option types, HeaderMap instead of KeyValues, Urls instead of String etc. It should be safe to use blueprint::Server.

tusharmath · 2023-10-13T11:31:19Z

src/blueprint/from_config.rs


-  Valid::Ok(())
+  server
 }


Can `impl Fromconfig::Server for blueprint::Server

This should be tryFrom and return Valid

tusharmath · 2023-10-13T11:31:45Z

src/valid/valid.rs

      },
    }
  }
+  fn validate_both<A1>(self, other: Result<A1, ValidationError<E>>) -> Valid<(A, A1), E> {


Did we use this function anywhere?

wrath-of-god · 2023-10-13T18:09:42Z

@tusharmath Please review before conflicts come 🙏 .

* commit '69814ec785fef5d5cf3d4ba925a4910e0a09715e': refactor: use default batch settings if group_by is used (tailcallhq#497) refactor: use blueprint server throughout (tailcallhq#495) feat: hostname config in server (tailcallhq#493) Load YAML & JSON formats via CLI (tailcallhq#467) fix(deps): update rust crate regex to 1.10.1 (tailcallhq#491) fix(deps): update rust crate async-trait to 0.1.74 (tailcallhq#492) feat: support custom-headers (tailcallhq#455) fix: n + 1 issue (tailcallhq#487) fix: mandatory file path refactor: drop key from group by (tailcallhq#484) feature: load multiple configs via CLI (tailcallhq#482) feat: merge right on config (tailcallhq#476) doc: example update fix: upstream setting keys should be optional (tailcallhq#474) refactor: prerequisite to test multi sdl (tailcallhq#475) refactor: use btreeset where duplicates are not allowed (tailcallhq#478) fix: info loss in const serialisation (tailcallhq#477) feat: support to configure http settings (tailcallhq#388)

wrath-of-god force-pushed the custom-headers branch from c98282e to 7b5b171 Compare October 11, 2023 03:54

algora-pbc bot mentioned this pull request Oct 11, 2023

Support custom response headers #377

Closed

algora-pbc bot added the 🙋 Bounty claim label Oct 11, 2023

wrath-of-god marked this pull request as ready for review October 11, 2023 04:14

amitksingh1490 reviewed Oct 11, 2023

View reviewed changes

examples/jsonplaceholder.graphql Outdated Show resolved Hide resolved

tusharmath reviewed Oct 11, 2023

View reviewed changes

wrath-of-god force-pushed the custom-headers branch 2 times, most recently from dd1b38b to e5743cb Compare October 12, 2023 18:18

tusharmath reviewed Oct 13, 2023

View reviewed changes

wrath-of-god added 12 commits October 13, 2023 21:13

feat:custom-headers

7f2fe9b

invalid headers

56cfbd0

lint

43fbacf

lint

056e2e4

integration test

aa06427

revert change

b6f6fc8

rename

8b5e4e9

multi validate

c030d9a

in progress refactor

38bbb6b

use blueprint server

cc14cf7

cleanup 😥

db637ad

review comment

6a9999b

wrath-of-god force-pushed the custom-headers branch from e5743cb to 6a9999b Compare October 13, 2023 17:52

cleanup 😥

de9f61f

tusharmath changed the title ~~feat:custom-headers~~ feat: support custom-headers Oct 14, 2023

tusharmath approved these changes Oct 14, 2023

View reviewed changes

tusharmath merged commit 1635866 into tailcallhq:main Oct 14, 2023

tusharmath mentioned this pull request Oct 14, 2023

feat: custom response headers #392

Closed

5 tasks

tusharmath added the enhancement label Oct 17, 2023

		@@ -0,0 +1,17 @@
		#> server-sdl
		schema @server(baseURL: "https://jsonplaceholder.typicode.com", setHeaders: [{key: "🤣", value: "a"}]) {

Conversation

wrath-of-god commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wrath-of-god commented Oct 9, 2023

Uh oh!

tusharmath commented Oct 10, 2023

Uh oh!

tusharmath commented Oct 10, 2023

Uh oh!

Uh oh!

tusharmath commented Oct 11, 2023

Uh oh!

tusharmath Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

tusharmath Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

tusharmath Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

tusharmath Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

amitksingh1490 Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

tusharmath Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

wrath-of-god Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

wrath-of-god commented Oct 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wrath-of-god commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading