Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

use urldecoder for proper handling of url-encoded filepath #6

Merged
merged 1 commit into from
Aug 23, 2020
Merged

use urldecoder for proper handling of url-encoded filepath #6

merged 1 commit into from
Aug 23, 2020

Conversation

aberndl
Copy link
Contributor

@aberndl aberndl commented Jul 17, 2020

No description provided.

File file = new File(url.getFile());
File file;
try {
file = new File(URLDecoder.decode(url.getFile(), "UTF-8"));
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a unit test for this class which should cover the URL decoding by checking a path containing spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you point me to that unittest? SourceFileMatcherTest does not contain such a test. As far as I can tell AsciidocReportPluginTest.defaultIndexDocument() is the only test that executes the affected code fragment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! It means that SourceFileMatcherTest#detectIndexFileFromRuleSources does not test what it is supposed to be covering...
The reason is that the test method initializes the SourceFileMatcher with a rulesDirectory but it should be SourceFileMatcher sourceFileMatcher = new SourceFileMatcher(null, null, null); instead (maybe the API could need some improvement here, but probably not in the scope of this PR).
Afterwards the test will fail as expected if the path contains white spaces. Proposing to change "src/test/resources/jqassistant/" to "src/test/resources/working dir/jqassistant/" for verifying this. Would it be Ok for you to include this in the PR?

@DirkMahler DirkMahler merged commit f316f3a into jqassistant-archive:master Aug 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants