FasterXML / jackson-jaxrs-providers

Multi-module project that contains Jackson-based "old" JAX-RS (ones under `javax.ws.rs`) providers for JSON, XML, YAML, Smile, CBOR formats
Apache License 2.0
109 stars 77 forks source link

Fix errant “accept anything” handling of providers #162

Open kdubb opened 1 year ago

kdubb commented 1 year ago

When hasMatchingMediaType returns true by default, it is impossible to have a catch-all provider, or even a sensible and predicatable error, when the response doesn’t have a content-type.

kdubb commented 1 year ago

It would great to consider this as a "bug" and consider backporting these changes.

Working with multiple formats and detecting an unsupported or errant format is impossible the way it currently is.

cowtowncoder commented 1 year ago

Ok. I don't think this can be backported due to very likely compatibility issues: some users are likely to rely on default behavior. But even for going forward it seems like there should be an option for configuring settings.

Timing-wise, either way I just released the last RC for 2.14 so this will have to wait until 2.15, unless there was a way to make it so that:

  1. There's a config setting that allows toggling behavior, AND
  2. Initial setting for 2.14 is the old behavior and user needs to opt-in to get new and improved handling.

It might make sense to bring this up on "jackson-dev" google group since while description makes sense, I think I'd like others to voice their opinions. Especially considering that this setting has been there for past 8+ years or so... meaning I don't think it can be exactly obvious flaw.

kdubb commented 1 year ago

Wether or not it can be back ported or may be relied upon for current users is a debate to be had for sure, but I have to disagree with it not being an obvious flaw.

The entire point of MessageBodyReader.isReadable (which flows through to hasMatchingMediaType is to check if the body can be read by this instance. By returning true when there is no media type it just reads whatever data there is as if it knows it's the correct type.

This becomes a big issue when supporting multiple formats. For example we support JSON and CBOR. As it stands if no media type is present, whatever format is queried first is assumed to be the format and you generally get a nondescript error. We added our own catch all (i.e. */*) to catch this and it is never called because the Jackson providers also use */* and then return true for isReadable so they consume the body before our catchall is queried.

cowtowncoder commented 1 year ago

I am not arguing so much about it not being problematic. But from purely pragmatic viewpoint I am surprised no one else considered this problematic enough to report a bug.

And I would be guessing that this is considered a "feature, not bug" by some developers who want defaulting and do not support multiple media types. Once again I am not arguing this is proper way to think about things (it is sloppy) but that change here would cause breakage for anyone relying on current behavior.