fix: support non-lower-case Content-Type header provided by app

[vladimir-mencl-eresearch]

The changes in #511 change what headers get set by PDFKit and make all headers set by PDFKit lower-case.

However, the changes also affect code depending on headers set by the app - which for Rack 2.x apps will be 'Content-Type'.

To address this, the code should check if Content-Type is present and in that case use it, otherwise default to content-type.

As the code also sets this header (changing the content type to 'application/pdf'), it should set the same header that the original value is retrieved from.

So decide on the exact header name first, store it in the content_type_header variable, and use it to index the headers dict.

Fix #515.

[vladimir-mencl-eresearch]

@serene , please let me know whether happy to merge this PR or whether any changes and/or clarification are needed.

[vladimir-mencl-eresearch]

Hi @serene , this (the issue fixed here and described in #515 ) is blocking our rollout of 0.8.7. Would you be able to review this PR - and if OK, merge and cut a release? Thanks a lot in advance. Cheers, Vlad

[serene]

@vladimir-mencl-eresearch Please add a test that covers this case

[vladimir-mencl-eresearch]

Hi @serene ,

Thanks, I've just added the tests.

Note that as rack 3.x outright rejects mixed case headers, the test for the mixed case scenario had to be marked as pending - but passes (without the pending flag) with rack 2.x.

Cheers,
Vlad

[vladimir-mencl-eresearch]

Hi @serene ,

I've made the pending flag conditional on Rack version >= 3.0.0 - so on Rack 2.2.4, it actually runs the test and records it as correctly passing, while on Rack 3.0.0, the test is flagged as pending - and records the exception raised by Rack (Rack::Lint::LintError: uppercase character in header name: Content-Type), but does not cause the test-suite to fail.

Please let me know whether you're happy with this.

Cheers,
Vlad

[exviva]

I got bitten by this too, I'm also setting 'Content-Disposition' to attachment in my controller, but with pdfkit 0.8.7 it reverts to inline. Is the idea here that we should be using lowercase headers from now on?

[vladimir-mencl-eresearch]

Hi @exviva ,

Yes, that's correct, the code now checks for and updates the content-disposition header under the lower-case name content-disposition.

If you set that from your app, the easiest would be to update the app to set the header lowercase.

I made this PR because Rack 2.x apps do not set the content-type header explicitly and Rack 2.x implicitly sets it as Content-Type.

While the trick I used hear could also be extended to Content-Disposition, if you control it from within the app, it might be easiest to change it in the app.

Please let me know if it works for you.

Cheers,
Vlad

[exviva]

Gotcha, thanks a lot @vladimir-mencl-eresearch!