fix: import with meta without creating new keys (#3000) #3113
Conversation
WalkthroughA new boolean parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Test as ImportSettingsControllerApplicationTest
participant Processor as CoreImportFilesProcessor
participant Manager as ImportDataManager
participant KeyMetaService
Test->>Processor: applySettings(..., createNewKeys)
Processor->>Processor: getOrCreateKey()
alt key has keyMeta
Processor->>Manager: prepareKeyMeta(keyMeta)
alt saving enabled
Processor->>KeyMetaService: save(keyMeta)
end
end
Processor->>Processor: save key
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/data/src/main/kotlin/io/tolgee/service/dataImport/ImportDataManager.kt (1)
217-220: Consider refactoring to eliminate code duplication.The new
prepareKeyMetamethod duplicates the logic fromprepareKeyMetas(lines 211-215). While this supports the per-key processing approach needed to fix the Hibernate flush issues, consider refactoring to eliminate duplication.For example, you could refactor
prepareKeyMetasto use the new method:fun prepareKeyMetas() { - this.storedKeys.mapNotNull { it.value.keyMeta }.forEach { meta -> - meta.comments.onEach { comment -> comment.author = comment.author ?: import.author } - meta.codeReferences.onEach { ref -> ref.author = ref.author ?: import.author } - } + this.storedKeys.mapNotNull { it.value.keyMeta }.forEach { meta -> + prepareKeyMeta(meta) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ImportController/importSettings/ImportSettingsControllerApplicationTest.kt(7 hunks)backend/app/src/test/resources/import/po/simplePoWithSingleKeyMeta.po(1 hunks)backend/data/src/main/kotlin/io/tolgee/service/dataImport/CoreImportFilesProcessor.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/service/dataImport/ImportDataManager.kt(1 hunks)
🔇 Additional comments (6)
backend/app/src/test/resources/import/po/simplePoWithSingleKeyMeta.po (1)
1-3: LGTM! Well-formatted PO test resource.The PO file follows correct gettext format with a reference comment, message ID, and German translation. This appropriately supports the test case for importing keys with metadata.
backend/data/src/main/kotlin/io/tolgee/service/dataImport/CoreImportFilesProcessor.kt (2)
48-50: LGTM! Proper service injection.The KeyMetaService injection is correctly implemented to support per-key metadata handling.
304-309: Good solution for Hibernate flush issues.The per-key metadata preparation and saving correctly addresses the Hibernate flush problems mentioned in the PR objectives. While this approach may have performance implications compared to bulk operations, it's a justified trade-off to avoid the cascading complexity and "unsaved transient instance" errors.
The conditional saving based on
saveDataflag is properly handled.backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ImportController/importSettings/ImportSettingsControllerApplicationTest.kt (3)
21-22: LGTM! Appropriate test resource addition.The new test resource correctly supports the specific test case for importing with key metadata when
createNewKeys=false.
129-139: Excellent test coverage for the GitHub issue.The new test case specifically targets the scenario described in GitHub issue #3000, verifying that import works correctly when
createNewKeys=falseand the first key has metadata. This provides good coverage for the Hibernate flush fix.
195-208: Consistent parameter addition across test methods.The
createNewKeysparameter has been properly added to theapplySettingsmethod signature and all existing test calls have been updated consistently. The defaulttruevalues maintain backward compatibility for existing tests.
d7dffc0 to
c91768e
Compare
| @@ -297,7 +301,9 @@ class CoreImportFilesProcessor( | |||
| this.keys.computeIfAbsent(name) { | |||
| ImportKey(name = name, this.fileEntity) | |||
| }.also { | |||
| it.keyMeta?.also { keyMeta -> importDataManager.prepareKeyMeta(keyMeta) } | |||
There was a problem hiding this comment.
I am afraid that these changes might affect performance. In the case of imports the performance is crucial because we are handling huge amount of saves. Are you sure this is fine?
It looks to me like we are saving key metas even if they are already exist, which might cost us some resources...
There was a problem hiding this comment.
repository.save method doesn't execute an updatequery if entity is not marked dirty. For example the following snippet doesn't issue "update" to db:
Optional<Entity> entity = reposRepository.findById(id);
entity.ifPresent(existingEntity -> {
existingEntity.setUrl(existingEntity.getUrl());
reposRepository.save(existingEntity);
});When saving keys, isn't it correct to save KeyMeta? What's done in the fix is actually just applying the cascading manually.
Regarding the performance, of course, it needs to be tested, but I suppose that it should not be affected since the number of requests to db must not change. Is there an example of a such huge files for imports?
There was a problem hiding this comment.
i've added another change. Now it works correctly. I've tested it on the "example.po". This fix generates two additional "update" queries for key_meta. I will try to get rid of it. Meanwhile will move this pr do drafts
There was a problem hiding this comment.
Thanks a lot! I believe I have separated all the saves to groups, so hibernate can execute the updates in single statement... However, I am not hibernate expert so if you are sure it's not harming the performance, we can merge it. However, it's a good idea to test it with some larger dataset like 10000 keys and see how fast it is.
There was a problem hiding this comment.
Also, as stated in this article, it's not always that straightforward with the save() method. Read more in the "The redundant save anti-pattern" section. https://vladmihalcea.com/jpa-persist-and-merge/
There was a problem hiding this comment.
@JanCizmar it's ready for review. Now, the number of queries to the database is the same as it was before, plus it fixes the issue.
It was not only about the save method, but more about the order of saving entities.
My opinion is that current mapping can lead to unexpected bugs like this. However, as they say, if you criticize, offer a solution :) I'm sure there is a reason behind such relations between entities, that I don't see at the moment. I will get more familiar with it and with the code in general. Then I'll be able to offer a solution... or will admit that it's ok :)
There was a problem hiding this comment.
OK, thanks a lot for the PR. I will merge it and let's see what happens.
| @@ -312,10 +318,8 @@ class CoreImportFilesProcessor( | |||
| } | |||
| keyEntity.shouldBeImported = shouldImportKey(keyEntity.name) | |||
| } | |||
| importDataManager.prepareKeyMetas() | |||
There was a problem hiding this comment.
this methods are unused now, right? So they should be removed.
| if (saveData) { | ||
| importDataManager.saveAllStoredKeys() | ||
| importDataManager.saveAllKeyMetas() |
There was a problem hiding this comment.
this methods are unused now, right? So they should be removed.
150d6d7 to
9aa4bed
Compare
Signed-off-by: dmitrii.bocharov <bdshadow@gmail.com>
## [3.123.2](v3.123.1...v3.123.2) (2025-06-16) ### Bug Fixes * import with meta without creating new keys ([#3000](#3000)) ([#3113](#3113)) ([d266ce3](d266ce3)), closes [/github.com//issues/3000#issuecomment-2865753203](https://github.com//github.com/tolgee/tolgee-platform/issues/3000/issues/issuecomment-2865753203)
When
shouldImportKeyis called in theFileProcessorContext.processTranslations()method, Hibernate does the flush before executing a select query. At this flush, it discovers thatImportKeyhasKeyMeta, which is newly created and needs to be flushed, however, there is no cascading.So cadacding would be a good solution. It is also the first one which is recommended in many articles, e.g. https://www.baeldung.com/hibernate-unsaved-transient-instance-error, as well as cascading is usually (sure, not always) applied in OneToOne relations: https://vladmihalcea.com/the-best-way-to-map-a-onetoone-relationship-with-jpa-and-hibernate/. But when I started applying cascading, I ended up modifying too many entities, which led to many unpredictable problems
So I think this solution of saving a
KeyMetaalongside theImportKeyis a good compromise here.Probably the test can be moved to a more suitable place. (I'm just getting acquainted with the code base). It can easily be replicated also with the
strings.xmlfile mentioned in the commentSummary by CodeRabbit