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

Introduce getContentAsString and getContentAsByteArray to Resource #24651

Conversation

derrick-pericipio
Copy link

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 6, 2020
@derrick-pericipio derrick-pericipio changed the title Create ClassPathResource function getString Introduce ClassPathResource function getString() to allow for streamlined access Mar 6, 2020
@sbrannen
Copy link
Member

sbrannen commented Mar 6, 2020

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.

  1. Adding a compile dependency on commons-io is not an option. Instead, we'd likely want to use our own FileCopyUtils (potentially introducing new methods if necessary to allow for configurable encoding).
  2. The code must adhere to the project's code style. Executing ./gradlew check would be a good place to start to see Javadoc violations.
  3. The name getString() is potentially misleading. A better name would be something like getContentAsString().
  4. Why would we want to limit this to ClassPathResource? Why not introduce it in Resource?

@sbrannen sbrannen changed the title Introduce ClassPathResource function getString() to allow for streamlined access Introduce getString() in ClassPathResource to retrieve resource content as a String Mar 6, 2020
@mp911de
Copy link
Member

mp911de commented Mar 6, 2020

That's a neat optimization that sounds useful on the Resource level.

@derrick-pericipio
Copy link
Author

All great feedback:

  • Not sure why my local build didnt flag the code style violation.. my bad.

  • I wondered what level to introduce this at so I started higher rather than lower.

  • I'll look at FileCopyUtils, can you elaborate on the desire to avoid commons? It's already on the class path at the parent project level. Ultimately it just makes the implementation cleaner, it's not necessary for functionality.

  • Thoughts on throwing both FileNotFoundException and IOException? I couldn't think of a more appropriate set when I put this together.

@sbrannen
Copy link
Member

sbrannen commented Mar 6, 2020

As @jhoeller mentioned elsewhere, have you considered wrapping your ClassPathResource in an EncodedResource and then passing encodedResource.getReader() to org.springframework.util.FileCopyUtils.copyToString(Reader)?

@derrick-pericipio
Copy link
Author

@sbrannen

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

@derrick-pericipio
Copy link
Author

I have pushed the implementation of the new function getContentAsString() down to the Resource interface and provided it with a default behavior of toString() to avoid having to override in both the Web and MVC modules.

The Concrete implementation was provided at the AbstractFileResolvingResource level as any local or jar file should be able to be serialized. It no longer uses Apache Commons and uses the same base raw Java implementation as the Kotlin equivalent. This does rely on Java version being 7 or higher which I did not consider an issue as the minimum for the project is 8.

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 Files.readBytes(File.path, Charset)) and having to handle the exceptions on their own (or forgetting)

This is a largely QOL enhancement to the Java developer experience as Kotlin provides a convenience function known as getResource(path).readText()

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.

@derrick-pericipio derrick-pericipio changed the title Introduce getString() in ClassPathResource to retrieve resource content as a String Introduce getContentAsString() in Resource to retrieve resource content as a String Mar 7, 2020
@derrick-pericipio
Copy link
Author

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.

@rstoyanchev
Copy link
Contributor

Couldn't this be implemented for FileSystemResource as well?

If the primary use case for this is in tests, I'm not sure I like seeing the methods directly on Resource. Another option could be ResourceUtils but even then the implementation looks so straight forward, we could just make it a test utility somewhere.

You mentioned for "testing REST or WEBMVC". Is there a specific place you've noticed where we could add this so resources are logged?

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Mar 25, 2020
@derrick-anderson
Copy link
Contributor

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 FileSystemResource if that makes sense. I pushed it down to Resource at @sbrannen 's reccomendation.

Where do we think this should belong?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 3, 2020
@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Nov 8, 2021
if ( !isReadable() ) {
throw new IOException(getDescription() + " cannot be opened for reading.");
}
return new String(Files.readAllBytes(Paths.get(getFile().getAbsolutePath())), Charset.defaultCharset());
Copy link
Contributor

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)

Copy link
Contributor

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.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Dec 6, 2021
@rstoyanchev rstoyanchev added in: core Issues in core modules (aop, beans, core, context, expression) and removed in: web Issues in web modules (web, webmvc, webflux, websocket) labels Sep 29, 2022
@rstoyanchev rstoyanchev added this to the Triage Queue milestone Sep 29, 2022
@rstoyanchev rstoyanchev removed this from the Triage Queue milestone Jan 20, 2023
@sbrannen sbrannen added the type: enhancement A general enhancement label Jan 26, 2023
@poutsma poutsma self-assigned this Feb 14, 2023
@poutsma poutsma removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 14, 2023
@poutsma poutsma added this to the 6.0.5 milestone Feb 14, 2023
@poutsma poutsma changed the title Introduce getContentAsString() in Resource to retrieve resource content as a String Introduce getContentAsString and getContentAsByteArray to Resource Feb 14, 2023
@poutsma poutsma closed this in 4da2499 Feb 14, 2023
poutsma added a commit that referenced this pull request Feb 14, 2023
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
@poutsma
Copy link
Contributor

poutsma commented Feb 14, 2023

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.

@derrick-anderson
Copy link
Contributor

derrick-anderson commented Feb 14, 2023 via email

jhoeller added a commit that referenced this pull request Feb 15, 2023
mdeinum pushed a commit to mdeinum/spring-framework that referenced this pull request Jun 29, 2023
This commit introduces the getContentAsString method to Resource,
returning the string contents of the resource.

Closes spring-projectsgh-24651
mdeinum pushed a commit to mdeinum/spring-framework that referenced this pull request Jun 29, 2023
This commit makes several changes to PR spring-projects#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 spring-projectsgh-24651
mdeinum pushed a commit to mdeinum/spring-framework that referenced this pull request Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants