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

Update description with crash #627

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions config/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,19 @@ arisa:
exceptionRegex: 'ig[0-9]{1,2}icd[0-9]{2}\.dll'
duplicates: MC-32606

crashInfo:
projects:
- MC
- MCL
- MCTEST
resolutions:
- Unresolved
excludedStatuses:
- Postponed
crashExtensions:
- txt
- log

revokeConfirmation:
projects:
- MC
Expand Down
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@ kotlin.code.style=official
org.gradle.caching=true
org.gradle.console=auto
org.gradle.daemon=false
org.gradle.parallel=true
org.gradle.parallel=true
9 changes: 9 additions & 0 deletions src/main/kotlin/io/github/mojira/arisa/ModuleRegistry.kt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import io.github.mojira.arisa.modules.AttachmentModule
import io.github.mojira.arisa.modules.CHKModule
import io.github.mojira.arisa.modules.CommandModule
import io.github.mojira.arisa.modules.ConfirmParentModule
import io.github.mojira.arisa.modules.CrashInfoModule
import io.github.mojira.arisa.modules.CrashModule
import io.github.mojira.arisa.modules.DuplicateMessageModule
import io.github.mojira.arisa.modules.EmptyModule
Expand Down Expand Up @@ -149,6 +150,14 @@ class ModuleRegistry(private val config: Config) {
)
)

register(
Modules.CrashInfo,
CrashInfoModule(
config[Modules.CrashInfo.crashExtensions],
CrashReader()
)
)

register(Modules.Empty, EmptyModule(config[Modules.Empty.message]))

register(
Expand Down
1 change: 1 addition & 0 deletions src/main/kotlin/io/github/mojira/arisa/domain/Issue.kt
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ data class Issue(
val addComment: (options: CommentOptions) -> Unit,
val addRestrictedComment: (options: CommentOptions) -> Unit,
val addNotEnglishComment: (language: String) -> Unit,
val addRawComment: (body: String) -> Unit,
val addRawRestrictedComment: (body: String, restriction: String) -> Unit,
val markAsFixedWithSpecificVersion: (fixVersion: String) -> Unit,
val changeReporter: (reporter: String) -> Unit,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,12 @@ object Arisa : ConfigSpec() {
)
}

object CrashInfo : ModuleConfigSpec() {
val crashExtensions by required<List<String>>(
description = "File extensions that should be checked for crash reports."
)
}

object RevokeConfirmation : ModuleConfigSpec()

object KeepPrivate : ModuleConfigSpec() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,26 @@ fun addRestrictedComment(
}
}

fun addRawComment(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed anymore, is it?

Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to leave it for future purposes.

context: Lazy<IssueUpdateContext>,
comment: String
) {
context.value.otherOperations.add {
runBlocking {
Either.catch {
val key = context.value.jiraIssue.key

when (val checkResult = CommentCache.check(key, comment)) {
is Either.Left -> log.error(checkResult.a.message)
is Either.Right -> context.value.jiraIssue.addComment(comment)
}

Unit
}
}
}
}

fun createLink(
context: Lazy<IssueUpdateContext>,
getContext: (key: String) -> Lazy<IssueUpdateContext>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ fun JiraIssue.toDomain(
)
)
},
addRawComment = ::addRawComment.partially1(context),
addRawRestrictedComment = ::addRestrictedComment.partially1(context),
::markAsFixedWithSpecificVersion.partially1(context),
::changeReporter.partially1(context),
Expand Down
92 changes: 92 additions & 0 deletions src/main/kotlin/io/github/mojira/arisa/modules/CrashInfoModule.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package io.github.mojira.arisa.modules

import arrow.core.Either
import arrow.core.extensions.fx
import arrow.core.left
import arrow.core.right
import arrow.syntax.function.partially2
import io.github.mojira.arisa.domain.Issue
import io.github.mojira.arisa.infrastructure.AttachmentUtils
import com.urielsalis.mccrashlib.Crash
import com.urielsalis.mccrashlib.CrashReader
import com.urielsalis.mccrashlib.deobfuscator.getSafeChildPath
import java.nio.file.Files
import java.time.Instant

class CrashInfoModule(
private val crashReportExtensions: List<String>,
private val crashReader: CrashReader
) : Module {
override fun invoke(issue: Issue, lastRun: Instant): Either<ModuleError, ModuleResponse> = with(issue) {
Either.fx {
val crashes = AttachmentUtils(crashReportExtensions, crashReader).extractCrashesFromAttachments(issue)
val newCrashes = assertContainsNewCrash(crashes, lastRun).bind()
uploadDeobfuscatedCrashes(issue, newCrashes)

newCrashes
.filter { it.second is Crash.Minecraft }
.filterNot { it.first.name.endsWith("deobfuscated.txt") }
Comment on lines +26 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably restrict this to crash reports attached by the reporter (or by a helper / moderator?).
Otherwise users can vandalize existing issues by attaching unrelated crash reports, or files which Arisa considers to be crash reports.

.forEach {
if (!description!!.contains("""\{code.*${it.first.name}]}(\S|\s)*\{code}""".toRegex())) {
Copy link
Contributor

@Marcono1234 Marcono1234 May 28, 2021

Choose a reason for hiding this comment

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

Is (\S|\s) to match any character including line breaks? Maybe the complete (\S|\s)*\{code} part can be removed, it is rather unlikely that the description contains {code:title=...} which is not actually a code block.

Additionally we should probably only add the crash report to the description if it does not contain any (instead of checking for the specific crash report name), to prevent adding multiple crash report snippets. Therefore we should probably only consider newCrashes if its size is 1, otherwise Arisa might pick the 'wrong' crash report in case a user uploads unrelated ones.

The regex could probably changed to this:

Suggested change
if (!description!!.contains("""\{code.*${it.first.name}]}(\S|\s)*\{code}""".toRegex())) {
// Check if description already contains code block which might contain crash report
// Note: Jira seems to allow spaces between most of the characters
if (!description!!.contains("""\{code:\s*title\s*=.*\[\^.+\.txt\s*]\s*}""".toRegex())) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this regex is too unspecific though and causes some false negatives, e.g. when {code} blocks link to a non-crash attachment (e.g. code snippet file when a code analysis was done). But I assume that this will happen rarely, so it might be fine for now and we can adjust it later if necessary.

updateDescription(
description + "\n\n" +
generateCrashMessage(it.first.name, it.second as Crash.Minecraft)
)
}
}
}
}

private fun generateCrashMessage(name: String, minecraft: Crash.Minecraft) =
"{code:title=(${minecraft.minecraftVersion}) [^$name]}\n\n" +
"Description: ${minecraft.exception.split("\n").first()}\n\n" +
"Exception: ${minecraft.exception}\n\n" +
(if (minecraft.deobfException != null) "Deobfuscated: ${minecraft.deobfException}\n" else "\n") +
"{code}\n"

private fun uploadDeobfuscatedCrashes(issue: Issue, crashes: List<Pair<AttachmentUtils.TextDocument, Crash>>) {
val minecraftCrashesWithDeobf = crashes
.map { it.first.name to it.second }
.filter { it.second is Crash.Minecraft }
.map { it.first to (it.second as Crash.Minecraft).deobf }
.filter { it.second != null }
.filterNot {
issue.attachments.any { attachment ->
attachment.name == getDeobfName(it.first) || attachment.name.endsWith(
"deobfuscated.txt"
)
}
}
minecraftCrashesWithDeobf.forEach {
val tempDir = Files.createTempDirectory("arisa-crash-upload").toFile()
val safePath = getSafeChildPath(tempDir, getDeobfName(it.first))
if (safePath == null) {
tempDir.delete()
} else {
safePath.writeText(it.second!!)
issue.addAttachment(safePath) {
// Once uploaded, delete the temp directory containing the crash report
tempDir.deleteRecursively()
}
}
}
}

private fun getDeobfName(name: String): String =
"${name.substringBeforeLast(".").replace("\\", "").replace("/", "")}-deobfuscated.txt"

private fun crashNewlyAdded(crash: Pair<AttachmentUtils.TextDocument, Crash>, lastRun: Instant) =
crash.first.created.isAfter(lastRun)

private fun assertContainsNewCrash(
crashes: List<Pair<AttachmentUtils.TextDocument, Crash>>,
lastRun: Instant
): Either<OperationNotNeededModuleResponse, List<Pair<AttachmentUtils.TextDocument, Crash>>> {
val newCrashes = crashes.filter(::crashNewlyAdded.partially2(lastRun))
return if (newCrashes.isNotEmpty()) {
newCrashes.right()
} else {
OperationNotNeededModuleResponse.left()
}
}
}
35 changes: 1 addition & 34 deletions src/main/kotlin/io/github/mojira/arisa/modules/CrashModule.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@ import arrow.core.right
import arrow.syntax.function.partially2
import com.urielsalis.mccrashlib.Crash
import com.urielsalis.mccrashlib.CrashReader
import com.urielsalis.mccrashlib.deobfuscator.getSafeChildPath
import io.github.mojira.arisa.domain.CommentOptions
import io.github.mojira.arisa.domain.Issue
import io.github.mojira.arisa.infrastructure.AttachmentUtils
import io.github.mojira.arisa.infrastructure.config.CrashDupeConfig
import java.nio.file.Files
import java.time.Instant

class CrashModule(
Expand All @@ -30,8 +28,7 @@ class CrashModule(

val crashes = AttachmentUtils(crashReportExtensions, crashReader).extractCrashesFromAttachments(issue)

val newCrashes = assertContainsNewCrash(crashes, lastRun).bind()
uploadDeobfuscatedCrashes(issue, newCrashes)
assertContainsNewCrash(crashes, lastRun).bind()
assertNoValidCrash(crashes).bind()

val key = crashes
Expand All @@ -50,36 +47,6 @@ class CrashModule(
}
}

private fun uploadDeobfuscatedCrashes(issue: Issue, crashes: List<Pair<AttachmentUtils.TextDocument, Crash>>) {
val minecraftCrashesWithDeobf = crashes
.map { it.first.name to it.second }
.filter { it.second is Crash.Minecraft }
.map { it.first to (it.second as Crash.Minecraft).deobf }
.filter { it.second != null }
.filterNot {
issue.attachments.any { attachment ->
attachment.name == getDeobfName(it.first) || attachment.name.endsWith(
"deobfuscated.txt"
)
}
}
minecraftCrashesWithDeobf.forEach {
val tempDir = Files.createTempDirectory("arisa-crash-upload").toFile()
val safePath = getSafeChildPath(tempDir, getDeobfName(it.first))
if (safePath == null) {
tempDir.delete()
} else {
safePath.writeText(it.second!!)
issue.addAttachment(safePath) {
// Once uploaded, delete the temp directory containing the crash report
tempDir.deleteRecursively()
}
}
}
}

private fun getDeobfName(name: String): String = "${name.substringBeforeLast(".")}-deobfuscated.txt"

/**
* Checks whether an analyzed crash report matches any of the specified known crash issues.
* Returns the key of the parent bug report if one is found, and null otherwise.
Expand Down
Loading