-
Notifications
You must be signed in to change notification settings - Fork 38.5k
Introduce getContentAsString and getContentAsByteArray to Resource #24651
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
Introduce getContentAsString and getContentAsByteArray to Resource #24651
Conversation
69c8337
to
501b795
Compare
Thanks for the PR. I'm not yet sure if we'll introduce functionality like this, but if we do, we'll need to take the following into account.
|
That's a neat optimization that sounds useful on the |
All great feedback:
|
As @jhoeller mentioned elsewhere, have you considered wrapping your |
My ultimate goal was a developer quality of life improvement to bring this on par with the Kotlin experience. I will work on implementing this without Commons |
501b795
to
1907a96
Compare
I have pushed the implementation of the new function The Concrete implementation was provided at the Ultimately I expect this to be utilized during Contract Driven and Test Driven Development processes to avoid having to use Raw String variables and to pull developers away from using ResourceUtils directly with either Commons or the overtly verbose This is a largely QOL enhancement to the Java developer experience as Kotlin provides a convenience function known as An alternative reasoning behind this enhancement is to close the need for access to ResourceUtils outside of the spring framework and allow for moving that Class to package private in a future change. |
spring-core/src/main/java/org/springframework/core/io/AbstractFileResolvingResource.java
Outdated
Show resolved
Hide resolved
Please let me know if there is any other change desired for this enhancement. If not, I look forward to potentially seeing this contribution join the Spring Ecosystem. |
Couldn't this be implemented for If the primary use case for this is in tests, I'm not sure I like seeing the methods directly on You mentioned for "testing REST or WEBMVC". Is there a specific place you've noticed where we could add this so resources are logged? |
Picking this back up on my new account. @rstoyanchev. When i mentioned the use case it was in reference to situations when it made sense to serve static content from a jar file. Often i've seen it for sample json payloads for dummy rest endpoints (when users could consume a stub jar etc) I can implement it for the Where do we think this should belong? |
if ( !isReadable() ) { | ||
throw new IOException(getDescription() + " cannot be opened for reading."); | ||
} | ||
return new String(Files.readAllBytes(Paths.get(getFile().getAbsolutePath())), Charset.defaultCharset()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relying on the default charset is a recipe for disaster, as it can vary between environments and make code fail in collocation that runs fine in another.
Because we cannot determine the resource's charset with a 100% certainty, Resource::getContentAsString()
should be removed, or it should be defined to use UTF-8 as default. (see Files::readString
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Poutsma!
As this function is the core of this feature proposal I'll update the request to use utf-8 by default instead of throwing out the whole PR lol.
This commit makes several changes to PR #24651. - Add byte[] getContentAsByteArray() on Resource. - Remove getContentAsString() from Resource, as it relied on the default charset which is not reliable. - Add getContentAsString() to EncodedResource, as a charset is provided through the constructor. See gh-24651
Thank you for submitting a PR, and our apologies for taking this long to resolve it. We made several changes to the PR, see 12d4dc1 for more details. |
Thank you for the update! I'll review the updates this weekend.
Derrick Anderson (He/Him)
|
Projects that use java heavily rely on a clunky, verbose, and error prone method to access Resources for tests.
Adding the ability to request a file's contents as a string for testing REST or WEBMVC allows for cleaner test classes and more readable code.