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

Make Scapegoat sensor work with absolute file paths in the Scapegoat report #116

Merged
merged 4 commits into from Sep 28, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -19,8 +19,8 @@
package com.mwz.sonar.scala.scapegoat

import org.sonar.api.batch.ScannerSide
import java.nio.file.{Path, Paths}

import java.nio.file.Path
import scala.xml.XML

trait ScapegoatReportParserAPI {
Expand All @@ -32,9 +32,21 @@ trait ScapegoatReportParserAPI {
final class ScapegoatReportParser extends ScapegoatReportParserAPI {
private[this] val AllDotsButLastRegex = raw"\.(?=.*\.)".r

/** Replaces all dots (.) except the last one in a scapegoat path with slashes (/) */
/**
* Replaces all dots '.' except the last one in a scapegoat path with slashes '/'
* while keeping valid directories which contain '.' in their name.
*/
private[scapegoat] def replaceAllDotsButLastWithSlashes(path: String): String =
AllDotsButLastRegex.replaceAllIn(target = path, replacement = "/")
if (path.startsWith(".")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't confirm it, neither test it (cause I'm on my cell phone right now).
But, I think this isn't working the way you think.
For example, I think this file will make it fail: target.scala.12..file.Scala

Ok, so I just noticed that all files in #115 scapegoat report start with '.' - that's probably because they are full paths. But, that wasn't the problem you tried to solve here, but instead paths with dots in their files - which could be relative.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add some tests with extra dots in the middle of the path, but I think it's rather uncommon to have Scala source files hidden, i.e. starting with a ., so I think we can safely ignore this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hidden files probably don't, but what about paths like scala-2.12 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, so your example would work since the implementation checks first if scala-2 exists in the file system and if it doesn't instead of resolving 12 as a subdirectory it concatenates it with a . and the previous path before checking again for existence in the file system - I'll add a unit test to cover that.

path.split('.').reduceLeft[String] {
case (acc, str) =>
val s = Option(str).filter(_.nonEmpty).getOrElse("/")
val a = Option(acc).filter(_.nonEmpty).getOrElse("/")
if (Paths.get(a).toFile.exists)
Paths.get(a).resolve(s).toString
else s"$a.$str"
}
} else AllDotsButLastRegex.replaceAllIn(target = path, replacement = "/")

/** Parses the scapegoat xml report and returns all scapegoat issues by filename */
override def parse(scapegoatReportPath: Path): Map[String, Seq[ScapegoatIssue]] = {
Expand Down
Expand Up @@ -98,9 +98,13 @@ final class ScapegoatSensor(scapegoatReportParser: ScapegoatReportParserAPI) ext

scapegoatIssuesByFilename foreach { tuple =>
val (filename, scapegoatIssues) = tuple
log.info(s"Saving the scapegoat issues for file '$filename'.")
val relativeFile =
if (Paths.get(filename).isAbsolute)
context.fileSystem.baseDir.toPath.relativize(Paths.get(filename)).toString
else filename
log.info(s"Saving the scapegoat issues for file '$relativeFile'.")

getModuleFile(filename, filesystem) match {
getModuleFile(relativeFile, filesystem) match {
case Some(file) =>
scapegoatIssues foreach { scapegoatIssue =>
log.debug(s"Saving the scapegoat issue: $scapegoatIssue.")
Expand Down Expand Up @@ -146,7 +150,7 @@ final class ScapegoatSensor(scapegoatReportParser: ScapegoatReportParserAPI) ext
}
}
case None =>
log.error(s"The file '$filename' couldn't be found.")
log.error(s"The file '$relativeFile' couldn't be found.")
}
}
}
Expand Down
30 changes: 30 additions & 0 deletions src/test/scala/com/mwz/sonar/scala/WithFile.scala
@@ -0,0 +1,30 @@
/*
* Sonar Scala Plugin
* Copyright (C) 2018 All contributors
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package com.mwz.sonar.scala

import java.io.File
import java.nio.file.{Files, Path}

trait WithFile {
def withFile(path: Path)(test: File => Any): Unit = {
val file = Files.createFile(path).toFile
try test(file)
finally Files.deleteIfExists(path)
}
}
Expand Up @@ -16,25 +16,46 @@
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package com.mwz.sonar.scala.scapegoat

import org.scalatest.{FlatSpec, Matchers}
package com.mwz.sonar.scala
package scapegoat

import java.nio.file.Paths

import com.mwz.sonar.scala.util.PathUtils.cwd
import org.scalatest.{FlatSpec, Matchers}

/** Tests the correct behavior of the Scapegoat XML reports parser */
class ScapegoatReportParserSpec extends FlatSpec with Matchers {
class ScapegoatReportParserSpec extends FlatSpec with Matchers with WithFile {
val scapegoatReportParser = new ScapegoatReportParser()

behavior of "the Scapegoat XML Report Parser"

it should "replace all dots (.) except the last one in a scaegoat path with slashes (/)" in {
"replaceAllDotsButLastWithSlashes" should "work with relative paths" in {
val scapegoatPath = "com.mwz.sonar.scala.scapegoat.TestFile.scala"
val linuxPath = scapegoatReportParser.replaceAllDotsButLastWithSlashes(scapegoatPath)

linuxPath shouldBe "com/mwz/sonar/scala/scapegoat/TestFile.scala"
}

it should "work with absolute paths" in {
val scapegoatPath = cwd.resolve("ScapegoatReportParserSpec.scala").toString.replace("/", ".")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather use a literal value. Both for the input and for the expected result.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is possible because the new implementation tests for the existence of the path on the file system so the input and output are both dynamic values based on where this project is located in the file system.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yeah yeah my fault...

val linuxPath = scapegoatReportParser.replaceAllDotsButLastWithSlashes(scapegoatPath)

linuxPath shouldBe cwd.resolve("ScapegoatReportParserSpec.scala").toString
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add a test with filenames that have dots (.) inside them. Like: target/scala.12/.file.Scala.

it should "handle correctly dots in the path" in withFile(cwd.resolve("example.file.scala")) { file =>
val scapegoatPath = file.toString.replace("/", ".")
val linuxPath = scapegoatReportParser.replaceAllDotsButLastWithSlashes(scapegoatPath)

linuxPath shouldBe file.toString
}

it should "handle correctly multiple dots in path" in withFile(cwd.resolve("example.file.scala")) { file =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't those two tests exactly the same?, or what am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, it was meant to be a different test - I fixed it in 9c8fdba.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

val scapegoatPath = file.toString.replace("/", ".")
val linuxPath = scapegoatReportParser.replaceAllDotsButLastWithSlashes(scapegoatPath)

linuxPath shouldBe file.toString
}

it should "be able to parse an empty report" in {
val scapegoatReportPath = Paths.get("src", "test", "resources", "scapegoat", "no-warnings.xml")
val scapegoatWarnings = scapegoatReportParser.parse(scapegoatReportPath)
Expand Down