Conversation
- 서버 측 404 에러 반환으로 인해 deletePaths에 계속 남아있어 매번 삭제 요청하는 문제 해결 - 이미지가 삭제되거나 권한이 변경되었을 경우를 대비해 DB에 업로드 시점에 파일명 저장 - 이후 삭제 요청 시 해당 파일명 사용 - supervisorScope와 chunked 처리를 통해 대량의 이미지 업로드 성능을 개선하고 예외 처리 강화
📝 WalkthroughWalkthroughThe changes implement file name tracking for uploaded images across the application. A new Changes
Sequence DiagramsequenceDiagram
actor User
participant Repo as ImageAnalysisRepository
participant Upload as Upload Service
participant DAO as AnalyzedImageDao
participant DB as AppDatabase
User->>Repo: uploadImage(uri, fileName)
activate Repo
Repo->>Repo: splitIntoChunks(uri)
par Parallel Uploads
Repo->>Upload: uploadChunk(chunk1)
Upload-->>Repo: uri1
Repo->>Upload: uploadChunk(chunk2)
Upload-->>Repo: uri2
Repo->>Upload: uploadChunk(chunkN)
Upload-->>Repo: uriN
end
Repo->>Repo: collect Pair<uri, fileName>
Note over Repo: List<Pair<String, String>>
Repo->>DAO: runInTransaction()
activate DAO
DAO->>DAO: finishAnalysis(pairs)
DAO->>DB: insertAllPaths(entities)
DB-->>DAO: ✓ persisted
DAO-->>Repo: ✓ transaction complete
deactivate DAO
Repo-->>User: ✓ image analysis complete
deactivate Repo
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In
@core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/ImageAnalysisRepositoryImpl.kt:
- Around line 172-201: The current use of analyzedImageDao.runInTransaction does
not provide real transactional guarantees; replace that block with a call to the
Room database's withTransaction (e.g., database.withTransaction { ... }) so the
personIndexDataSource.setLastPersonIndex, personRepository add/get methods and
analyzedImageDao.insertAllPaths run inside a single transaction; move the entire
block currently inside analyzedImageDao.runInTransaction into
database.withTransaction { } and ensure you call DAO methods (analyzedImageDao)
and repository operations from within that transaction context so failures roll
back atomically.
- Line 129: The fallback to "unknown.jpg" when
galleryRepository.getFileName(uri) returns null can mask frequent failures;
update ImageAnalysisRepositoryImpl where fileName is assigned to detect the null
case from getFileName(uri) and (1) log a warning including the uri and context
using the existing logger, (2) attempt a better deterministic fallback (e.g.,
derive a name from the uri path or timestamp) before using "unknown.jpg", and
(3) ensure any upload/delete code that relies on this name treats "unknown.jpg"
as exceptional; reference getFileName, galleryRepository, and the fileName usage
so you can add logging and the alternative fallback in the same assignment spot.
In
@core/room/api/src/main/java/com/example/metasearch/core/room/api/dao/AnalyzedImageDao.kt:
- Around line 27-30: The runInTransaction(suspend () -> Unit) DAO method is
ineffective because Room’s @Transaction does not wrap lambdas; remove the
runInTransaction method from AnalyzedImageDao and instead perform transactional
work with AppDatabase.withTransaction from the repository layer (inject
AppDatabase into ImageAnalysisRepositoryImpl and wrap the existing transaction
logic in database.withTransaction { ... }) or alternatively replace the DAO
helper with a concrete @Transaction DAO method that directly calls the required
DAO operations (e.g., a new @Transaction method that orchestrates the multiple
DAO calls).
In
@core/room/impl/src/main/java/com/example/metasearch/core/room/impl/di/DatabaseModule.kt:
- Line 29: The migration chain is incomplete: update the call to addMigrations
on the Room database builder to include AppDatabase.MIGRATION_2_3 between
MIGRATION_1_2 and MIGRATION_3_4 (i.e., .addMigrations(AppDatabase.MIGRATION_1_2,
AppDatabase.MIGRATION_2_3, AppDatabase.MIGRATION_3_4)); if MIGRATION_2_3
intentionally doesn’t exist, either implement AppDatabase.MIGRATION_2_3 with the
schema changes from v2→v3 or add a clear comment/validation that no production
DBs are at version 2 and handle/throw a descriptive error during initialization.
🧹 Nitpick comments (1)
core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/ImageAnalysisRepositoryImpl.kt (1)
129-129: Consider sanitizing fileName to prevent potential multipart issues.File names retrieved from the gallery or MediaStore may contain special characters, path separators, or Unicode that could cause issues when used in multipart form data or on the server side.
🔎 Suggested fileName sanitization
Add a helper function to sanitize file names:
private fun sanitizeFileName(fileName: String): String { return fileName .replace(Regex("[/\\\\:*?\"<>|]"), "_") .take(255) // Most filesystems limit to 255 chars }Use it when retrieving file names:
val fileName = sanitizeFileName(galleryRepository.getFileName(uri) ?: "unknown.jpg")Also applies to: 218-222
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/ImageAnalysisRepositoryImpl.ktcore/room/api/src/main/java/com/example/metasearch/core/room/api/dao/AnalyzedImageDao.ktcore/room/api/src/main/java/com/example/metasearch/core/room/api/entity/AnalyzedImageEntity.ktcore/room/impl/src/main/java/com/example/metasearch/core/room/impl/database/AppDatabase.ktcore/room/impl/src/main/java/com/example/metasearch/core/room/impl/di/DatabaseModule.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci-build
🔇 Additional comments (7)
core/room/api/src/main/java/com/example/metasearch/core/room/api/entity/AnalyzedImageEntity.kt (1)
16-17: LGTM!The new
fileNamefield is correctly defined with proper Room annotations. The non-null type aligns with the migration's DEFAULT value, ensuring data integrity for existing rows.core/room/impl/src/main/java/com/example/metasearch/core/room/impl/database/AppDatabase.kt (1)
30-34: LGTM!The migration correctly adds the
file_namecolumn with a NOT NULL constraint and a sensible default value for existing rows. This ensures backward compatibility while maintaining data integrity.core/room/api/src/main/java/com/example/metasearch/core/room/api/dao/AnalyzedImageDao.kt (2)
15-16: LGTM!Batch insert method with REPLACE strategy correctly handles bulk insertion of analyzed image paths.
24-25: LGTM!The query correctly retrieves the stored file name for a given path. The nullable return type appropriately handles cases where the path doesn't exist, and LIMIT 1 optimizes the query.
core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/ImageAnalysisRepositoryImpl.kt (3)
78-87: Good refactoring to track fileName alongside path.The change from
List<String>toList<Pair<String, String>>enables proper tracking of server file names, which is the key fix for the infinite delete loop issue.
101-118: Excellent fix for the 404 infinite delete loop.Using the saved
fileNamefrom the database instead of querying MediaStore is the correct solution. This handles both the encoding issue and the permission loss scenario mentioned in the PR objectives.The fallback to
"unknown.jpg"provides safety, though it's worth noting this could theoretically delete the wrong file if a previous migration failed and left actual filenames as "unknown.jpg".
122-133: Good performance improvement with parallel chunk upload.Using
supervisorScopewithasync/awaitAllcorrectly parallelizes the chunk uploads while ensuring that one failure doesn't cancel other uploads. ThefilterNotNull()properly handles partial success.
| ): List<Pair<String, String>> = supervisorScope { | ||
| chunk.map { uri -> | ||
| async { | ||
| val fileName = galleryRepository.getFileName(uri) ?: "unknown.jpg" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the implementation of getFileName
ast-grep --pattern $'fun getFileName($$$): String? {
$$$
}'Repository: komodgn/meta-android
Length of output: 46
🏁 Script executed:
# Also search for getFileName method definitions more broadly
rg -t kt 'fun getFileName' -A 10Repository: komodgn/meta-android
Length of output: 90
🏁 Script executed:
# Check if there's a GalleryRepository interface definition
rg -t kt 'interface GalleryRepository|class.*GalleryRepository' -A 20Repository: komodgn/meta-android
Length of output: 90
🏁 Script executed:
# Search for getFileName without type constraints
rg 'fun getFileName' -A 10Repository: komodgn/meta-android
Length of output: 3032
🏁 Script executed:
# Find all Kotlin files and their locations
fd -e kt -e java . core/Repository: komodgn/meta-android
Length of output: 7521
🏁 Script executed:
# Look at the ImageAnalysisRepositoryImpl file directly
cat -n core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/ImageAnalysisRepositoryImpl.kt | head -150 | tail -50Repository: komodgn/meta-android
Length of output: 2537
getFileName can legitimately return null—ensure null uploads don't accumulate.
The implementation queries MediaStore.Images.Media.DISPLAY_NAME, which can return null if the query fails or the URI yields no results (e.g., invalid URI, deleted file, missing display name, or permission issues). The fallback to "unknown.jpg" is a reasonable defensive mechanism, and this pattern is consistently applied elsewhere in the code (e.g., line 105 in the delete operation). However, if null returns occur frequently in normal operation with valid URIs, files would be uploaded without proper names, defeating the purpose of the fix. Monitor logs to ensure this fallback is rare and investigate if it occurs unexpectedly.
🤖 Prompt for AI Agents
In
@core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/ImageAnalysisRepositoryImpl.kt
around line 129, The fallback to "unknown.jpg" when
galleryRepository.getFileName(uri) returns null can mask frequent failures;
update ImageAnalysisRepositoryImpl where fileName is assigned to detect the null
case from getFileName(uri) and (1) log a warning including the uri and context
using the existing logger, (2) attempt a better deterministic fallback (e.g.,
derive a name from the uri path or timestamp) before using "unknown.jpg", and
(3) ensure any upload/delete code that relies on this name treats "unknown.jpg"
as exceptional; reference getFileName, galleryRepository, and the fileName usage
so you can add logging and the alternative fallback in the same assignment spot.
| analyzedImageDao.runInTransaction { | ||
| if (newMax > lastIndex) { | ||
| personIndexDataSource.setLastPersonIndex(newMax) | ||
| } | ||
|
|
||
| response.images.forEach { person -> | ||
| runCatching { | ||
| if (person.isFaceExit && person.imageName != null && person.imageBytes != null) { | ||
| val decodedBytes = decode(person.imageBytes, android.util.Base64.DEFAULT) | ||
|
|
||
| val existingPersonId = personRepository.getPersonIdByImageName(person.imageName!!) | ||
|
|
||
| if (existingPersonId != null) { | ||
| Log.d(tag, "기존 인물 매핑 성공: ${person.imageName} -> ID:$existingPersonId") | ||
| personRepository.addFaceToExistingPerson(existingPersonId, person.imageName!!, decodedBytes) | ||
| } else { | ||
| Log.d(tag, "새로운 인물 생성: ${person.imageName}") | ||
| personRepository.addAnalyzedPerson(person.imageName!!, decodedBytes) | ||
| } | ||
| } else { | ||
| Log.d(tag, "새로운 인물 생성: ${person.imageName}") | ||
| personRepository.addAnalyzedPerson(person.imageName!!, decodedBytes) | ||
| Log.w(tag, "저장 스킵됨: 얼굴없음(${!person.isFaceExit}) 또는 데이터가 null임") | ||
| } | ||
| } else { | ||
| Log.w(tag, "저장 스킵됨: 얼굴없음(${!person.isFaceExit}) 또는 데이터가 null임") | ||
| } | ||
| }.onFailure { e -> Log.e(tag, "인물 개별 저장 실패: ${person.imageName}", e) } | ||
| }.onFailure { e -> Log.e(tag, "인물 개별 저장 실패: ${person.imageName}", e) } | ||
| } | ||
|
|
||
| val entries = successfulPaths.map { (path, fileName) -> | ||
| AnalyzedImageEntity(imagePath = path, fileName = fileName) | ||
| } | ||
| analyzedImageDao.insertAllPaths(entries) | ||
| } |
There was a problem hiding this comment.
Transaction will not work as implemented.
The analyzedImageDao.runInTransaction call does not provide actual transactional guarantees (see the DAO review comment). This means that if any operation within this block fails, the other operations may still commit, potentially causing data inconsistency between:
- Person index updates
- Person/face additions
- Analyzed image path insertions
This issue is addressed in the review comment for AnalyzedImageDao.kt. Please apply the suggested fix using database.withTransaction { }.
🤖 Prompt for AI Agents
In
@core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/ImageAnalysisRepositoryImpl.kt
around lines 172-201, The current use of analyzedImageDao.runInTransaction does
not provide real transactional guarantees; replace that block with a call to the
Room database's withTransaction (e.g., database.withTransaction { ... }) so the
personIndexDataSource.setLastPersonIndex, personRepository add/get methods and
analyzedImageDao.insertAllPaths run inside a single transaction; move the entire
block currently inside analyzedImageDao.runInTransaction into
database.withTransaction { } and ensure you call DAO methods (analyzedImageDao)
and repository operations from within that transaction context so failures roll
back atomically.
| @Transaction | ||
| suspend fun runInTransaction(action: suspend () -> Unit) { | ||
| action() | ||
| } |
There was a problem hiding this comment.
Room @transaction does not wrap arbitrary lambda blocks.
The @Transaction annotation on runInTransaction will not provide transactional semantics for the action lambda. Room's @Transaction only works for database operations directly invoked within the annotated DAO method—it does not wrap arbitrary suspend lambdas passed as parameters.
To execute a transaction with multiple operations, use database.withTransaction { } from the repository layer or define a concrete @Transaction DAO method that calls multiple DAO operations directly.
🔎 Recommended fix using RoomDatabase.withTransaction
Remove this method from the DAO:
- @Transaction
- suspend fun runInTransaction(action: suspend () -> Unit) {
- action()
- }In the repository (ImageAnalysisRepositoryImpl.kt), inject the AppDatabase instance and use:
// Add to constructor
private val database: AppDatabase
// In processAnalysisFinish
database.withTransaction {
// existing transaction logic
}Or define a proper @Transaction DAO method that directly invokes other DAO operations.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@core/room/api/src/main/java/com/example/metasearch/core/room/api/dao/AnalyzedImageDao.kt
around lines 27-30, The runInTransaction(suspend () -> Unit) DAO method is
ineffective because Room’s @Transaction does not wrap lambdas; remove the
runInTransaction method from AnalyzedImageDao and instead perform transactional
work with AppDatabase.withTransaction from the repository layer (inject
AppDatabase into ImageAnalysisRepositoryImpl and wrap the existing transaction
logic in database.withTransaction { ... }) or alternatively replace the DAO
helper with a concrete @Transaction DAO method that directly calls the required
DAO operations (e.g., a new @Transaction method that orchestrates the multiple
DAO calls).
| DATABASE_NAME, | ||
| ).build() | ||
| ) | ||
| .addMigrations(AppDatabase.MIGRATION_1_2, AppDatabase.MIGRATION_3_4) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for MIGRATION_2_3 and migration definitions
rg -n "MIGRATION_2_3|MIGRATION_[0-9]_[0-9]" --type kotlin -A 2Repository: komodgn/meta-android
Length of output: 1473
🏁 Script executed:
# Find AppDatabase file to check all migration definitions
fd -e "kt" "AppDatabase" --exec grep -l "MIGRATION" {} \;Repository: komodgn/meta-android
Length of output: 156
🏁 Script executed:
# Get the DatabaseModule.kt file context around line 29
fd -e "kt" "DatabaseModule" --exec cat -n {} \;Repository: komodgn/meta-android
Length of output: 1722
🏁 Script executed:
# Get AppDatabase class definition to see version annotation
cat -n core/room/impl/src/main/java/com/example/metasearch/core/room/impl/database/AppDatabase.kt | head -40Repository: komodgn/meta-android
Length of output: 1748
🏁 Script executed:
# Search for any reference to database version or migration fallback strategies
rg -n "version|Schema|fallbackToDestructiveMigration" --type kotlin core/room/impl/src/main/java/com/example/metasearch/core/room/impl/database/Repository: komodgn/meta-android
Length of output: 295
Add missing MIGRATION_2_3 to complete the migration chain.
The database schema version is 4, but the migration path is broken: MIGRATION_1_2 (1→2) and MIGRATION_3_4 (3→4) are defined, but MIGRATION_2_3 is missing. Any production database at version 2 will fail to migrate to version 4. Define MIGRATION_2_3 or document why the gap exists and that no databases are at version 2.
🤖 Prompt for AI Agents
In
@core/room/impl/src/main/java/com/example/metasearch/core/room/impl/di/DatabaseModule.kt
around line 29, The migration chain is incomplete: update the call to
addMigrations on the Room database builder to include AppDatabase.MIGRATION_2_3
between MIGRATION_1_2 and MIGRATION_3_4 (i.e.,
.addMigrations(AppDatabase.MIGRATION_1_2, AppDatabase.MIGRATION_2_3,
AppDatabase.MIGRATION_3_4)); if MIGRATION_2_3 intentionally doesn’t exist,
either implement AppDatabase.MIGRATION_2_3 with the schema changes from v2→v3 or
add a clear comment/validation that no production DBs are at version 2 and
handle/throw a descriptive error during initialization.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
개요
주요 변경 사항
AnalyzedImageEntity에file_name컬럼 추가 (서버 식별자 통일)supervisorScope와chunked적용📄 결과
삭제 로직 성공 확인
서버 파일명 확인
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.