Add TransfomHeadersAgent #26

szmarczak · 2021-08-03T12:48:01Z

Moved from #25

The should add custom headers test is flaky, as the headers are fixed in the agent. ~~Will fix this asap.~~ Fixed.

mnmkng

Thanks for the PR, the code looks pretty good and I think it's a nice improvement. There are only two things we should do before merging:

Please add some unit tests for the TransformHeadersAgent class. Especially some sanity checks for the private APIs it uses. I noticed that you made some comments in the previous PR, but in the PR comments, not in code.
I made a lot of comments about comments 😄 We try to explain everything that's not "common knowledge", or at least point to the relevant resource via link. Not all devs have deep knowledge of Node.js core HTTP (or any other complex system) so it's always nice to at least point them in the right direction. We also try to follow the KISS principle, because as a company, we never know who might be the next guy who will need to read this code and make changes to it.

src/agent/transform-headers-agent.js

mnmkng · 2021-08-03T13:44:03Z

src/hooks/proxy.js

@@ -25,27 +26,25 @@ exports.proxyHook = async function (options) {
 const parsedProxy = new URL(proxyUrl);

 validateProxyProtocol(parsedProxy.protocol);
- const agents = await getAgents(parsedProxy, options.https.rejectUnauthorized);
+ options.agent = await getAgents(parsedProxy, options.https.rejectUnauthorized);


I'm a bit torn whether this improves readability. I know it's shorter this way, but it made more sense to me when both the options were next to each other and not separated by the big comment. I don't have a strong opinion about this, but would like to learn why you think it's better this way.

It's required. Otherwise it would fail if the user provided their own agents.

It's because http2.request is used here.

Maybe I'm blind, but I don't see any difference in behavior between:

const agents = await getAgents(parsedProxy, options.https.rejectUnauthorized); if (resolvedRequestProtocol === 'http2') { options.agent = agents[resolvedRequestProtocol]; } else { options.agent = agents; }

and

options.agent = await getAgents(parsedProxy, options.https.rejectUnauthorized); if (resolvedRequestProtocol === 'http2') { options.agent = options.agent[resolvedRequestProtocol]; }

🤷‍♂️

The if (resolvedRequestProtocol === 'http2') { is outside if (proxyUrl) {, previously it was inside. You're not blind, it just may not be obvious at first sight. Let me add appropriate comments in this regards :)

got-scraping/src/hooks/proxy.js

Lines 46 to 51 in 8ead6d3

* The `if` below cannot be placed inside the `if` above.

* Otherwise `http2.request` would receive the entire `agent` object

* __when not using proxy__.

* ---

* `http2.request`, in contrary to `http2.auto`, expects an instance of `http2.Agent`.

* `http2.auto` expects an object with `http`, `https` and `http2` properties.

Unless you mean something else 🤔

test/helpers/dummy-server.js

src/agent/transform-headers-agent.js

szmarczak · 2021-08-03T15:17:11Z

Some other tests are still flaky. We would need a server that would return rawHeaders. browser-info API would suit this.

szmarczak · 2021-08-04T08:09:04Z

Should I update the readme as well? People may get confused when they're expecting lower-case headers to be sent and got Pascal-Case instead.

mnmkng

Just one small nitpick in tests. I think you've done a great job 👏

test/agent.test.js

mnmkng · 2021-08-04T08:17:23Z

Should I update the readme as well? People may get confused when they're expecting lower-case headers to be sent and got Pascal-Case instead.

Yeah, that's a good idea. There's just one thing that did not occur to me before and I probably have not mentioned it. We want to pascal case the headers sent by the browser itself for HTTP/1.1, that's for sure. But custom headers like x-something-stuff should probably stay with the user provided case. What do you think?

szmarczak · 2021-08-04T08:18:45Z

What do you think?

Good idea as well.

mnmkng

Cool. Is it mergeable or do we need to wait for header-generator or something else?

mnmkng · 2021-08-04T08:43:30Z

src/agent/transform-headers-agent.js

+ if (key.toLowerCase().startsWith('x-')) {
+ headers[key] = request.getHeader(key);
+ } else {
+ headers[this.toPascalCase(key)] = request.getHeader(key);
+ }


Oh, sorry for the confusion. The x-something was just an example. The header could be my-header as well or some other randomness. We've seen quite a few.

Also I think there are some X- headers which are actually sent by the browsers like X-Requested-With.

So the determination of the "custom headers" is a bit more complex.

Lemme patch this real quick

X-Requested-With doesn't exist in the spec: https://www.iana.org/assignments/message-headers/message-headers.xhtml nor in the MDN docs: https://developer.mozilla.org/pl/docs/Web/HTTP/Headers

If the generator returns in the correct casing then it should be no problem I think?

Yeah, the generator should always return correct casing. Unless we encounter some crap UA like the applebot.

And yeah, I might be wrong about the X-Requested-With. I've seen it multiple times so I thought it's sent by the browser but maybe it's just common among developers to include it.

Then this PR is good to go I think. I'm planning to optimize HTTP/2 related stuff next such as the ALPN negotiation. I think there's no need to manually store the cache anymore.

szmarczak · 2021-08-04T08:45:18Z

It's mergeable. I think it'd be better to add header-generator order integration in a different PR.

https:/apify/header-generator/blob/2ebee495b88efb144fc75e374ca3acb59e4d2832/src/header-generator.js#L288-L294

header-generator returns already sorted headers, but it's sorted at the wrong point. This way we don't know where the Connection header could go. I believe it would be best to return the headers as they are and additionally return an array of sorted headers.

README.md

szmarczak added 3 commits August 3, 2021 12:09

Add TransfomHeadersAgent

0a641d7

Fix flaky test

c4ccbf2

Default agents should be transformed as well

c99fd8b

szmarczak requested a review from mnmkng August 3, 2021 13:15

mnmkng requested changes Aug 3, 2021

View reviewed changes

mnmkng reviewed Aug 3, 2021

View reviewed changes

src/agent/transform-headers-agent.js Outdated Show resolved Hide resolved

szmarczak added 2 commits August 3, 2021 16:30

More comments

ba8ce71

Add another comment

2be4f86

Rename transformHeader to toPascalCase

328c700

szmarczak marked this pull request as draft August 3, 2021 17:42

szmarczak added 2 commits August 3, 2021 22:58

Update comment

8ead6d3

Finish sorting, add agent tests

8228e07

szmarczak requested a review from mnmkng August 4, 2021 07:46

szmarczak marked this pull request as ready for review August 4, 2021 07:47

mnmkng requested changes Aug 4, 2021

View reviewed changes

test/agent.test.js Outdated Show resolved Hide resolved

szmarczak added 3 commits August 4, 2021 10:19

Add another test

8bc624f

Do not modify x-headers

814d3de

Update readme

7e79a8e

mnmkng approved these changes Aug 4, 2021

View reviewed changes

mnmkng reviewed Aug 4, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

expection -> exception

318ef1d

mnmkng merged commit fe2bcc8 into master Aug 4, 2021

szmarczak deleted the transform-headers branch August 4, 2021 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TransfomHeadersAgent #26

Add TransfomHeadersAgent #26

szmarczak commented Aug 3, 2021 •

edited

Loading

mnmkng left a comment •

edited

Loading

mnmkng Aug 3, 2021

szmarczak Aug 3, 2021

szmarczak Aug 3, 2021

mnmkng Aug 3, 2021

szmarczak Aug 3, 2021

szmarczak Aug 3, 2021

szmarczak commented Aug 3, 2021

szmarczak commented Aug 4, 2021 •

edited

Loading

mnmkng left a comment

mnmkng commented Aug 4, 2021

szmarczak commented Aug 4, 2021

mnmkng left a comment

mnmkng Aug 4, 2021

szmarczak Aug 4, 2021

szmarczak Aug 4, 2021

szmarczak Aug 4, 2021

mnmkng Aug 4, 2021

mnmkng Aug 4, 2021

szmarczak Aug 4, 2021

szmarczak commented Aug 4, 2021

	* The `if` below cannot be placed inside the `if` above.
	* Otherwise `http2.request` would receive the entire `agent` object
	* __when not using proxy__.
	* ---
	* `http2.request`, in contrary to `http2.auto`, expects an instance of `http2.Agent`.
	* `http2.auto` expects an object with `http`, `https` and `http2` properties.

Add TransfomHeadersAgent #26

Add TransfomHeadersAgent #26

Conversation

szmarczak commented Aug 3, 2021 • edited Loading

mnmkng left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak commented Aug 3, 2021

szmarczak commented Aug 4, 2021 • edited Loading

mnmkng left a comment

Choose a reason for hiding this comment

mnmkng commented Aug 4, 2021

szmarczak commented Aug 4, 2021

mnmkng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak commented Aug 4, 2021

szmarczak commented Aug 3, 2021 •

edited

Loading

mnmkng left a comment •

edited

Loading

szmarczak commented Aug 4, 2021 •

edited

Loading