[#295] fix failing with OverlongHeaders #315
Conversation
Problem: xrefcheck may fail with OverlongHeaders making it impossible to check a given file. Solution: make it possible to configure max header length for responses that xrefcheck is handling.
| metavar "INT" <> | ||
| value Nothing <> | ||
| help "The maximum allowed total size of HTTP headers (in bytes) \ | ||
| \ that can be returned by the server." |
There was a problem hiding this comment.
Nitpick: you insert two spaces between (in bytes) and that. I would strip the one before that.
There was a problem hiding this comment.
Actually, let's strip extending CLI options. The code is good, the reason is that, in this project, CLI arguments serve to tune how verification is run, and config serves to tune which set of errors should be covered. If I run xrefcheck for the first time on any given project, I shouldn't need to know that I have to specify --header-limit option.
So having header length limit specified in config should suffice.
| { cNetworking = addNetworkingOptions (cNetworking config) oNetworkingOptions } | ||
|
|
||
| mgr <- newManager $ managerSetMaxHeaderLength (ncMaxHeaderLength (cNetworking parsedConfig)) tlsManagerSettings | ||
| let fullConfig = parsedConfig & cNetworkingL . ncHttpManagerL .~ Just mgr |
There was a problem hiding this comment.
I think it's better not to mix config and things created at runtime, and propagate the manager as separate argument. Shouldn't require too much chances as far as I can tell.
| @@ -164,6 +166,7 @@ tests: | |||
| - tasty | |||
| - tasty-hunit | |||
| - tasty-quickcheck | |||
| - text | |||
| - time | |||
| - universum | |||
| - uri-bytestring | |||
There was a problem hiding this comment.
Just global comment: Make sure commit names start with uppercase, and use infinitive.
- [#xxx] Fix OverlongHeaders
- [#xxx] Add tests
Other than that, looks good, thanks for using Problem/Solution template for commit description 👍
|
Thanks for bringing this PR, really good job. Code is good and close to what I'd expect here. I'm leaving a few comments related to our coding practices and direction of this project. And yet to check the tests a bit later. |
|
Oh my apologies, I misclicked and closed the PR by accident. Wanted to say just in case, feel free to address review requests after the weekends. |
| [ VerifyLinkTestEntry | ||
| { vlteConfigModifier = \c -> c | ||
| & cNetworkingL . ncMaxHeaderLengthL .~ mhl | ||
| & cNetworkingL . ncHttpManagerL .~ Just mgr |
There was a problem hiding this comment.
If manager will be not part of config, its creation will likely be somewhere inside checkMultipleLinksWithServer, maybe right in verifyReferenceWithProgress even.
Description
Problem:
xrefcheck was failing with
OverlongHeaders, making it impossible to validate web pages (e.g., Notion pages) that respond with too long headers.Solution:
I added a configurable parameter,
ncMaxHeaderLength, allowing users to define the maximum header length that xrefcheck can handle.To achieve this, I added a new field,
ncHttpManager, which contains configurations related toOverlongHeaders. This field is purely internal and cannot be configured by users. Reusing a singleManagerfor the project provides maximal connection sharing.Related issue(s)
Fixes #295
✅ Checklist for your Pull Request
Ideally a PR has all of the checkmarks set.
If something in this list is irrelevant to your PR, you should still set this
checkmark indicating that you are sure it is dealt with (be that by irrelevance).
Related changes (conditional)
Tests
silently reappearing again.
Documentation
Public contracts
of Public Contracts policy.
and
Stylistic guide (mandatory)
✓ Release Checklist
package.yaml.under the "Unreleased" section to a new section for this release version.
vX.Y.Z.xrefcheck-action.