diff --git a/AGENTS.md b/AGENTS.md index 162798ec16..4149e5238f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -116,6 +116,12 @@ Variant names combine both dimensions, e.g. `defaultsBc8Debug`, `customEntitleme ## Code Style - **Imports over inline fully-qualified references**: Always add an `import` statement at the top of the file rather than using a fully-qualified name inline (e.g., write `import foo.Bar` and use `Bar`, not `foo.Bar` inline in the code). +- **No public enums in SDK API surface**: Public `enum class` breaks binary compatibility when new entries are added (consumers' exhaustive `when` expressions break). Use sealed classes/interfaces with objects, or annotate with `@InternalRevenueCatAPI`. Enforced by detekt `ForbiddenPublicEnum`. +- **Explicit `@SerialName` for snake_case wire fields**: This module has no global naming strategy, so `@Serializable` properties whose wire name differs from the Kotlin property name **must** have `@SerialName("wire_name")`. Omitting it causes silent deserialization failures. +- **Prefer `Default`/`Impl` suffix over `Type` for interface implementations**: `Type` is not idiomatic Kotlin/Android. Name implementations `DefaultFoo` or `FooImpl`, not `FooType`. +- **Log level discipline**: Use `Logger.v` for per-iteration/polling output and high-frequency events. Reserve `Logger.d` for one-shot noteworthy events, `Logger.w` for recoverable errors, and `Logger.e` for unrecoverable errors. +- **Thread safety — atomic check-then-act**: When shared mutable state requires a "read then conditionally write" pattern, wrap the entire check-then-act sequence in a single `synchronized` block or use `AtomicReference.compareAndSet`. Do not split the read and write across separate synchronized calls. +- **Prefer `Exception` over `RuntimeException` for recoverable domain errors**: `RuntimeException` signals an unrecoverable bug and may crash the app. For domain-specific errors the caller is expected to handle, extend `Exception` instead. ## Testing Framework diff --git a/config/detekt/detekt-baseline.xml b/config/detekt/detekt-baseline.xml index 6ad237c429..4f8ccebd78 100644 --- a/config/detekt/detekt-baseline.xml +++ b/config/detekt/detekt-baseline.xml @@ -42,8 +42,31 @@ Filename:subscriberAttributesFactories.kt$com.revenuecat.purchases.subscriberattributes.subscriberAttributesFactories.kt Filename:subscriptionOptionConversions.kt$com.revenuecat.purchases.google.subscriptionOptionConversions.kt Filename:utils.kt$com.revenuecat.purchases.common.utils.kt + ForbiddenPublicEnum:AmazonLWAConsentStatus.kt$AmazonLWAConsentStatus + ForbiddenPublicEnum:BillingFeature.kt$BillingFeature + ForbiddenPublicEnum:CacheFetchPolicy.kt$CacheFetchPolicy + ForbiddenPublicEnum:Constants.kt$Constants$BackendEnvironment + ForbiddenPublicEnum:EntitlementInfo.kt$OwnershipType + ForbiddenPublicEnum:EntitlementInfo.kt$PeriodType + ForbiddenPublicEnum:EntitlementInfo.kt$Store + ForbiddenPublicEnum:EntitlementVerificationMode.kt$EntitlementVerificationMode + ForbiddenPublicEnum:GoogleReplacementMode.kt$GoogleReplacementMode : ReplacementMode + ForbiddenPublicEnum:InAppMessageType.kt$InAppMessageType + ForbiddenPublicEnum:LogLevel.kt$LogLevel + ForbiddenPublicEnum:NamingStyle.kt$NamingStyle + ForbiddenPublicEnum:OfferPaymentMode.kt$OfferPaymentMode + ForbiddenPublicEnum:OfflineMode.kt$OfflineMode + ForbiddenPublicEnum:Package.kt$PackageType + ForbiddenPublicEnum:Period.kt$Period$Unit + ForbiddenPublicEnum:ProductType.kt$ProductType + ForbiddenPublicEnum:PurchaseState.kt$PurchaseState + ForbiddenPublicEnum:PurchasesAreCompletedBy.kt$PurchasesAreCompletedBy + ForbiddenPublicEnum:PurchasesErrorCode.kt$PurchasesErrorCode + ForbiddenPublicEnum:RecurrenceMode.kt$RecurrenceMode + ForbiddenPublicEnum:StoreTransaction.kt$PurchaseType + ForbiddenPublicEnum:TypographyType.kt$TypographyType + ForbiddenPublicEnum:VerificationResult.kt$VerificationResult ForbiddenPublicSealedClass:AmazonPurchasingData.kt$AmazonPurchasingData : PurchasingData - ForbiddenPublicSealedClass:GalaxyPurchasingData.kt$GalaxyPurchasingData : PurchasingData ForbiddenPublicSealedClass:GooglePurchasingData.kt$GooglePurchasingData : PurchasingData ForbiddenPublicSealedClass:PaywallFont.kt$PaywallFont : Parcelable ForbiddenPublicSealedClass:PaywallResult.kt$PaywallResult : Parcelable diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index c32f20b5f7..071993e68f 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -45,3 +45,7 @@ revenuecat: active: true excludes: [ '**/examples/**', '**/rules-engine-internal/**' ] ignoreAnnotated: [ 'InternalRevenueCatAPI' ] + ForbiddenPublicEnum: + active: true + excludes: [ '**/examples/**', '**/rules-engine-internal/**' ] + ignoreAnnotated: [ 'InternalRevenueCatAPI' ] diff --git a/detekt-rules/src/main/kotlin/com/revenuecat/purchases/detekt/ForbiddenPublicEnum.kt b/detekt-rules/src/main/kotlin/com/revenuecat/purchases/detekt/ForbiddenPublicEnum.kt new file mode 100644 index 0000000000..5ea9450278 --- /dev/null +++ b/detekt-rules/src/main/kotlin/com/revenuecat/purchases/detekt/ForbiddenPublicEnum.kt @@ -0,0 +1,53 @@ +package com.revenuecat.purchases.detekt + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Rule +import io.gitlab.arturbosch.detekt.api.Severity +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtClass +import org.jetbrains.kotlin.psi.KtClassOrObject +import org.jetbrains.kotlin.psi.KtModifierListOwner +import org.jetbrains.kotlin.psi.psiUtil.parents + +class ForbiddenPublicEnum(config: Config) : Rule(config) { + + override val issue = Issue( + id = "ForbiddenPublicEnum", + severity = Severity.Maintainability, + description = "Public enum classes break binary compatibility. " + + "Adding new entries is a source-breaking change for consumers using exhaustive when expressions. " + + "Annotate with @InternalRevenueCatAPI if intentional.", + debt = Debt.TWENTY_MINS, + ) + + private val ignoreAnnotated: List = valueOrDefault("ignoreAnnotated", emptyList()) + + override fun visitClass(klass: KtClass) { + super.visitClass(klass) + if (klass.isEnum() && isPublicApi(klass) && !isAnnotatedWith(klass, ignoreAnnotated)) { + report(CodeSmell(issue, Entity.atName(klass), message = "${klass.name}: ${issue.description}")) + } + } + + private fun isPublicApi(klass: KtClass): Boolean { + if (klass.isNonPublic()) return false + return klass.parents.filterIsInstance().none { it.isNonPublic() } + } + + private fun KtModifierListOwner.isNonPublic(): Boolean = + hasModifier(KtTokens.PRIVATE_KEYWORD) || + hasModifier(KtTokens.INTERNAL_KEYWORD) || + hasModifier(KtTokens.PROTECTED_KEYWORD) + + private fun isAnnotatedWith(klass: KtClass, annotationNames: List): Boolean { + if (annotationNames.isEmpty()) return false + return klass.annotationEntries.any { entry -> + val shortName = entry.shortName?.asString() ?: return@any false + annotationNames.any { it == shortName || it.endsWith(".$shortName") } + } + } +} diff --git a/detekt-rules/src/main/kotlin/com/revenuecat/purchases/detekt/RevenueCatRuleSetProvider.kt b/detekt-rules/src/main/kotlin/com/revenuecat/purchases/detekt/RevenueCatRuleSetProvider.kt index da04a44934..b3dfbeb297 100644 --- a/detekt-rules/src/main/kotlin/com/revenuecat/purchases/detekt/RevenueCatRuleSetProvider.kt +++ b/detekt-rules/src/main/kotlin/com/revenuecat/purchases/detekt/RevenueCatRuleSetProvider.kt @@ -9,6 +9,9 @@ class RevenueCatRuleSetProvider : RuleSetProvider { override fun instance(config: Config): RuleSet = RuleSet( ruleSetId, - listOf(ForbiddenPublicSealedClass(config)), + listOf( + ForbiddenPublicSealedClass(config), + ForbiddenPublicEnum(config), + ), ) } diff --git a/detekt-rules/src/test/kotlin/com/revenuecat/purchases/detekt/ForbiddenPublicEnumTest.kt b/detekt-rules/src/test/kotlin/com/revenuecat/purchases/detekt/ForbiddenPublicEnumTest.kt new file mode 100644 index 0000000000..61747b5455 --- /dev/null +++ b/detekt-rules/src/test/kotlin/com/revenuecat/purchases/detekt/ForbiddenPublicEnumTest.kt @@ -0,0 +1,129 @@ +package com.revenuecat.purchases.detekt + +import io.gitlab.arturbosch.detekt.test.TestConfig +import io.gitlab.arturbosch.detekt.test.lint +import kotlin.test.Test +import kotlin.test.assertEquals + +class ForbiddenPublicEnumTest { + + private fun rule(ignoreAnnotated: List = emptyList()) = ForbiddenPublicEnum( + TestConfig( + "active" to true, + "ignoreAnnotated" to ignoreAnnotated, + ), + ) + + // region should flag + + @Test + fun `flags public enum class`() { + val findings = rule().lint("public enum class MyEnum { A, B }") + assertEquals(1, findings.size) + assertEquals(true, findings[0].message.contains("MyEnum")) + } + + @Test + fun `flags implicit public enum class`() { + val findings = rule().lint("enum class MyEnum { A, B }") + assertEquals(1, findings.size) + } + + @Test + fun `flags multiple enum declarations in one file`() { + val code = """ + public enum class First { A } + public enum class Second { B } + internal enum class ShouldBeIgnored { C } + """.trimIndent() + val findings = rule().lint(code) + assertEquals(2, findings.size) + } + + @Test + fun `flags enum class with an annotation not in the ignore list`() { + val code = """ + @SomeOtherAnnotation + public enum class MyEnum { A } + """.trimIndent() + val findings = rule(ignoreAnnotated = listOf("InternalRevenueCatAPI")).lint(code) + assertEquals(1, findings.size) + } + + // endregion + + // region should not flag + + @Test + fun `does not flag internal enum class`() { + val findings = rule().lint("internal enum class MyEnum { A }") + assertEquals(0, findings.size) + } + + @Test + fun `does not flag private enum class`() { + val findings = rule().lint("private enum class MyEnum { A }") + assertEquals(0, findings.size) + } + + @Test + fun `does not flag protected enum class`() { + val code = """ + open class Outer { + protected enum class MyEnum { A } + } + """.trimIndent() + val findings = rule().lint(code) + assertEquals(0, findings.size) + } + + @Test + fun `does not flag non-enum class`() { + val findings = rule().lint("public class MyClass") + assertEquals(0, findings.size) + } + + @Test + fun `does not flag enum class nested inside internal class`() { + val code = """ + internal class Outer { + enum class Inner { A } + } + """.trimIndent() + val findings = rule().lint(code) + assertEquals(0, findings.size) + } + + @Test + fun `does not flag enum class annotated with an ignored annotation`() { + val code = """ + @InternalRevenueCatAPI + public enum class MyEnum { A } + """.trimIndent() + val findings = rule(ignoreAnnotated = listOf("InternalRevenueCatAPI")).lint(code) + assertEquals(0, findings.size) + } + + @Test + fun `does not flag when suppressed with Suppress annotation`() { + val code = """ + @Suppress("ForbiddenPublicEnum") + public enum class MyEnum { A } + """.trimIndent() + val findings = rule().lint(code) + assertEquals(0, findings.size) + } + + @Test + fun `does not flag enum class nested inside private class`() { + val code = """ + private class Outer { + enum class Inner { A } + } + """.trimIndent() + val findings = rule().lint(code) + assertEquals(0, findings.size) + } + + // endregion +}