SeasideSt / Seaside

The framework for developing sophisticated web applications in Smalltalk.
MIT License
508 stars 71 forks source link

Fix #requestHeadersFor: on ZnZincServerAdaptor to combine field values for repeated field names #1387

Closed Rinzwind closed 8 months ago

Rinzwind commented 8 months ago

This pull request fixes #requestHeadersFor: on ZnZincServerAdaptor to combine field values for repeated header field names.

Section ‘5.2. Field Lines and Combined Field Value’ in RFC 9110 states:

When a field name is only present once in a section, the combined "field value" for that field consists of the corresponding field line value. When a field name is repeated within a section, its combined field value consists of the list of corresponding field line values within that section, concatenated in order, with each field line value separated by a comma.

See also section ‘6.3. Header Fields’.

Example:

request := String crlf join: #('GET / HTTP/1.1'
    'Accept: application/xml'
    'Accept: application/json'
    '' '').
(ZnZincServerAdaptor new requestFor: (ZnRequest readFromString: request))
    headerAt: 'accept'

Without this fix, the example returns 'application/json'. With this fix, the example returns 'application/xml,application/json'.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (80af760) 48.67% compared to head (ed0006f) 48.67%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1387 +/- ## ======================================= Coverage 48.67% 48.67% ======================================= Files 8948 8948 Lines 80512 80512 ======================================= Hits 39192 39192 Misses 41320 41320 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jbrichau commented 8 months ago

This reminds me of https://github.com/SeasideSt/Seaside/issues/1211 which the exact same issue for responses... It does show how we need unit tests for these adaptors (https://github.com/SeasideSt/Seaside/issues/1213)

Rinzwind commented 8 months ago

Reading issue #1211 makes me wonder: shouldn’t ZnZincServerAdaptor’s #requestHeadersFor: return a WAHeaderFields instead of a Dictionary? Then the separate headers can be preserved. The methods in WARequest’s ‘accessing-headers’ protocol that answer a collection should then combine them.