Allow custom content type for JSON body by mgirlich · Pull Request #190 · r-lib/httr2

mgirlich · 2023-01-09T07:54:47Z

Fixes #189. Fixes #284.

This PR doesn't support parameters in the media type. They could be added but I never encountered them and I'm not sure the extra complexity is worth it.
For req_body_form() it only allows application/x-www-form-urlencoded. I'm happy to add support for the more general form similar to JSON but I'm not sure it's worth it.

R/req-body.R

tests/testthat/_snaps/resp-body.md

tests/testthat/_snaps/req-body.md

mgirlich · 2023-01-10T13:36:18Z

I thought it's worth to properly refactor this into some helpers for the content type.

tests/testthat/_snaps/resp-body.md

hadley · 2023-01-10T13:46:57Z

Can you take a look at the CI failures please?

mgirlich · 2023-01-10T15:02:03Z

Ah, it seems the names of capture groups are only properly supported for recent R versions. Fixed now 😄

R/content-type.R

hadley · 2023-01-17T14:39:02Z

R/resp-body.R

  check_response(resp)
  check_installed("jsonlite")
-  check_content_type(resp,
+  check_resp_content_type(resp,


Why can the suffix arguments go away now?

It isn't needed as the suffix can be determined from the types argument.

I think that only coincidentally works for json and xml, but isn't generally true: https://www.iana.org/assignments/media-type-structured-suffix/media-type-structured-suffix.xhtml.

I don't see a counter example there. Maybe I misunderstood.
There is also an extensive list of media types.

Do you think any of these cases should be treated differently?

devtools::load_all("~/GitHub/httr2/") #> ℹ Loading httr2 # anything with suffix `+cbor` should be valid check_content_type("application/cbor", "application/cbor") #> NULL check_content_type("application/ace+cbor", "application/cbor") #> NULL check_content_type("application/ace", "application/cbor") #> Error: #> ! Unexpected content type 'application/ace' #> ℹ Expecting 'application/cbor' or 'application/<subtype>+cbor' #> Backtrace: #> ▆ #> 1. └─httr2:::check_content_type("application/ace", "application/cbor") #> 2. └─rlang::abort(...) at httr2/R/content-type.R:68:2 # if `valid_type` has <subtype>+<suffix> the content type must have the same structure check_content_type("application/ace+cbor", "application/ace+cbor") #> NULL check_content_type("application/ace", "application/ace+cbor") #> Error: #> ! Unexpected content type 'application/ace' #> ℹ Expecting 'application/ace+cbor' #> Backtrace: #> ▆ #> 1. └─httr2:::check_content_type("application/ace", "application/ace+cbor") #> 2. └─rlang::abort(...) at httr2/R/content-type.R:68:2 check_content_type("application/cbor", "application/ace+cbor") #> Error: #> ! Unexpected content type 'application/cbor' #> ℹ Expecting 'application/ace+cbor' #> Backtrace: #> ▆ #> 1. └─httr2:::check_content_type("application/cbor", "application/ace+cbor") #> 2. └─rlang::abort(...) at httr2/R/content-type.R:68:2

^{Created on 2023-01-18 with reprex v2.0.2}

As far as I can see from reading the spec, there's no relationship between the subtype and the suffix, except that there happens to be both subtypes and suffixes for xml and json.

IOTW this is incorrect because there's no +plain suffix:

check_content_type("application/cbor", "text/plain") #> Error: #> ! Unexpected content type 'application/cbor' #> ℹ Expecting 'text/plain' or 'text/<subtype>+plain'

I'm not quite sure I can follow. As far as I understand there are two cases:

content type has no suffix: e.g. application/json or application/xml. The subtype json resp. xml tells you that the body is in JSON resp. XML format and can therefore be parsed accordingly.

content type has a suffix: e.g. application/vnd.api+json has the suffix json. This tells you that the content is in JSON format. The subtype vnd.api tells you that the JSON has a specific structure.

This is also what Wikipedia says about this:

Suffix is an augmentation to the media type definition to additionally specify the underlying structure of that media type, allowing for generic processing based on that structure and independent of the exact type's particular semantics. Media types that make use of a named structured syntax should use the appropriate IANA registered "+"suffix for that structured syntax when they are registered.

and this is how I understood section 4.2.8. Structured Syntax Name Suffixes and 4.11. Fragment Identifier Requirements of RFC6838 (though I find that a bit difficult to understand).

Therefore I would argue that check_content_type("application/vnd.api+json", "application/json") should work (the actual content type is more specific than the demanded one) but check_content_type("application/json", "application/vnd.api+json") should not work (the actual content type is less specific than the demanded one).

Does this make sense to you or do you see other issues here?

I think you're pre-supposing a relationship between type and suffix that doesn't exist, and it would be better to keep suffix as a separate argument. In practice, I don't think this will affect how the code works, but I don't like writing a function that implies a structure that I don't believe to be true.

But I don't think I'm doing a good job of explaining exactly what I mean here, so would you mind just trusting me and switching back to an explicit suffix argument? I think it's a pretty simple change.

I'm happy to go back to suffix. But I'm not quite sure what the interface should like like in the end. I think there are three types of questions on the content type:

Is the content in JSON format?

Is the content type application/json?

Is the content type application/vnd.api+json?

Is the content in JSON format? Is the content type application/json? Is the content type application/vnd.api+json?

application/cbor no no no

example/json yes no no

application/json yes yes no

application/test+json yes yes no

application/vnd.api+json yes yes yes

Do you agree that these three questions all make sense? And what do you think the interface should look like then?

Let take a stab at this PR and give you something to look at.

…into req_body_json-type

R/resp-body.R

#Conflicts: # NEWS.md # tests/testthat/_snaps/req-error.md

jonthegeek · 2023-08-25T12:13:58Z

It looks like the only remaining issue in this is a slight conflict in NEWS.md.

#Conflicts: # NEWS.md

hadley · 2023-08-26T01:07:25Z

R/req-body.R


-  # Must set header afterwards
+  # Respect existing Content-Type if set
+  type_idx <- match("content-type", tolower(names(req$headers)))


I've changed the logic a bit here — now if you manually override with the content-type header, we don't check it. I think this is ok, and gives the user an escape hatch if they're having problems. But generally we expect people to set the content type via the appropriate req_body_* function, which will verify that the type is as expected.

R/content-type.R

hadley · 2023-08-26T01:12:07Z

R/content-type.R

+#'
+#' @param resp A response object.
+#' @param valid_types A character vector of valid content types.
+#' @param valid_suffix A string given an "structured media type" suffix.


This keeps the type and suffixes orthogonal in a way I think matches the spec.

hadley · 2023-08-26T01:15:01Z

@mgirlich let me know what you think of this approach.

#Conflicts: # NEWS.md # R/resp-body.R # tests/testthat/_snaps/req-body.md # tests/testthat/_snaps/req-error.md # tests/testthat/_snaps/resp-body.md

mgirlich · 2023-09-06T11:22:13Z

I find it a bit confusing that the following works:

check_content_type(
  "audio/abc+json",
  valid_types = "application/json",
  valid_suffix = "json"
)

If we want to allow this then valid_types should be an optional argument.

Another really confusing example is

check_content_type("application/test+json", "application/test+json")

This would suggest that we should validate the valid_types argument or that maybe the interface isn't yet fully right.

mgirlich · 2023-09-06T11:23:19Z

R/content-type.R

+  # ```
+  stopifnot(length(x) == 1)
+  regex <- "^(?<type>application|audio|font|example|image|message|model|multipart|text|video)/(?<subtype>(?:(?:vnd|prs|x)\\.)?(?:[^+;])+)(?:\\+(?<suffix>(?:[^;])+))?(?:;(?<parameters>(?:.)+))?$"
+  if (!grepl(regex, x, perl = TRUE)) {


Maybe this should be an error instead? And we might just have a special handling when the content type is NULL or the empty string?

hadley · 2023-09-06T18:15:05Z

Oh yeah, we can definitely make valid_types optional. I don't think it's worth validating it; but I can document it and fix that test.

Allow custom content type for JSON body

586c6e3

hadley reviewed Jan 9, 2023

View reviewed changes

R/req-body.R Outdated Show resolved Hide resolved

R/req-body.R Outdated Show resolved Hide resolved

mgirlich added 3 commits January 9, 2023 14:27

Refactor check_content_type()

2865783

Add comment on check

16b560e

Add test for <subtype>+<suffix> form

bab4aa8

mgirlich commented Jan 9, 2023

View reviewed changes

tests/testthat/_snaps/resp-body.md Outdated Show resolved Hide resolved

Use margrittr pipe

911f20b

hadley reviewed Jan 9, 2023

View reviewed changes

tests/testthat/_snaps/req-body.md Outdated Show resolved Hide resolved

mgirlich added 5 commits January 9, 2023 14:55

Replace another base pipe

22cea4b

Update snapshots

261a1fe

Refactor content type checking

27c6b23

Add test for check_type = FALSE

2edba49

Remove superfluous test

743584c

mgirlich commented Jan 10, 2023

View reviewed changes

tests/testthat/_snaps/resp-body.md Outdated Show resolved Hide resolved

mgirlich added 4 commits January 10, 2023 13:59

More carefully check if there is no match

2672373

Merge commit '65b9f279d8ca12da818da298b433bbeabc59b26b'

6330e47

Try without using names of capture groups

40cf4da

Fix incorrect indices

993e01b

Merge branch 'main' into req_body_json-type

bb7e798

hadley reviewed Jan 17, 2023

View reviewed changes

mgirlich added 2 commits January 17, 2023 15:06

Add link for types

2f45570

Merge branch 'req_body_json-type' of https://github.com/mgirlich/httr2 …

6fa9ecf

…into req_body_json-type

hadley reviewed Feb 8, 2023

View reviewed changes

R/resp-body.R Outdated Show resolved Hide resolved

mgirlich added 3 commits August 3, 2023 06:10

Merge commit '817ca328d652c52727cbd655d5e021ecffc3975c'

58da9ff

#Conflicts: # NEWS.md # tests/testthat/_snaps/req-error.md

Export check_resp_content_type()

c549f55

Add to pkgdown yaml

544a4b8

hadley added 6 commits August 25, 2023 19:08

Eliminate helper

dd3edbb

Merge commit '970626ca902dceb453d5cffbc590cd0990e33af1'

c979b5a

#Conflicts: # NEWS.md

Add some input verification

3ca1e6a

Keep suffix and type orthogonal

2b618f3

Tweak errors/tests/news

ddbc197

Set content type with a parameter

816ba42

hadley reviewed Aug 26, 2023

View reviewed changes

Fix news

4a7ae05

hadley reviewed Aug 26, 2023

View reviewed changes

R/content-type.R Outdated Show resolved Hide resolved

Tweak type docs

00302f2

hadley reviewed Aug 26, 2023

View reviewed changes

hadley added 7 commits August 27, 2023 13:50

Add test for missing content type

6b49f6b

Merge commit 'bc8b02a2406fd81365c557ac50e186b08d2ac526'

9c4fbd1

#Conflicts: # NEWS.md # R/resp-body.R # tests/testthat/_snaps/req-body.md # tests/testthat/_snaps/req-error.md # tests/testthat/_snaps/resp-body.md

Use cli_abort()

1785a9a

Add full stops

434e323

Rename function

a9af8bc

Remove comma

c79df44

Update news bullet

0dc7430

mgirlich commented Sep 6, 2023

View reviewed changes

Feedback from code review

221674a

hadley merged commit 9c60bd2 into r-lib:main Sep 6, 2023

mgirlich deleted the req_body_json-type branch September 7, 2023 08:09

	Is the content in JSON format?	Is the content type `application/json`?	Is the content type `application/vnd.api+json`?
`application/cbor`	no	no	no
`example/json`	yes	no	no
`application/json`	yes	yes	no
`application/test+json`	yes	yes	no
`application/vnd.api+json`	yes	yes	yes

Conversation

mgirlich commented Jan 9, 2023 • edited by hadley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgirlich commented Jan 10, 2023

Uh oh!

Uh oh!

hadley commented Jan 10, 2023

Uh oh!

mgirlich commented Jan 10, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonthegeek commented Aug 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hadley Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hadley commented Aug 26, 2023

Uh oh!

mgirlich commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hadley commented Sep 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mgirlich commented Jan 9, 2023 •

edited by hadley

Loading

hadley Aug 26, 2023 •

edited

Loading

mgirlich commented Sep 6, 2023 •

edited

Loading