Skip to content

allow passing headers to subgraphs#53

Merged
mcollina merged 10 commits into
platformatic:mainfrom
mikaelkaron:mikaelkaron/header-forwarding
May 1, 2024
Merged

allow passing headers to subgraphs#53
mcollina merged 10 commits into
platformatic:mainfrom
mikaelkaron:mikaelkaron/header-forwarding

Conversation

@mikaelkaron
Copy link
Copy Markdown
Contributor

@mikaelkaron mikaelkaron commented Apr 23, 2024

@mikaelkaron mikaelkaron marked this pull request as draft April 23, 2024 20:44
Comment thread lib/composer.js Outdated
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test?

Comment thread lib/composer.js Outdated
@mikaelkaron
Copy link
Copy Markdown
Contributor Author

Can you add a test?

Of course. That’s why it’s a WIP ;)

@mikaelkaron
Copy link
Copy Markdown
Contributor Author

A few notes on the implementation:

  1. Is it enough to just have a blacklist of headers to remove
  2. Should there also be a "whitelist" of headers to keep
  3. Is there a need to transform headers
  4. Should there be a way to inject whitelist/blacklist/transformer from the outside

So I'm not sure about 1, I'm hoping that it's OK to just forward all headers (except a few) but I can already think about problems this may cause downstream.

The problem with 1 can be partially solved with 2, but it adds complexity that may not be needed at this stage.

I don't think we should transform headers here - if it's needed it should happen before we get here.

Providing some configuration would probably be a good idea - I'll try to add this without adding complexity. I'm hoping this can be simple enough to be serialized and provided in a platformatic configuration in the end.

@mcollina
Copy link
Copy Markdown
Member

IMHO having a callback to transform the headers would solve all the configurability problems. However, keep this implementation as simple as possible, and we can add that later on if needed.

@mcollina
Copy link
Copy Markdown
Member

Any help with the tests?

@mikaelkaron
Copy link
Copy Markdown
Contributor Author

Sorry. I’ll try to get to it today.

@mikaelkaron mikaelkaron requested a review from mcollina April 29, 2024 17:24
@mikaelkaron mikaelkaron marked this pull request as ready for review April 29, 2024 17:24
Comment thread lib/network.js Outdated
@mikaelkaron mikaelkaron changed the title allow passing headers downstream allow passing headers to subgraphs Apr 29, 2024
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mikaelkaron
Copy link
Copy Markdown
Contributor Author

Sorry @mcollina - just pushed a change renaming the test to align with other test names

@mcollina mcollina merged commit ec0e239 into platformatic:main May 1, 2024
@mikaelkaron mikaelkaron deleted the mikaelkaron/header-forwarding branch May 1, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants