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

Be more defensive in UrlResource about cleaning the path [SPR-17198] #21732

Closed
spring-issuemaster opened this Issue Aug 20, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Aug 20, 2018

Dave Syer opened SPR-17198 and commented

This method from UrlResource throws UnsupportedFeatureError in GraalVM:

private URL getCleanedUrl(URL originalUrl, String originalPath) {
     try {
          return new URL(StringUtils.cleanPath(originalPath));
     }
     catch (MalformedURLException ex) {
          // Cleaned URL path cannot be converted to URL
          // -> take original URL.
          return originalUrl;
     }
}

So the originalUrl would have been fine, but it cannot get past this private method to read the input stream. We could be more defensive there.


Affects: 5.0.8

Reference URL: oracle/graal#623

Issue Links:

  • #21529 Initial GraalVM native images (Substrate VM) support ("is depended on by")
  • #19974 UrlResource getFilename should not contain query parameters
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 22, 2018

Dave Syer commented

PR: #1936 (verified to work with Graal AOT).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 22, 2018

Juergen Hoeller commented

A straightforward refinement for a start is to only create a new URL instance if the cleaned path actually differs from the original path. This should cover most internal use of UrlResource and avoid common Graal issues with it that way.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 22, 2018

Dave Syer commented

That would also work. I can change the PR if you want.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Aug 22, 2018

Juergen Hoeller commented

Ha, seems we posted at exactly the same time there ;-) Got the change locally working already, let's see how far we get with it (to be pushed ASAP)... I quite like it as a general optimization, avoiding the repeated URL parsing effort. I was experimenting with handling the cleaned path as a String but unfortunately that causes some regressions due to special handling in URL.equals, so the "original URL as far as possible" approach sounds like the best compromise.

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