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

Add endColumn/endLine to SARIF region #5011

Merged
merged 4 commits into from Jul 1, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions detekt-report-sarif/build.gradle.kts
Expand Up @@ -10,4 +10,5 @@ dependencies {
testImplementation(projects.detektTestUtils)
testImplementation(testFixtures(projects.detektApi))
testImplementation(libs.assertj)
testImplementation(libs.mockk)
}
Expand Up @@ -23,26 +23,43 @@ private fun SeverityLevel.toResultLevel() = when (this) {
SeverityLevel.INFO -> Level.Note
}

private fun Finding.toResult(ruleSetId: RuleSetId) = io.github.detekt.sarif4k.Result(
ruleID = "detekt.$ruleSetId.$id",
level = severity.toResultLevel(),
locations = (listOf(location) + references.map { it.location }).map(Location::toLocation).toSet().toList(),
message = Message(text = messageOrDescription())
)

private fun Location.toLocation() = io.github.detekt.sarif4k.Location(
physicalLocation = PhysicalLocation(
region = Region(
startLine = source.line.toLong(),
startColumn = source.column.toLong(),
),
artifactLocation = if (filePath.relativePath != null) {
ArtifactLocation(
uri = filePath.relativePath?.toUnifiedString(),
uriBaseID = SRCROOT
)
} else {
ArtifactLocation(uri = filePath.absolutePath.toUnifiedString())
}
private fun Finding.toResult(ruleSetId: RuleSetId): io.github.detekt.sarif4k.Result {
val code = entity.ktElement?.containingFile?.text

return io.github.detekt.sarif4k.Result(
ruleID = "detekt.$ruleSetId.$id",
level = severity.toResultLevel(),
locations = (listOf(location) + references.map { it.location }).map { it.toLocation(code) }.toSet().toList(),
VitalyVPinchuk marked this conversation as resolved.
Show resolved Hide resolved
message = Message(text = messageOrDescription())
)
}

private fun Location.toLocation(code: String?): io.github.detekt.sarif4k.Location {
var endLine: Long? = null
var endColumn: Long? = null

if (code != null) {
val snippet = code.take(text.end).split('\n')
VitalyVPinchuk marked this conversation as resolved.
Show resolved Hide resolved
endLine = snippet.size.toLong()
endColumn = snippet.last().length.toLong() + 1
VitalyVPinchuk marked this conversation as resolved.
Show resolved Hide resolved
}

return io.github.detekt.sarif4k.Location(
physicalLocation = PhysicalLocation(
region = Region(
startLine = source.line.toLong(),
startColumn = source.column.toLong(),
endLine = endLine,
endColumn = endColumn
),
artifactLocation = if (filePath.relativePath != null) {
ArtifactLocation(
uri = filePath.relativePath?.toUnifiedString(),
uriBaseID = SRCROOT
)
} else {
ArtifactLocation(uri = filePath.absolutePath.toUnifiedString())
}
)
)
)
}
Expand Up @@ -3,6 +3,7 @@ package io.github.detekt.report.sarif
import io.github.detekt.test.utils.readResourceContent
import io.github.detekt.tooling.api.VersionProvider
import io.gitlab.arturbosch.detekt.api.CodeSmell
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.Finding
import io.gitlab.arturbosch.detekt.api.SeverityLevel
import io.gitlab.arturbosch.detekt.api.UnstableApi
Expand All @@ -12,7 +13,11 @@ import io.gitlab.arturbosch.detekt.test.TestDetektion
import io.gitlab.arturbosch.detekt.test.createEntity
import io.gitlab.arturbosch.detekt.test.createFindingForRelativePath
import io.gitlab.arturbosch.detekt.test.createIssue
import io.mockk.every
import io.mockk.mockk
import org.assertj.core.api.Assertions.assertThat
import org.jetbrains.kotlin.com.intellij.psi.PsiFile
import org.jetbrains.kotlin.psi.KtElement
import org.junit.jupiter.api.Test
import java.nio.file.Paths

Expand Down Expand Up @@ -66,10 +71,87 @@ class SarifOutputReportSpec {

assertThat(report).isEqualToIgnoringWhitespace(systemAwareExpectedReport)
}

@Test
fun `snippet region should be bounded with word`() {
val entity = createEntity(
path = "TestFile.kt",
position = 3 to 7,
text = 33..41,
ktElement = mockKtElement(),
)

val result = TestDetektion(
createFinding(ruleName = "TestSmellB", entity = entity, severity = SeverityLevel.WARNING),
)

val report = SarifOutputReport()
.apply { init(EmptySetupContext()) }
.render(result)

val boundedRegion = """
"region": {
"endColumn": 16,
"endLine": 3,
"startColumn": 7,
"startLine": 3
}
"""
assertThat(report).containsIgnoringWhitespaces(boundedRegion)
}

@Test
fun `snippet region should be bounded with block`() {
val entity = createEntity(
path = "TestFile.kt",
position = 3 to 7,
text = 33..88,
ktElement = mockKtElement(),
)

val result = TestDetektion(
createFinding(ruleName = "TestSmellB", entity = entity, severity = SeverityLevel.WARNING),
)

val report = SarifOutputReport()
.apply { init(EmptySetupContext()) }
.render(result)

val boundedRegion = """
"region": {
"endColumn": 2,
"endLine": 5,
"startColumn": 7,
"startLine": 3
}
"""
assertThat(report).containsIgnoringWhitespaces(boundedRegion)
}
}

private fun mockKtElement(): KtElement {
val ktElementMock = mockk<KtElement>()
val psiFileMock = mockk<PsiFile>()
val code = """
package com.example.test

class testClass {
val greeting: String = "Hello, World!"
}

""".trimIndent()

every { psiFileMock.text } returns code
every { ktElementMock.containingFile } returns psiFileMock
return ktElementMock
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to avoid the usage of mockk here if possible. Could this ktelement created/faked instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.
But why is it a bad idea to use mockk here?

}

private fun createFinding(ruleName: String, severity: SeverityLevel): Finding {
return object : CodeSmell(createIssue(ruleName), createEntity("TestFile.kt"), "TestMessage") {
private fun createFinding(ruleName: String, severity: SeverityLevel, entity: Entity? = null): Finding {
return object : CodeSmell(
createIssue(ruleName),
entity ?: createEntity("TestFile.kt"),
VitalyVPinchuk marked this conversation as resolved.
Show resolved Hide resolved
"TestMessage"
) {
override val severity: SeverityLevel
get() = severity
}
Expand Down