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

Simplify MediaType.parse Calls #404

Merged
merged 6 commits into from
Aug 19, 2023
Merged

Conversation

AlekSimpson
Copy link
Contributor

@AlekSimpson AlekSimpson commented Aug 3, 2023

What's changed?

This PR introduces a recipe to replaces uses of MediaType.parse("application/json") with MediaType.APPLICATION_JSON.

What's your motivation?

Fixes #337

Anything in particular you'd like reviewers to focus on?

Currently when the recipe runs the methodType for MediaType.parse is null which is causing the recipe to stop running early. I think it is because I am not including the correct classpath. I have tried using classpath("spring-web") but it kept telling me that this was not a valid resource name. I'm confused why it would tell me that since I've seen spring-web used in other tests in rewrite-spring.

Anyone you would like to review specifically?

@timtebeek

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@AlekSimpson AlekSimpson added the recipe Recipe requested label Aug 3, 2023
@AlekSimpson AlekSimpson self-assigned this Aug 3, 2023
@AlekSimpson AlekSimpson marked this pull request as draft August 3, 2023 20:44
@AlekSimpson AlekSimpson linked an issue Aug 3, 2023 that may be closed by this pull request
@joanvr
Copy link
Contributor

joanvr commented Aug 18, 2023

The reason why it's returning null is because the method parse(String) does not actually exist in MediaType. I understand that you are using this method name because of the original issue, but if you check at the docs it seems that the method you are looking for is called parseMediaType(String) or maybe valueOf(String), probably both... if you take a look at the code it calls the first one.
Maybe it's a typo? Or the API just changed at some point? I could not find any reference to MediaType.parse(String) from Spring in a fast google search.

P.D.: Also added spring-core in the classpath of the test, since it's needed as a dependency of spring-web for properly parsing org.springframework.http.MediaType

Comment on lines 50 to 51
if ("application/json".equals(test.getValue())) {
return JavaTemplate.builder("MediaType.APPLICATION_JSON")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is perhaps too limiting; we already have a recipe to replace literals with constants:

A comment there was logged as an improvement to pick up cases where we (after the above recipe) pass a constant into a parse method:

If you ask me this implementation should detect String constants passed into parse, and replace those with the pre existing MediaType constants. Open to discuss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so just to make sure I understand this correctly instead of detecting something like "application/json" like we are doing now; We should instead detect constants and make the replacement then?

Are there are other MediaType constants this recipe should try covering also? Instead of just looking for APPLICATION_JSON?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes when the value feeding into the parse call is a constant defined on MediaType then you want to replace the whole method invocation with the sister constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

so

-MediaType.parseMediaType(MediaType.APPLICATION_JSON_VALUE)
+MediaType.APPLICATION_JSON

notice how we drop the method call, and replace it with a "sister constant" that lacks the _VALUE suffix.

Copy link
Contributor

Choose a reason for hiding this comment

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

the above is from memory, so be sure to check what's available on the MediaType class in your IDE, but that's the rough outline of what I would expect. And we should not limit ourselves to APPLICATION_JSON, instead just any constants that follow that pattern (without naming each explicitly).

@AlekSimpson
Copy link
Contributor Author

AlekSimpson commented Aug 18, 2023

Ok I updated this recipe to work not just with APPLICATION_JSON_VALUE but also with the other constants that MediaType has. It also was modified to only with constants and not literals.

@timtebeek
Copy link
Contributor

Make a small change not to use a hardcoded list of variables, instead using the convention that if there's a String constant defined on MediaType that there's a matching constant that lacks the _VALUE suffix. We also don't need to use JavaTemplate for such a replacement.

@timtebeek timtebeek marked this pull request as ready for review August 19, 2023 09:42
@timtebeek timtebeek merged commit 8a56300 into main Aug 19, 2023
1 check failed
@timtebeek timtebeek deleted the alek/SimplifyMediaTypeParseCalls branch August 19, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Simplify unnecessary MediaType.parse calls
3 participants