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

Add URLEqualHashCode Recipe #251

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

JLLeitschuh
Copy link
Contributor

Based upon, but not completely fixing this: https://errorprone.info/bugpattern/URLEqualsHashCode

Signed-off-by: Jonathan Leitschuh [email protected]

What's changed?

Adds a recipe URLEqualsHashCode.

What's your motivation?

I was curious about trying out the new template recipe generation. This seemed like a fun way to try it out.

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

Are there any use cases/tests that I likely missed?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Thought about writing the recipe not using the template engine, but this seemed like a good use case.

Also, the reason I'm using URI#toString to create a URI, instead of URL#toURI is because URL#toURI throws a checked exception

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@knutwannheden
Copy link
Contributor

Interesting. I suppose there are also legitimate uses (like file URLs). Otherwise any HashSet or HashMap would also be problematic. In this case the URLs are known to be for files: https://github.com/jetty/jetty.project/blob/5d9679adf861bc589fdb213758a88f16436f6286/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/MetaInfConfiguration.java#L660

@JLLeitschuh
Copy link
Contributor Author

Interesting insight. Not sure if there's a great way to detect that. In general, this could be an optionally applied recipe.

Dealing with collections could be super complicated, especially when the collection is returned/escapes the scope of the method.

@JLLeitschuh JLLeitschuh force-pushed the feat/JLL/url-equals-hashcode branch 2 times, most recently from 4114518 to 4d7dd97 Compare January 29, 2024 01:50
@JLLeitschuh
Copy link
Contributor Author

Is this good to merge?

description = "Uses of equals() and hashCode() cause java.net.URL to make blocking internet connections. " +
"Instead, use java.net.URI.",
tags = {"errorprone"}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add some Markdown formatting here. Also, I don't think the errorprone tag is very useful. Does Errorprone have a check of this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://errorprone.info/bugpattern/URLEqualsHashCode

I'll add the markdown formatting. Good point. 😄

Based upon, but not completely fixing this: https://errorprone.info/bugpattern/URLEqualsHashCode

Signed-off-by: Jonathan Leitschuh <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

I must have missed this PR the first time around; are we still looking to add it? Approved from my end if so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Ready to Review
Development

Successfully merging this pull request may close these issues.

3 participants