Fix errant “accept anything” handling of providers

[kdubb]

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]

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]

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]

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]

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.

[kdubb]

@cowtowncoder Given that #194 was merged and to some extent tightens up this behavior. Is it time to get this fix in to?

[kdubb]

FYI... we use the variant feature quite a bit to switch between JSON/YAML/CBOR and in my experience it is the JAX-RS implementation that should be selecting a "default". This generally allows more nuanced decision making; additionally, major frameworks seem to already handle this.

For example, Quarkus & Spring select JSON (by default or configuration) if no MB(Reader|Writer) returns true from isReadable/isWritable (which flows to the methods here). In the case of Quarkus, logic based on the type being serialized/deserialized to choose that default, choosing appropriate body readers/writers when for SSE, binary, etc.

In general, adding this modules just breaks all of that specialized handling because Jackson has effectively committed to handling "everything" in all of its providers.

[kdubb]

Another point of information your decision is, this isn't easy to fix, even when you known there's a problem.

We handle this now but deriving our own YAMLProvider and CBORProvider like so (please excuse the Kotlin):

@Provider
@Produces(WILDCARD) // Allow handling of suffixed types (e.g. `+cbor`, `+yaml` 
@Consumes(WILDCARD) // Wildcard is not an issue when isReadable/isWritable return proper values
@Priority(Priorities.USER)
class YAMLProvider : JacksonYAMLProvider {
  override hasMatchingMediaType(MediaType mediaType) {
    // Check for application/yaml and `+yaml` and then let Quarkus/JAX-RS decide.
    return false
  }
}

Unfortunately this doesn't work, after a bit of testing you will conclude you need two things

• Elevated Priority via @Priority(Int.MAX_VALUE)
  This ensures your provider gets called before Jackson's and returns false as appropriate.
• New MB(Reader|Writer) implementations that stop Jackson's WILDCARD handling
  These are needed to ensure any other providers Jackson provides don't get a chance to misreport that they should be used for missing/wildcard return types.

All in all, this can be quite a nuisance and hard to keep correctly working. Quarkus has had major changes to their Reactive JAX-RS that are tripped up by this and our code must adapt accordingly; thankfully proper testing ensures we are able to detect these problems.

[cowtowncoder]

@kdubb Apologies for super-slow follow-up, but I am back and I agree with your assessment: change is desirable.

So as to how/where to make the change, I think this should go in the next 2.x minor release, 2.19. So target branch should be 2.19 (easier to merge forward).
And for 2.x (but not necessarily 3.0/master), I think we need a new JaxRSFeature to determine behavior; defaulting to NEW (arguably correct feature), but allowing change to former behavior. Usually we would do it the other way around, but I think this is exceptional case where I think old behavior really is broken (like you said).

So: would it be possible to create a new PR targeting 2.19, adding new com.fasterxml.jackson.jaxrs.cfg.JaxRSFeature constant (not sure what to call it, you can propose something), defaulting in such a way to use the new correct behavior but allowing changing to old (2.18 and earlier) wide matching?

If so, would be happy to get this merged & I can handle rolling forward to master/3.0.

[kdubb]

@cowtowncoder I'd like to pick this back up. If so, it is too late for 3.0?

[kdubb]

It seems this affects both jaxrs and Jakarta (unless that module was updated accordingly).

[cowtowncoder]

@kdubb Not too late for 3.0. But also doable for 2.20 if you wanted to add a feature.

[kdubb]

Ok. I'll eek out some time to tackle the next few days!

[cowtowncoder]

It seems this affects both jaxrs and Jakarta (unless that module was updated accordingly).

I think it does affect both, yes.

[cowtowncoder]

Alas, we did miss 3.0.0...

[cowtowncoder]

This could now be done for 3.1.0 tho -- but time running short, 3.1.0-rc1 to be released soon (maybe as soon as tomorrow).

[cowtowncoder]

@kdubb WDYT? I am hoping to get this (and matching one for Jakarta-RS repo) done ASAP, to make it in 3.1.0-rc1 (to get some more testing -- plus 3.1.0 will be the new baseline, LTS)

[cowtowncoder]

Matching PR for Jakarta-RS providers: FasterXML/jackson-jakarta-rs-providers#65

[kdubb]

@cowtowncoder Thanks for getting to this. I have had it on my task list but have been absolutely swamped.

I'm not 100% sure wether this is the complete fix.

Our working solution was to derive new providers from JacksonCBORProvider and JacksonYAMLProvider and remove the WILDCARD (which seemingly still exists after #194) and replace it with application/yaml|cbor. Although, that might because of the priority issue we had to overcome.

Honestly the best solution would be to be specific about what can be consumed and produced. Basically, for example with yaml, make @Consumes and @Produces both be {"application/yaml", "*/*+yaml"} and the same for CBOR/XML/etc; I'm not totally sure the */*+yaml is matched by JAX-RS because we only ever need application/yaml.

The real gotcha is that any provider that gets installed from this package, which all consume WILDCARD, will take over the output. So even if we derive new CBOR and YAML providers and tighten the Consumes/Produces, and XML is included (even though we don't use it)... we're bitten by this. That required us to create MessageBodyReader|Writer classes to make sure the other providers were never allowed to take over.

[cowtowncoder]

@kdubb Ok, I think the question here is whether this change is at least a step forward.

[cowtowncoder]

Ah. I see.. the big(ger) problem is

@Consumes(MediaType.WILDCARD)

(and to some degree, @Produces, although bit more specific))

because that'll "hi-jack" all content already so by the time method is called damage is already done, possibly (depending on how framework deals with multiple matches).

But changing those annotations carries a big risk of breaking lots of users who rely on broad matching...

Yeah, this is a mess.

I am just wondering if this PR's defaults should be changed to make it opt-in.

[kdubb]

@cowtowncoder I really wish I had made time to get this into 3.0 because it's such a breaking change already. That being said, removing the "hi hack" (haha) probably isn't the broad breaking change due to the limited number of apis that actually use content negotiation; I would guess that 90+% of apps use this for "matching" not supporting it as a true "request for output content-type".

[cowtowncoder]

FWTW there are couple of unfortunate but necessary behavioral changes in 3.1 over 3.0 -- to try to make 3.1 true 3.x baseline -- so it's ok.

And 3.1.0-rc1 has been out for... 2 weeks? ... no reports, but then again RCs don't get much testing. We'll see. :)