[SPARK-49240][BUILD] Add `scalastyle` and `checkstyle` rules to avoid `URL` constructors by dongjoon-hyun · Pull Request #47762 · apache/spark

dongjoon-hyun · 2024-08-14T22:04:12Z

What changes were proposed in this pull request?

This PR aims to add scalastyle and checkstyle rules to avoid URL constructors.

Why are the changes needed?

The java.net.URL class does not itself encode or decode any URL components according to the escaping mechanism defined in RFC2396.

So, from Java 20, all URL constructors are deprecated. We had better use better URI class.

https://bugs.openjdk.org/browse/JDK-8295949

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the CIs with newly added rules.

After this PR, there is only two exceptional instances in JettyUtils.scala and UISuite.scala.

JettyUtils is tricky instance
UISuite test case is supposed to add bad URL which URI prevents with java.net.URISyntaxException. This is an example why URI is better. In this PR, we keep the old, URL class, to keep the test coverage.

$ git grep -C1 'new URL('
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala-        // scalastyle:off URLConstructor
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:        val newUrl = new URL(requestURL, prefixedDestPath).toString
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala-        // scalastyle:on URLConstructor
--
core/src/test/scala/org/apache/spark/ui/UISuite.scala-      // scalastyle:off URLConstructor
core/src/test/scala/org/apache/spark/ui/UISuite.scala:      val badRequest = new URL(
core/src/test/scala/org/apache/spark/ui/UISuite.scala-        s"http://$localhost:${serverInfo.boundPort}$path/root?bypass&invalid<=foo")

Was this patch authored or co-authored using generative AI tooling?

No.

dongjoon-hyun · 2024-08-14T22:07:59Z

This is a little tricky to change. So, this PR allows this as the only exception.

val requestURI = new URI(request.getRequestURL.toString) val newUri = requestURI.resolve(prefixedDestPath).toString response.sendRedirect(newUri)

Will there be any problems with this change?

Let me check. Thanks, @LuciferYang .

To @LuciferYang , I checked multiple corner cases. And, it's not the same in some cases like the following. I'd prefer to keep the existing logic to avoid any risk here.

$ jshell | Welcome to JShell -- Version 21.0.4 | For an introduction type: /help intro jshell> new URL(new URL("https://a"), "?index.html") $1 ==> https://a/?index.html jshell> new URI("https://a").resolve("?index.html") $2 ==> https://a?index.html

To be clear, this PR aims to prevent any future addition of new URL( instead of removing everyone instances of new URL(.

dongjoon-hyun · 2024-08-14T22:13:20Z

Could you review this when you have some time, @LuciferYang ?

… `URL` constructors

…on correctly

dongjoon-hyun · 2024-08-15T00:46:07Z

      // Try a request with bad content in a parameter to make sure the security filter
      // is being added to new handlers.
+      // scalastyle:off URLConstructor
      val badRequest = new URL(


To keep the existing test coverage, we need to keep URL because it doesn't complain while URI throws URISyntaxException.

[info] - add and remove handlers with custom user filter *** FAILED *** (92 milliseconds) [info] java.net.URISyntaxException: Illegal character in query at index 47: http://127.0.0.1:54129/test/root?bypass&invalid<=foo

dongjoon-hyun · 2024-08-15T02:11:27Z

Thank you, @HyukjinKwon . I excluded one more instance to keep the existing test coverage.

LuciferYang · 2024-08-15T02:31:50Z

@@ -351,7 +351,7 @@ private class TestMasterInfo(val ip: String, val dockerId: DockerId, val logFile
  def readState(): Unit = {
    try {
      val masterStream = new InputStreamReader(


not related to this PR, but it seems that masterStream has not been closed.

LuciferYang · 2024-08-15T02:52:03Z

val requestURI = new URI(request.getRequestURL.toString) val newUri = requestURI.resolve(prefixedDestPath).toString response.sendRedirect(newUri)

Will there be any problems with this change?

LuciferYang · 2024-08-15T02:59:29Z

    ]]></customMessage>
  </check>
+
+  <check customId="URLConstructor" level="error" class="org.scalastyle.file.RegexChecker" enabled="true">


Should this rule also be added to dev/checkstyle.xml?

It's already added by this PR, here.

spark/dev/checkstyle.xml

Lines 209 to 212 in b1af69e

<module name="RegexpSinglelineJava">

<property name="format" value="new URL\("/>

<property name="message" value="Use URI.toURL or URL.of instead of URL constructors." />

</module>

dongjoon-hyun · 2024-08-15T03:32:22Z

Could you review this once more, @LuciferYang ?

LuciferYang

LGTM

LuciferYang · 2024-08-15T04:27:53Z

Merged into master. Thanks @dongjoon-hyun and @HyukjinKwon

dongjoon-hyun · 2024-08-15T04:29:02Z

Thank you, @LuciferYang and @HyukjinKwon !

github-actions bot added SQL KUBERNETES WEB UI BUILD YARN CORE AVRO labels Aug 14, 2024

dongjoon-hyun commented Aug 14, 2024

View reviewed changes

dongjoon-hyun force-pushed the SPARK-49240 branch 2 times, most recently from d67ef0d to 5ef3706 Compare August 14, 2024 22:44

[SPARK-49240][BUILD] Add scalastyle and checkstyle rules to avoid…

3178377

… `URL` constructors

dongjoon-hyun force-pushed the SPARK-49240 branch from 5ef3706 to 3178377 Compare August 14, 2024 23:00

HyukjinKwon approved these changes Aug 15, 2024

View reviewed changes

Ignore another test case because URI raises java.net.URISyntaxExcepti…

b1af69e

…on correctly

dongjoon-hyun commented Aug 15, 2024

View reviewed changes

LuciferYang reviewed Aug 15, 2024

View reviewed changes

LuciferYang approved these changes Aug 15, 2024

View reviewed changes

LuciferYang closed this in 0c24ae1 Aug 15, 2024

dongjoon-hyun deleted the SPARK-49240 branch August 15, 2024 04:29

	<module name="RegexpSinglelineJava">
	<property name="format" value="new URL\("/>
	<property name="message" value="Use URI.toURL or URL.of instead of URL constructors." />
	</module>

Conversation

dongjoon-hyun commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Aug 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Aug 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Aug 15, 2024

Uh oh!

LuciferYang left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Aug 15, 2024

Uh oh!

dongjoon-hyun commented Aug 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dongjoon-hyun commented Aug 14, 2024 •

edited

Loading