Conversation
📝 WalkthroughWalkthroughConsolidates and centralizes configuration by replacing many per-feature constructor-injected property beans with a single Changes
Sequence Diagram(s)(Skipped — changes are refactor/DI consolidation and do not introduce a new multi-component sequential flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Anty0
force-pushed
the
jirikuchynka/fix-properties-achitecture
branch
from
f872cd3 to
497fcce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/ImportProperties.kt (1)
22-23: Typos in documentation: "foler" → "folder".The documentation contains two instances of "foler" that should be "folder".
📝 Proposed fix
- "└─Project # the foler name is the project name\n" + - " └─validation # the foler name is namespace\n" + + "└─Project # the folder name is the project name\n" + + " └─validation # the folder name is namespace\n" +backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/SsoGlobalProperties.kt (1)
69-80: Ensure SSO validation still runs after de-beaning this class.With
@ConfigurationPropertiesremoved,SsoGlobalPropertiesis likely no longer a Spring bean, so@PostConstructmay never execute. That would silently skip the required-field checks whenenabled = true. Please ensurevalidate()is invoked from a bean lifecycle hook (e.g.,TolgeeProperties@PostConstruct) or migrate to Bean Validation (@Validated+@Valid+@field:NotBlank) on the root properties.backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/GithubAuthenticationProperties.kt (1)
20-25: Update the description to match the actual token endpoint usage.The property
authorizationUrlis used inGithubOAuthDelegate.ktto exchange the authorization code for an access token viarestTemplate.postForObject(). The description incorrectly states "URL to the OAuth authorization screen," but should instead describe it as a token endpoint. This matches the actual implementation and aligns with howGoogleAuthenticationProperties.ktdescribes the same property as "URL to Google/tokenAPI endpoint."Change the description from "URL to the OAuth authorization screen" to "URL to the OAuth token/access token API endpoint. Useful if you want to authenticate against a self-hosted GitHub Enterprise Server."
backend/data/src/test/kotlin/io/tolgee/security/ratelimit/RateLimitServiceTest.kt (1)
62-95: Reset the newtolgeePropertiesmock between tests.
tolgeePropertiesis stubbed insetupMocks()but never reset; this can leak interactions or stubbing across tests. Add it toresetMocks().🔧 Suggested fix
- fun resetMocks() { - Mockito.reset(currentDateProvider, rateLimitProperties, userAccount) - } + fun resetMocks() { + Mockito.reset(currentDateProvider, rateLimitProperties, tolgeeProperties, userAccount) + }backend/security/src/test/kotlin/io/tolgee/security/ratelimit/RateLimitInterceptorTest.kt (1)
49-89: Reset the newtolgeePropertiesmock between tests.
tolgeePropertiesis stubbed insetupMocks()but never reset; add it toresetMocks()to prevent cross‑test leakage.🔧 Suggested fix
- fun resetMocks() { - Mockito.reset(rateLimitProperties, currentDateProvider, authenticationFacade, userAccount) - } + fun resetMocks() { + Mockito.reset(rateLimitProperties, tolgeeProperties, currentDateProvider, authenticationFacade, userAccount) + }
🤖 Fix all issues with AI agents
In
`@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobTestBase.kt`:
- Around line 31-59: The setup() method mutates shared TolgeeProperties (via
tolgeeProperties and machineTranslationProperties) which can leak into other
cached Spring test contexts; fix by capturing the original relevant fields
(e.g., tolgeeProperties.internal.fakeMtProviders and fields under
machineTranslationProperties.google and .aws) before mutating and restore them
in an `@AfterEach` teardown method, or alternatively annotate the test class or
individual tests with `@DirtiesContext` to force a fresh context; ensure
restoration targets the exact properties you change in setup() so no mutated
state persists across tests.
🧹 Nitpick comments (4)
backend/security/src/test/kotlin/io/tolgee/security/authentication/AuthenticationInterceptorTest.kt (1)
44-47: Consider updatingresetMocksto include all mocks for consistency.The
@AfterEachresetsauthenticationFacadeanduserAccountbut omitstolgeePropertiesandauthenticationProperties. While JUnit 5's defaultPER_METHODlifecycle creates fresh instances anyway, keeping the reset list consistent avoids confusion if the lifecycle changes or if someone copies this pattern elsewhere.Suggested fix
`@AfterEach` fun resetMocks() { - Mockito.reset(authenticationFacade, userAccount) + Mockito.reset(authenticationFacade, userAccount, tolgeeProperties, authenticationProperties) }backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/RabbitmqAutostartProperties.kt (1)
15-26: Pre-existing documentation error: enum comments reference "postgres" instead of "rabbitmq".The doc comments in
RabbitMqAutostartModeappear to be copy-pasted from a Postgres autostart class. Consider updating them to reference RabbitMQ for clarity.📝 Suggested fix
enum class RabbitMqAutostartMode { /** - * Starts docker container with postgres + * Starts docker container with RabbitMQ */ DOCKER, /** - * Expects that postgres is installed in the same container. - * So the Postgres is started with Tolgee. + * Expects that RabbitMQ is installed in the same container. + * So RabbitMQ is started with Tolgee. */ EMBEDDED, }backend/data/src/test/kotlin/io/tolgee/security/authentication/JwtServiceTest.kt (1)
104-107: Consider addingtolgeePropertiesto the reset call for consistency.The
tolgeePropertiesmock is not included inresetMocks(), while other mocks are. Although the@BeforeEachre-establishes stubs before each test, including it would maintain consistency and prevent potential issues if tests modify the mock's behavior.Proposed fix
`@AfterEach` fun resetMocks() { - Mockito.reset(authenticationProperties, currentDateProvider, userAccountService, authenticationFacade, userAccount) + Mockito.reset(tolgeeProperties, authenticationProperties, currentDateProvider, userAccountService, authenticationFacade, userAccount) }backend/data/src/main/kotlin/io/tolgee/component/machineTranslation/providers/DeeplApiService.kt (1)
77-81: Pre-existing: Potential NPE on nullauthKey.The
!!operator on line 78 will throw an NPE ifauthKeyis null. This is pre-existing behavior not introduced by this PR, but worth noting for future improvement. Consider adding a null check or using the Elvis operator to provide a safer fallback or clearer error message.
| @Autowired | ||
| lateinit var machineTranslationProperties: MachineTranslationProperties | ||
| lateinit var tolgeeProperties: TolgeeProperties | ||
|
|
||
| val machineTranslationProperties: MachineTranslationProperties get() = tolgeeProperties.machineTranslation | ||
|
|
||
| @Autowired | ||
| lateinit var entityManager: EntityManager | ||
|
|
||
| var fakeBefore: Boolean = false | ||
|
|
||
| @Autowired | ||
| private lateinit var internalProperties: InternalProperties | ||
|
|
||
| @Autowired | ||
| private lateinit var testDataService: TestDataService | ||
|
|
||
| fun setup() { | ||
| batchJobOperationQueue.clear() | ||
| testData = BatchJobsTestData() | ||
|
|
||
| whenever(internalProperties.fakeMtProviders).thenReturn(true) | ||
|
|
||
| val googleMock = mock<GoogleMachineTranslationProperties>() | ||
| whenever(googleMock.apiKey).thenReturn("mock") | ||
| whenever(googleMock.defaultEnabled).thenReturn(true) | ||
| whenever(googleMock.defaultPrimary).thenReturn(true) | ||
|
|
||
| whenever(machineTranslationProperties.google).thenReturn(googleMock) | ||
| // Set properties directly instead of mocking | ||
| tolgeeProperties.internal.fakeMtProviders = true | ||
|
|
||
| val awsMock = mock<AwsMachineTranslationProperties>() | ||
| whenever(awsMock.defaultEnabled).thenReturn(false) | ||
| whenever(awsMock.accessKey).thenReturn("mock") | ||
| whenever(awsMock.secretKey).thenReturn("mock") | ||
| // Configure Google MT | ||
| machineTranslationProperties.google.apiKey = "mock" | ||
| machineTranslationProperties.google.defaultEnabled = true | ||
| machineTranslationProperties.google.defaultPrimary = true | ||
|
|
||
| whenever(machineTranslationProperties.aws).thenReturn(awsMock) | ||
| // Configure AWS MT | ||
| machineTranslationProperties.aws.defaultEnabled = false | ||
| machineTranslationProperties.aws.accessKey = "mock" | ||
| machineTranslationProperties.aws.secretKey = "mock" |
There was a problem hiding this comment.
Avoid leaking mutated TolgeeProperties across cached test contexts.
setup() mutates shared config without restoring it. If Spring reuses the context for other tests, this can bleed state and cause flakiness. Consider restoring previous values (or using @DirtiesContext) after each test.
🛠️ Suggested reset helper
class BatchJobTestBase {
+ private var prevFakeMtProviders: Boolean? = null
+ private var prevGoogleApiKey: String? = null
+ private var prevGoogleDefaultEnabled: Boolean? = null
+ private var prevGoogleDefaultPrimary: Boolean? = null
+ private var prevAwsDefaultEnabled: Boolean? = null
+ private var prevAwsAccessKey: String? = null
+ private var prevAwsSecretKey: String? = null
+
fun setup() {
batchJobOperationQueue.clear()
testData = BatchJobsTestData()
- // Set properties directly instead of mocking
- tolgeeProperties.internal.fakeMtProviders = true
+ // Set properties directly instead of mocking (capture previous values first)
+ prevFakeMtProviders = tolgeeProperties.internal.fakeMtProviders
+ prevGoogleApiKey = machineTranslationProperties.google.apiKey
+ prevGoogleDefaultEnabled = machineTranslationProperties.google.defaultEnabled
+ prevGoogleDefaultPrimary = machineTranslationProperties.google.defaultPrimary
+ prevAwsDefaultEnabled = machineTranslationProperties.aws.defaultEnabled
+ prevAwsAccessKey = machineTranslationProperties.aws.accessKey
+ prevAwsSecretKey = machineTranslationProperties.aws.secretKey
+
+ tolgeeProperties.internal.fakeMtProviders = true
// Configure Google MT
machineTranslationProperties.google.apiKey = "mock"
machineTranslationProperties.google.defaultEnabled = true
machineTranslationProperties.google.defaultPrimary = true
// Configure AWS MT
machineTranslationProperties.aws.defaultEnabled = false
machineTranslationProperties.aws.accessKey = "mock"
machineTranslationProperties.aws.secretKey = "mock"
}
+
+ fun cleanup() {
+ prevFakeMtProviders?.let { tolgeeProperties.internal.fakeMtProviders = it }
+ prevGoogleApiKey?.let { machineTranslationProperties.google.apiKey = it }
+ prevGoogleDefaultEnabled?.let { machineTranslationProperties.google.defaultEnabled = it }
+ prevGoogleDefaultPrimary?.let { machineTranslationProperties.google.defaultPrimary = it }
+ prevAwsDefaultEnabled?.let { machineTranslationProperties.aws.defaultEnabled = it }
+ prevAwsAccessKey?.let { machineTranslationProperties.aws.accessKey = it }
+ prevAwsSecretKey?.let { machineTranslationProperties.aws.secretKey = it }
+ }
}🤖 Prompt for AI Agents
In
`@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobTestBase.kt`
around lines 31 - 59, The setup() method mutates shared TolgeeProperties (via
tolgeeProperties and machineTranslationProperties) which can leak into other
cached Spring test contexts; fix by capturing the original relevant fields
(e.g., tolgeeProperties.internal.fakeMtProviders and fields under
machineTranslationProperties.google and .aws) before mutating and restore them
in an `@AfterEach` teardown method, or alternatively annotate the test class or
individual tests with `@DirtiesContext` to force a fresh context; ensure
restoration targets the exact properties you change in setup() so no mutated
state persists across tests.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/src/test/kotlin/io/tolgee/service/TelemetryServiceTest.kt (1)
37-40: Add TestData cleanup to@AfterEachmethod.The
reports when enabledtest creates TestData viatestDataService.saveTestData(testData.root)but the@AfterEachmethod doesn't clean it up. Following the pattern used in other tests (e.g., SsoOrganizationsTest, KeyUsageReportingTest), addtestDataService.cleanTestData(testData.root)to prevent test pollution.
🧹 Nitpick comments (9)
backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/RabbitmqAutostartProperties.kt (1)
15-26: Pre-existing documentation error: Comments reference Postgres instead of RabbitMQ.The enum documentation incorrectly mentions "postgres" when this is a RabbitMQ configuration class. This is a pre-existing issue but worth fixing while in this file.
📝 Suggested documentation fix
enum class RabbitMqAutostartMode { /** - * Starts docker container with postgres + * Starts docker container with RabbitMQ */ DOCKER, /** - * Expects that postgres is installed in the same container. - * So the Postgres is started with Tolgee. + * Expects that RabbitMQ is installed in the same container. + * So RabbitMQ is started with Tolgee. */ EMBEDDED, }backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/ContentStorageS3Properties.kt (1)
15-22: Optional: document intended use ofclear().
If this is meant for tests or resets only, a short KDoc helps prevent accidental runtime usage.📝 Suggested KDoc
data class ContentStorageS3Properties( override var bucketName: String? = null, override var accessKey: String? = null, override var secretKey: String? = null, override var endpoint: String? = null, override var signingRegion: String? = null, override var path: String? = null, ) : S3Config { + /** + * Resets all S3 properties to null (intended for tests or explicit resets). + */ fun clear() { bucketName = null accessKey = null secretKey = null endpoint = null signingRegion = null path = null } }backend/data/src/test/kotlin/io/tolgee/security/authentication/JwtServiceTest.kt (1)
104-107: Consider addingtolgeePropertiestoresetMocks().The newly added
tolgeePropertiesmock is not included in theMockito.reset()call. While this may not cause issues since the mock's behavior is set up fresh in each@BeforeEach, including it maintains consistency and prevents potential test pollution.♻️ Suggested fix
`@AfterEach` fun resetMocks() { - Mockito.reset(authenticationProperties, currentDateProvider, userAccountService, authenticationFacade, userAccount) + Mockito.reset(tolgeeProperties, authenticationProperties, currentDateProvider, userAccountService, authenticationFacade, userAccount) }backend/security/src/test/kotlin/io/tolgee/security/ratelimit/RateLimitInterceptorTest.kt (1)
87-90: Consider addingtolgeePropertiestoresetMocks().Similar to other test files in this PR, the
tolgeePropertiesmock should be included in the reset call for consistency and to prevent potential test pollution.♻️ Suggested fix
`@AfterEach` fun resetMocks() { - Mockito.reset(rateLimitProperties, currentDateProvider, authenticationFacade, userAccount) + Mockito.reset(tolgeeProperties, rateLimitProperties, currentDateProvider, authenticationFacade, userAccount) }backend/app/src/test/kotlin/io/tolgee/config/BatchJobBaseConfiguration.kt (1)
24-28: Prefer explicit instantiation over class-based spy for clarity.
Mockito.spy(TolgeeProperties::class.java)works correctly sinceTolgeePropertieshas a no-arg constructor with default values. However, being explicit about instantiation improves readability and aligns with modern Mockito conventions:`@Bean` `@Primary` fun tolgeeProperties(): TolgeeProperties { - return Mockito.spy(TolgeeProperties::class.java) + return Mockito.spy(TolgeeProperties()) }backend/data/src/main/kotlin/io/tolgee/service/machineTranslation/MtServiceConfigService.kt (1)
3-10: Remove unused imports (lines 4-10).The imports for
AwsMachineTranslationProperties,AzureCognitiveTranslationProperties,BaiduMachineTranslationProperties,DeeplMachineTranslationProperties,GoogleMachineTranslationProperties,LlmProperties, andMachineTranslationServicePropertiesare not referenced anywhere in the file and can be safely removed.backend/security/src/test/kotlin/io/tolgee/security/authentication/AuthenticationInterceptorTest.kt (1)
44-47: Include all mocks inresetMocks()for consistency.The
resetMocks()method omitstolgeePropertiesandauthenticationPropertiesfrom the reset call. While@BeforeEachre-stubs these mocks, including them ensures clean state and prevents subtle test pollution if future tests verify interactions.♻️ Suggested fix
`@AfterEach` fun resetMocks() { - Mockito.reset(authenticationFacade, userAccount) + Mockito.reset(authenticationFacade, userAccount, tolgeeProperties, authenticationProperties) }backend/app/src/main/kotlin/io/tolgee/postgresRunners/PostgresRunnerConfiguration.kt (1)
17-22: Consider using awhenexpression for mode dispatch.The if-chain can be expressed more idiomatically in Kotlin using a
whenexpression.♻️ Suggested refactor
- if (postgresAutostartProperties.mode == PostgresAutostartMode.DOCKER) { - return PostgresDockerRunner(tolgeeProperties) - } - if (postgresAutostartProperties.mode == PostgresAutostartMode.EMBEDDED) { - return PostgresEmbeddedRunner(tolgeeProperties) - } - return null + return when (postgresAutostartProperties.mode) { + PostgresAutostartMode.DOCKER -> PostgresDockerRunner(tolgeeProperties) + PostgresAutostartMode.EMBEDDED -> PostgresEmbeddedRunner(tolgeeProperties) + else -> null + }backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/MailjetProperties.kt (1)
7-9: Consider documentingcontactListIdfor config docs.
If config docs are generated from@DocProperty, add a brief description for the new field.📝 Suggested documentation
class MailjetProperties { var apiKey: String? = null var secretKey: String? = null + `@DocProperty`(description = "Mailjet contact list ID for default subscriptions.") var contactListId: Long? = null }
|
please, don't merge it before branching |
|
and what's the benefit of it? private val awsMachineTranslationProperties get() = tolgeeProperties.machineTranslation.awsBefore this refactoring it was also possible to inject consolidated |
|
@bdshadow The issue is that we currently have (at least) two instances of each Other than this, we currently have two things broken, which I want to fix this way:
I'm not saying we have to fix it like this, but it felt like the best way to make sure we never run into the same issue again. I can also just fix the names of the two broken properties and leave it be for now. ^^ |
JanCizmar
left a comment
JanCizmar
left a comment
There was a problem hiding this comment.
Thanks. 👍 I would merge it after branching.
if jakarta validation like Another thing is about accessing the properties. This seems weird, need to check. But probably this is because fields inside |
bdshadow
left a comment
bdshadow
left a comment
There was a problem hiding this comment.
I'll dare to request changes :) Sorry :)
Feel free to ping me
You mean they won't have default values anymore? I'm not sure I understand this one. ^^
Yes, please check and let me know (after branching). If there is a different way we can make sure the |
|
@Anty0 my comment was based on my previos experience in java and nested static classes. You can how it works here: https://docs.spring.io/spring-boot/3.5/reference/features/external-config.html#features.external-config.typesafe-configuration-properties.validation In Tolgee there are too many properties. Moving them to nested classes of Also the default values are set right in the koltin code, and before i had it all in properties files. In that case validation perfectly worked. Here it is different. So sorry for delaying this pr, but i'm glad i found it all |
3 participants
This changeset refactors the entire Tolgee backend to centralize property access through a single
TolgeePropertiesclass, rather than having individual*Propertiesclasses injected directly via Spring's@ConfigurationPropertiesscanning.Key changes:
@ConfigurationPropertiesScanto@EnableConfigurationProperties(TolgeeProperties::class)inApplication.kt@ConfigurationPropertiesannotations from all sub-property classesTolgeePropertiesinjection + delegated gettersrateLimit→rateLimitsandllmProperties→llminTolgeePropertiesMtServiceTypeenum to use lambda-based property getters instead of class referencesSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.