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

path normalization / absolutization is not allowed to use File::getCanonicalPath() #1575

Merged
merged 4 commits into from
Oct 23, 2018

Conversation

ivangalkin
Copy link
Contributor

@ivangalkin ivangalkin commented Oct 12, 2018

Path normalization / absolutization is not allowed to use File::getCanonicalPath(). If performed on Linux (Unix), this method resolves the symbolic links. This fact might make the queries like "is this normalized path in SonarQube project" go wrong.

Fixed #1574

  • review the SonarQube's filesystem queries
  • no path absolutization is necessary; filesystem deals perfectly with relative paths

Fixes #1465

  • CoberturaParser used CxxUtils::normalizePathFull()
  • This made all file paths in parsed report to absolute ones. Base directory of the first processed module was used in order to make paths absolute
  • Since parsed reports were cached, this would lead to the wrong absolute paths in multi-module project
  • Store original paths instead!

Misc:

  • minor refactoring
  • remove error-prone implementation of CxxUtils::normalizePathFull()

This change is Reviewable

* org.sonar.api.batch.fs.FileSystem::resolvePath() returns canonical form
* rewrite it by means of Path::normalize()

* review the sensor code, which performs normalization
* minor changes
@ivangalkin ivangalkin changed the title clean-up file path resolution (see #1574) path normalization / absolutization is not allowed to use File::getCanonicalPath() Oct 14, 2018
@ivangalkin ivangalkin self-assigned this Oct 14, 2018
Path absolutization is never a good idea. Especially in the coverage
sensor, which caches the file paths.

See SonarOpenCommunity#1465
@ivangalkin ivangalkin added the bug label Oct 14, 2018
@guwirth guwirth added this to the 1.2 milestone Oct 17, 2018
@guwirth
Copy link
Collaborator

guwirth commented Oct 19, 2018

@jmecosta ok for me. Do you have any comments? Think you had some special cases in your CI?

@jmecosta
Copy link
Member

@guwirth i think we have integration tests for bullseye in place, it should be ok by me also. i will raise issue if something goes bad in our instances. i dont expect so

@guwirth guwirth merged commit bb69606 into SonarOpenCommunity:master Oct 23, 2018
@ericlemes
Copy link
Contributor

Hi @guwirth and @ivangalkin . I've recently bumped in this issue in 1.1.0 and seems to be fixed by this PR. Any plans for releasing this?

@guwirth
Copy link
Collaborator

guwirth commented Nov 6, 2018

Hi @ericlemes, will be part of 1.2, see #1579. You are welcome to try it out and give feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants