Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix errant “accept anything” handling of providers #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kdubb
Copy link
Contributor

@kdubb kdubb commented Oct 29, 2022

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.

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
Copy link
Contributor Author

kdubb commented Oct 29, 2022

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
Copy link
Member

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
Copy link
Contributor Author

kdubb commented Oct 29, 2022

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
Copy link
Member

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.

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.

None yet

2 participants