Skip to content

Conversation

@pk-work
Copy link
Contributor

@pk-work pk-work commented Sep 19, 2025

Motivation:

At the moment it is not possible to use media types that are not included in the static supported mediatypes list in requests and responses. This forces the users to use generic media types where more specific ones exist in their openapi specs.

This pull-request replaces the current static list of mediatypes with a registry that allows the registration of additional mediatypes.

@pk-work
Copy link
Contributor Author

pk-work commented Sep 19, 2025

Hi @lukasjelonek I have made some modifications based on your PR. I Also added missing tests and missing copyright headers.

Could you please check if this is fine for you, especially the parts you are holding the copyright?

Tomorrow I will do some last changes, so that we don't longer use the already deprecated MediaType constants in MediaType.

Thanks for your great contribution.

@lukasjelonek
Copy link
Contributor

Hi,

I checked your code and identified the following code changes:

  • Replaced MediaTypeRegistration.create factory method with the DefaultMediaTypeRegistration type
  • Removed the access to the MediaTypeRegistry via the public OpenAPIContract.getMediaTypeRegistry method. Instead a non-public method was added to MediaTypeImpl that returns the media-type specific MediaTypeRegistration. This requires that the BaseValidator casts the MediaType object to the specific implementation instead of using the public api.
  • Replaced MediaTypeRegistry.createContentAnalyzer with get#MediaTypeRegistration, which delegates the content analyzer creation to the clients of the api
  • Removed usage of MediaTypeInfo in favor of plain strings in the public api
  • Removed the MediaTypeException when the client requests an unsupported media type in favor of client side null-handling

Here are my comments:

  1. In its original design the MediaTypeRegistry was the main object the users of the api should interact with. The setup client by adding MediaTypeRegistrations and the validators by requesting ContentAnalyzers. Now the responsibility to create ContentAnalyzers has moved to the validators by exposing the MediaTypeRegistrations to the validators. What was your intetion to pull this responsibility out of the registry?

  2. The MediaTypeRegistration.canHandle#MediaTypeInfo method signature was changed to #String. Was was your intention with that?

Besides these two points I am fine with the changes. And for these two points it might be sufficient to clarify the "why".

@pk-work
Copy link
Contributor Author

pk-work commented Sep 22, 2025

1:

  • When validating the OpenAPIContract, it is already checked whether there is a suitable MediaTypeRegistration for the media type. If not, an error is thrown. If it is already determined at this point which registration will be used, why should we then search for it again in every request? That consumes a lot of performance.

  • getMediaTypeRegistry() was annotated with @GenIgnore. This is bad because it is a public API and would be needed. With the current solution, this method is no longer needed. This has two advantages:

    1. The @GenIgnore problem is solved.
    2. Since the registry does not allow "unregister" and all necessary registrations must already be registered when the contract is created, I question the purpose of having a getter for the registry.
  • I also don't like casting to MediaTypeImpl , but due to the points mentioned above, I decided to go with it.

2:
There were two places where canHandle was called. At both places, a MediaTypeInfo object was created from a string and then passed to canHandle. Now, the string is directly passed and then cast to MediaTypeInfo at a central location.

I hope this makes sense.

@lukasjelonek
Copy link
Contributor

Thanks for the explanation. That is fine for me.

Co-authored-by: Pascal Krause <[email protected]>
Signed-off-by: Pascal Krause <[email protected]>
@pk-work pk-work merged commit 7e237cc into main Sep 23, 2025
8 of 9 checks passed
@pk-work pk-work deleted the mediatype branch September 23, 2025 19:32
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.

3 participants