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

Use URLStreamHandler for classpath protocol if available #250

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

pbrant
Copy link
Member

@pbrant pbrant commented Jan 3, 2024

No description provided.

@pbrant pbrant merged commit d650b6c into main Jan 3, 2024
1 check passed
@pbrant pbrant deleted the use-classpath-urlstreamhandler branch January 3, 2024 16:16
Copy link
Contributor

@asolntsev asolntsev left a comment

Choose a reason for hiding this comment

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

@pbrant Sorry, but I don't understand how it can help.
For me, the line return result.toURL().toString(); throws java.net.MalformedURLException: unknown protocol: classpath, and gets to the catch block.

Also, I see that org.xhtmlrenderer.pdf.ConcurrentPdfGenerationTest.samplePdf() fails without my change (and URLStreamHandler doesn't help with it).

@pbrant
Copy link
Member Author

pbrant commented Jan 3, 2024

No handler for the classpath protocol will be available unless the JRE is configured to make one available.

See e.g. https://docs.oracle.com/javase/8/docs/api/java/net/URL.html#URL-java.lang.String-java.lang.String-int-java.lang.String- for more information.

Based on Andrzej's description, Spring Boot (or maybe Spring itself) provides a handler for the classpath protocol.

This change provides backwards compatibility for those environments (Spring or otherwise) while falling back to the new implementation.

As an aside, I strongly suspect the Spring implementation is more defensive than the FS code. The context class loader cannot always be relied upon to return something useful (e.g. it could be null or set to something unhelpful). The FS code should be useful in many situations though.

@asolntsev
Copy link
Contributor

@pbrant Thank you for the explanation.
Now I understand why URLStreamHandler might work. But I still don't understand why searching of this file in classpath didn't work. It should always work if the file is located in classpath.

@asolntsev asolntsev added this to the 9.4.1 milestone Jan 7, 2024
@asolntsev
Copy link
Contributor

@pbrant Juhuu, I managed to reproduce the issue in a Spring project.
Given a Spring boot project

  • with a file src/main/resources/templates/selenide-logo-big.png.
  • and a line in html template: <img src="classpath:templates/selenide-logo-big.png"/>

It works with all FS versions: 9.3.0, 9.4.0, 9.4.1.
The generated PDF contains the image.

NB!
The above mentioned error happens ONLY if I add a leading slash to image url: <img src="classpath:/templates/selenide-logo-big.png"/>. Such URL fails with FS 9.4.0.

But I think it's just an invalid url. Users just need to remove the leading slash.

@pbrant
Copy link
Member Author

pbrant commented Jan 9, 2024

Hey, nice detective work!

My reading of RFC 3986 is that both variants are perfectly legal URIs.

In any case, the larger point is that, as a general rule, we should be quite tentative about breaking existing, working input (even if I'm sure it wasn't intentional in this case).

The new FS code is definitely not a drop-in replacement for the equivalent Spring code in more ways that just path handling. For a trivial example, take a look at https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/util/ClassUtils.html#getDefaultClassLoader() which handles the case when the context class loader is null, but it wouldn't surprise me if there are others.

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

Successfully merging this pull request may close these issues.

2 participants