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

Fix java.lang.ClassCastException is reading default yaml config #3920

Merged
merged 3 commits into from
Jul 2, 2021

Conversation

chao2zhang
Copy link
Member

Stacktrace

Caused by: java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String
	at io.gitlab.arturbosch.detekt.rules.style.MagicNumber$ignoreNumbers$2.invoke(MagicNumber.kt:259)
	at io.gitlab.arturbosch.detekt.rules.style.MagicNumber$ignoreNumbers$2.invoke(MagicNumber.kt:86)
	at io.gitlab.arturbosch.detekt.api.internal.TransformedConfigProperty.doGetValue(ConfigProperty.kt:87)
	at io.gitlab.arturbosch.detekt.api.internal.MemoizedConfigProperty.getValue(ConfigProperty.kt:64)
	at io.gitlab.arturbosch.detekt.api.internal.MemoizedConfigProperty.getValue(ConfigProperty.kt:60)
	at io.gitlab.arturbosch.detekt.rules.style.MagicNumber.getIgnoreNumbers(MagicNumber.kt:86)
	at io.gitlab.arturbosch.detekt.rules.style.MagicNumber.visitConstantExpression(MagicNumber.kt:138)

Root cause

With #3827, each line in a multi-line YAML array can be unquoted, such as

    ignoreNumbers:
      - -1
      - 0
      - 1
      - 2

This will lead to YAML config being parsed as List<Int>, but MagicNumber.ignoreValues() still have a default value of List<String>, causing the exception.

Fix

The pessimistic fix is to always print quotes in multi-line YAML array.

Testing

Since Gradle integration tests are only testing against latest published version, I have verified the fix through:

./gradlew :detekt-generator:generateDocumentation
./gradlew publishToMavenLocal
./gradlew :detekt-gradle-plugin:test
`

@marschwar
Copy link
Contributor

marschwar commented Jun 29, 2021

An alternative approach would be to change the fun Config.valueOrDefaultCommaSeparated in PathFilters.kt to assume a List<Any> and call toString() on every item.

fun Config.valueOrDefaultCommaSeparated(
    key: String,
    default: List<String>
): List<String> {
    fun fallBack() = valueOrDefault(key, default.joinToString(","))
        .trim()
        .commaSeparatedPattern(",", ";")
        .toList()

    return try {
        valueOrDefault<List<Any>>(key, default).map { it.toString() }
    } catch (_: IllegalStateException) {
        fallBack()
    } catch (_: ClassCastException) {
        fallBack()
    }
}

@marschwar
Copy link
Contributor

marschwar commented Jun 30, 2021

On second thought... Your solution is probably the safer bet for now. Since we only support lists that are parameterized with String it makes things easier to understand. Anything I can do to help?

@cortinico cortinico added this to the 1.18.0 milestone Jun 30, 2021
@cortinico cortinico added the housekeeping Marker for housekeeping tasks and refactorings label Jun 30, 2021
@cortinico
Copy link
Member

On second thought... Your solution is probably the safer bet for now.

I'd say let's go for that 👍

This PR is sort of crucial as it's currently blocking the release of 1.18.0.
We should also look into why those kind of bugs are happening whenever we're about to release a new version (and not before merging changes).

@marschwar
Copy link
Contributor

The expectation in detekt-generator/src/test/resources/RuleSetConfig.yml has to be updated as well.

Copy link
Contributor

@marschwar marschwar left a comment

Choose a reason for hiding this comment

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

I am so glad that you came across this before the release. Thank you

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #3920 (a751d74) into main (3056e06) will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3920      +/-   ##
============================================
- Coverage     83.59%   83.44%   -0.15%     
- Complexity     3123     3149      +26     
============================================
  Files           456      456              
  Lines          9015     9014       -1     
  Branches       1754     1754              
============================================
- Hits           7536     7522      -14     
- Misses          564      565       +1     
- Partials        915      927      +12     
Impacted Files Coverage Δ
.../io/gitlab/arturbosch/detekt/generator/out/Yaml.kt 68.57% <ø> (-1.70%) ⬇️
...sch/detekt/rules/coroutines/SleepInsteadOfDelay.kt 72.00% <0.00%> (-8.00%) ⬇️
...arturbosch/detekt/generator/collection/KDocTags.kt 62.50% <0.00%> (-5.00%) ⬇️
...h/detekt/rules/style/DataClassContainsFunctions.kt 95.00% <0.00%> (-5.00%) ⬇️
...rturbosch/detekt/rules/bugs/UnnecessarySafeCall.kt 73.91% <0.00%> (-4.35%) ⬇️
...io/github/detekt/report/sarif/SarifOutputReport.kt 90.90% <0.00%> (-3.04%) ⬇️
...rturbosch/detekt/rules/style/UnusedPrivateClass.kt 70.66% <0.00%> (-2.67%) ⬇️
...ekt/generator/collection/DocumentationCollector.kt 85.00% <0.00%> (-2.50%) ⬇️
...ab/arturbosch/detekt/rules/style/UnnecessaryLet.kt 80.00% <0.00%> (-2.36%) ⬇️
...t/rules/style/UtilityClassWithPublicConstructor.kt 93.61% <0.00%> (-2.13%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3056e06...a751d74. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants