Skip to content

Match application/json when Accept includes charset - #112

Open
gyanu2507 wants to merge 2 commits into
jshttp:masterfrom
gyanu2507:fix/accept-charset-match
Open

Match application/json when Accept includes charset#112
gyanu2507 wants to merge 2 commits into
jshttp:masterfrom
gyanu2507:fix/accept-charset-match

Conversation

@gyanu2507

@gyanu2507 gyanu2507 commented Sep 1, 2026

Copy link
Copy Markdown

Accept: application/json;charset=utf-8 listed application/json from mediaTypes(), but mediaTypes(['application/json']) returned nothing because every Accept parameter had to exist on the provided type.

charset on Accept is not a type discriminator unless the provided type also names one. application/json;charset=utf-8 still matches application/json. Conflicting charsets (utf-8 vs iso-8859-1) do not match. text/html;level=1 vs text/html is unchanged.

profile on application/ld+json is a different parameter and is not in this change.

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I found two actionable issues at exact head e855534:

  1. The fixes #62 claim is not satisfied. Issue #62's reproducer is Accept: application/ld+json;profile=http://www.w3.org/ns/json-ld#expanded with mediaType(['application/ld+json']); running this head still returns undefined because only charset is filtered and profile remains a required parameter. Please either remove fixes #62 or include the profile behavior and a regression test before claiming the issue is fixed.

  2. Filtering charset for every media type changes parameter matching in a way that can select an unacceptable representation. At this head, Accept: text/plain;charset=utf-8 with provided ['text/plain;charset=iso-8859-1'] returns the ISO-8859-1 type. Media-range parameter semantics are media-type-specific; an unconditional filter cannot safely assume that every charset parameter is irrelevant. The existing discussion in #35 and the maintainer guidance in #41 also treat charset/other parameters as type-specific. Please retain parameter matching by default (or document and scope an explicit policy) and add a regression test for conflicting charset values.

Ignoring charset on every Accept range made a utf-8 offer select an
iso-8859-1 type. Keep the application/json;charset=utf-8 case, but
require the values to agree when both sides set charset.
@gyanu2507

Copy link
Copy Markdown
Author

Both notes from the review are in c71457a.

fixes #62 is gone from the body. This PR only covers charset: Accept: application/json;charset=utf-8 still selects application/json, and Accept: text/plain;charset=utf-8 no longer selects text/plain;charset=iso-8859-1. Tests for both cases are in test/mediaType.js. profile is unchanged.

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