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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delegate parsing of media types to library #2471

Closed
wants to merge 1 commit into from

Conversation

jamietanna
Copy link
Collaborator

@jamietanna jamietanna commented Oct 31, 2023

Instead of us handling any media-type related parsing, we should instead
delegate to a library for this.


This is a reimplementation of #1686 in part to freshly recreate it to avoid the merge conflicts, but also shamelessly to see if it can count for Hacktoberfest 馃巸


private String[] parts;
private final Optional<MediaType> maybeMediaType;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has an IDE warning, but I believe it's the right thing to do to give us the ability to more easily handle the absense of the value. Thoughts? (I've not done Java in ~18 months so my knowledge is rusty!)

@jamietanna jamietanna marked this pull request as ready for review October 31, 2023 09:35
@jamietanna jamietanna requested a review from a team as a code owner October 31, 2023 09:35
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new library needs to be shaded

Instead of us handling any media-type related parsing, we should instead
delegate to a library for this.

This also shades the media-type library into the built JAR.
@jamietanna
Copy link
Collaborator Author

Thanks Oleg, I think I've done the shading right, but let me know!

@jamietanna jamietanna dismissed oleg-nenashev鈥檚 stale review November 6, 2023 08:16

Changes addressed 馃馃徑

@oleg-nenashev oleg-nenashev added enhancement needs-tom Tom's Train Project :) labels Nov 20, 2023
@tomakehurst
Copy link
Member

Sorry @jamietanna in its current state I feel like the library isn't offloading enough of the work to be worth the addition of an extra dependency.

I agree in general that it would be good to outsource generic HTTP wrangling to libraries. Ideally I'd like to have a whole HTTP model (including a better URL implementation than Java's) extracted as a library, but that's a different sized job entirely...

@jamietanna
Copy link
Collaborator Author

Hey @tomakehurst that's fair. Happy to leave this open, or close and we can look at adding in the future.

I know that one thing I was considering as a useful extension would be to allow Wiremock to perform content-negotiation - I've built some APIs in the past that will have an Accept header that is a bit awkward to statically mock up-front, but if we had i.e. negotiatesTo: "application/vnd.api+json" then it'd be possible to handle these requests more easily.

That being said, I'm not sure if it'd be that well-used - I don't see an issue raised for it, so happy to raise one and see if it's something the community would want?

@tomakehurst
Copy link
Member

I've had similar thoughts about content negotiation in the past but as you've said, there doesn't seem to be much demand for it.

I'll close this for now (in a futile effort at keeping things tidy), but happy to re-open it if we want to expand this use case a bit in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants