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

PathEditor cannot handle absolute Windows paths with forward slashes #29881

Closed
netmikey opened this issue Jan 25, 2023 · 5 comments
Closed

PathEditor cannot handle absolute Windows paths with forward slashes #29881

netmikey opened this issue Jan 25, 2023 · 5 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@netmikey
Copy link

netmikey commented Jan 25, 2023

Since Java can handle Windows paths expressed with forward slashes, people usually express any filesystem path in a configuration file with forward slashes, no matter the OS.

However, Spring's PathEditor doesn't seem to handle what seems to me like a pretty common case:

PathEditor tested = new PathEditor();
tested.setAsText("c:/tmp");

While FileEditor handles this case just fine and the rather obvious Path.of("c:/tmp"); works, the second line will result in:

java.io.FileNotFoundException: class path resource [c:/tmp] cannot be resolved to URL because it does not exist
	at org.springframework.core.io.ClassPathResource.getURL(ClassPathResource.java:214)
	at org.springframework.core.io.AbstractFileResolvingResource.getFile(AbstractFileResolvingResource.java:160)
	at org.springframework.beans.propertyeditors.PathEditor.setAsText(PathEditor.java:109)
	... 71 more

I should mention that, like for Path.of(...);, as a user, I wouldn't expect the resulting Path to necessarily exist on the filesystem. An example use-case would be specifying a path at which something should be created by the application. Such a path wouldn't exist at startup.

Using: org.springframework:spring-beans:5.3.25

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 25, 2023
@rstoyanchev rstoyanchev added the in: core Issues in core modules (aop, beans, core, context, expression) label Jan 27, 2023
@sreenath-tm
Copy link
Contributor

Hi, I would like to contribute to this issue can you assign this issue to me? Can you let me know how I can go about solving this issue?

@sbrannen
Copy link
Member

Hi @sreenath-tm,

Thanks for making the offer, but please hold off on submitting a PR.

This issue is still labeled as waiting-for-triage. Consequently, we have not yet investigated the claim. I'll bring this up with the team, and we will come to a decision.

@snicoll
Copy link
Member

snicoll commented Nov 27, 2023

However, Spring's PathEditor doesn't seem to handle what seems to me like a pretty common case:

The value is not a path but an URL that ultimately delegates to DefaultResourceLoader#getResource. As such, it's like any file URL in Java using Windows. You can make it more obvious it's an url and the following should work:

tested.setAsText("file:///c:/tmp");

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2023
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 27, 2023
@netmikey
Copy link
Author

May I kindly ask you to reconsider this?

I find it very counterintuitive that a class called PathEditor doesn't behave like the Path class it's supposed to convert to. If I use a Path object in a configuration, I would expect to be able to specify an actual path as string - straight forward - and not having to disguise it as file URL (otherwise I would've used URL).

The reliance on DefaultResourceLoader seems like an implementation detail and shouldn't be a justification for a clumsy API.

@snicoll
Copy link
Member

snicoll commented Nov 28, 2023

Alright. I thought using it as an URI was actually expected to avoid ending in a situation where we'd have too many fallbacks. We've discussed this and @jhoeller thinks we can make the fallback more lenient.

@snicoll snicoll reopened this Nov 28, 2023
@snicoll snicoll added type: bug A general bug and removed status: declined A suggestion or change that we don't feel we should currently apply labels Nov 28, 2023
@snicoll snicoll added this to the 6.1.x milestone Nov 28, 2023
@jhoeller jhoeller modified the milestones: 6.1.x, 6.1.2 Nov 28, 2023
@jhoeller jhoeller added the for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x label Nov 30, 2023
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x labels Nov 30, 2023
@jhoeller jhoeller added the for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x label Nov 30, 2023
@github-actions github-actions bot removed the for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x label Nov 30, 2023
jhoeller added a commit that referenced this issue Nov 30, 2023
jhoeller added a commit that referenced this issue Nov 30, 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) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

7 participants