[Feature] Implement flower event append and upload multipart image to S3#130
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds S3 byte-upload support and S3 upload result types; introduces REST endpoints and DTOs for bulk creation and thumbnail upload for FlowerEvent and FlowerSpotCafe; implements facades/appender services coordinating S3 uploads and persistence; extends repositories and DB core with save/updateThumbnailUrl and JPA/JTS geometry mapping. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as Controller
participant Facade as Facade
participant ImageS3Caller as ImageS3Caller
participant AwsS3 as AWS S3
participant Appender as Appender
participant DB as Repository/Database
Client->>Controller: POST /admin/.../thumbnail (multipart)
Controller->>Facade: uploadThumbnail(entityId, imageBytes)
Facade->>Facade: verify entity exists
Facade->>ImageS3Caller: uploadImage(prefix, id, subPath, contentType, bytes)
ImageS3Caller->>AwsS3: putObject(bucket, s3Key, contentType, bytes)
AwsS3-->>ImageS3Caller: S3UploadResult(s3Key, publicUrl)
ImageS3Caller-->>Facade: S3UploadResult
Facade->>Appender: updateThumbnailUrl(id, publicUrl)
Appender->>DB: execute thumbnailUrl JPQL update
DB-->>Appender: update result
Appender-->>Facade: done
Facade-->>Controller: HTTP 201 Created
Controller-->>Client: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
pida-core/core-domain/src/main/kotlin/com/pida/support/aws/ImageS3Caller.kt (1)
46-52: Consider wrapping blocking S3 call in IO dispatcher at call sites.The
uploadImagemethod performs blocking I/O (S3 PUT) but is defined as a non-suspend function, following the pattern ofcreateUploadUrl. When called from suspend contexts (likeFlowerEventAdminFacade.uploadThumbnail), consider wrapping inwithContext(Dispatchers.IO)to avoid blocking coroutine threads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-domain/src/main/kotlin/com/pida/support/aws/ImageS3Caller.kt` around lines 46 - 52, The uploadImage function performs blocking S3 I/O but is declared non-suspend, so callers running in coroutines (e.g., FlowerEventAdminFacade.uploadThumbnail) should avoid blocking coroutine threads; either change callers to wrap calls to ImageS3Caller.uploadImage in withContext(Dispatchers.IO) or refactor uploadImage itself to be a suspend function that wraps its blocking S3 PUT in withContext(Dispatchers.IO) before executing the S3 client call; update all uses (including the pattern used for createUploadUrl) to ensure the blocking call runs on the IO dispatcher.pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt (1)
82-85: Potential double-slash in public URL ifimageOriginUrlends with/.If
awsProperties.s3.imageOriginUrlis configured with a trailing slash, the constructed URL will contain a double slash (e.g.,https://cdn.example.com//flowerevent/...). Consider normalizing the URL construction.🔧 Proposed fix
return S3UploadResult( s3Key = s3Key, - publicUrl = "${awsProperties.s3.imageOriginUrl}/$s3Key", + publicUrl = "${awsProperties.s3.imageOriginUrl.trimEnd('/')}/$s3Key", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt` around lines 82 - 85, Normalize the public URL assembly to avoid a double slash when awsProperties.s3.imageOriginUrl may end with a trailing slash: in the code that returns S3UploadResult (the s3Key and publicUrl construction), trim any trailing slash from awsProperties.s3.imageOriginUrl (or ensure exactly one separator) before concatenating with s3Key so publicUrl becomes "${normalizedImageOriginUrl}/$s3Key" (use a helper or call like .removeSuffix("/") on imageOriginUrl before building the URL).pida-core/core-domain/src/main/kotlin/com/pida/flowerevent/NewFlowerEvent.kt (1)
7-16: Consider adding domain invariant validations.The domain model lacks validation for:
- Coordinate bounds (
longitudeshould be -180 to 180,latitudeshould be -90 to 90)- Date ordering (
startDateshould not be afterendDate)While validation in the request layer is primary, domain invariants provide defense-in-depth.
🛡️ Proposed validation
data class NewFlowerEvent( val name: String, val address: String?, val longitude: Double, val latitude: Double, val region: Region, val homepageUrl: String?, val startDate: LocalDate, val endDate: LocalDate, val categoryId: Long, ) { + init { + require(longitude in -180.0..180.0) { "longitude must be between -180 and 180" } + require(latitude in -90.0..90.0) { "latitude must be between -90 and 90" } + require(!startDate.isAfter(endDate)) { "startDate must not be after endDate" } + } + fun toDomain(): FlowerEvent =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-domain/src/main/kotlin/com/pida/flowerevent/NewFlowerEvent.kt` around lines 7 - 16, Add domain invariant checks inside the NewFlowerEvent data class (use an init block) to validate longitude is within -180..180 and latitude within -90..90 and that startDate is not after endDate; if any check fails throw an appropriate exception (e.g., IllegalArgumentException) with a clear message referencing the offending property (longitude, latitude, startDate/endDate) so the class enforces these invariants at construction.pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerevent/request/FlowerEventCreateRequest.kt (1)
29-35: Add validation for coordinates, dates, and required fields.The init block validates
homepageUrlformat but misses other important validations:
nameshould not be blanklongitudeshould be in range -180 to 180latitudeshould be in range -90 to 90startDateshould not be afterendDate🛡️ Proposed validation additions
init { + require(name.isNotBlank()) { "name must not be blank" } + require(longitude in -180.0..180.0) { "longitude must be between -180 and 180" } + require(latitude in -90.0..90.0) { "latitude must be between -90 and 90" } + require(!startDate.isAfter(endDate)) { "startDate must not be after endDate" } if (!homepageUrl.isNullOrBlank()) { require(URL_PATTERN.matches(homepageUrl)) { "Invalid homepage URL format: $homepageUrl" } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerevent/request/FlowerEventCreateRequest.kt` around lines 29 - 35, The init block in FlowerEventCreateRequest currently only validates homepageUrl via URL_PATTERN; add checks for required and range/date constraints: require name is not blank (validate name), require longitude in -180..180 (validate longitude), require latitude in -90..90 (validate latitude), and require startDate is not after endDate (validate startDate and endDate). Implement these using require(...) with clear messages inside the same init block (alongside the existing homepageUrl check) and reference the fields name, longitude, latitude, startDate, endDate and existing URL_PATTERN to locate where to add the validations.pida-core/core-domain/src/main/kotlin/com/pida/flowerevent/FlowerEventRepository.kt (1)
23-26: Consider returning update confirmation or throwing on not found.
updateThumbnailUrlreturnsUnit, so callers cannot distinguish between a successful update and a no-op (e.g., ifeventIddoesn't exist or is soft-deleted). Consider returning the affected row count or throwing an exception when no rows are updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-domain/src/main/kotlin/com/pida/flowerevent/FlowerEventRepository.kt` around lines 23 - 26, The updateThumbnailUrl method in FlowerEventRepository currently returns Unit so callers can't tell if the DB update succeeded; change updateThumbnailUrl to return a meaningful result (e.g., Int affectedRows or Boolean) or throw an exception when no rows are updated, and update its implementation to inspect the JDBC/ORM update result (the affected row count) and either return that value or throw a NotFoundException; update callers to handle the new return/exception behavior accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerevent/FlowerEventController.kt`:
- Around line 18-20: The controller FlowerEventController is mapped to a public
path via `@RequestMapping`("/test") and is thus unauthenticated; change its
mapping and/or add authorization annotations so these admin endpoints require an
admin principal — for example, replace the "/test" base path with a protected
admin path (e.g. "/admin/flower-events") and annotate the controller or its
methods (e.g. the class FlowerEventController or
createFlowerEvent/uploadThumbnail methods) with an authorization annotation your
app uses such as `@PreAuthorize`("hasRole('ADMIN')") or `@Secured`("ROLE_ADMIN") to
ensure only authenticated admins can call them before merging.
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerevent/FlowerEventAdminFacade.kt`:
- Around line 21-25: The upload currently hardcodes contentType = "image/jpeg"
in imageS3Caller.uploadImage (called from FlowerEventAdminFacade related to
eventId), causing PNG/WebP to be stored with wrong metadata; change the call to
pass a validated content type from the incoming multipart (or enforce/convert to
JPEG earlier) so uploadImage receives the real MIME type (e.g., derive and
validate contentType from the MultipartFile/part in the controller or service
before calling uploadImage and reject unsupported types), and ensure
imageS3Caller.uploadImage's contentType parameter is used rather than being
hardcoded.
- Line 12: processBatch currently calls flowerEventAppender.add for each item
which delegates to FlowerEventCoreRepository.save in its own writer transaction,
causing partial commits on failure; either make the whole batch atomic by moving
the transaction boundary out of add/save and wrapping the entire loop in a
single writer transaction (e.g., add a transactional method processBatchAtomic
that converts all NewFlowerEvent to domain and calls repository.saveAll inside
one transaction) or change processBatch to return per-item results/IDs (e.g.,
List<Result/Id>) so callers can handle partial failures and implement
idempotency; update processBatch (and related callers) to use the new
transactional method or to consume the per-item result list and ensure
FlowerEventCoreRepository.save no longer opens its own independent transaction
when using the atomic path.
- Around line 20-29: The current flow uploads to S3 via
imageS3Caller.uploadImage(...) and only then calls
flowerEventAppender.updateThumbnailUrl(eventId, uploadResult.publicUrl), risking
S3/DB drift if the DB write fails; change to a transactional-safe pattern:
either 1) create a pending-thumbnail DB record in the same writer transaction
(e.g. set thumbnail_status = "pending" or store a pending_upload_id) before
calling imageS3Caller.uploadImage, then after successful upload set the final
URL via a retryable background job that calls updateThumbnailUrl(eventId,
publicUrl) and marks pending as completed, or 2) keep the current order but add
compensation by catching failures of flowerEventAppender.updateThumbnailUrl and
immediately deleting the uploaded object (via imageS3Caller.deleteImage or
equivalent) with retries and logging; implement one of these strategies around
imageS3Caller.uploadImage and flowerEventAppender.updateThumbnailUrl to ensure
eventual consistency and retries.
In
`@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerevent/FlowerEventCustomRepository.kt`:
- Around line 18-25: The update JPQL in FlowerEventCustomRepository updates
FlowerEventEntity::thumbnailUrl filtering only by FlowerEventEntity::id
(eventId) and thus may affect soft-deleted rows and currently discards the
entityManager.createQuery(...).executeUpdate() result; modify the where clause
to include a soft-delete check (path(FlowerEventEntity::deletedAt).isNull() or
equivalent) alongside path(FlowerEventEntity::id).eq(eventId) and capture the
executeUpdate() return value from entityManager.createQuery(query,
jdslRenderContext).executeUpdate(); then handle the affected-row count (e.g.,
return it or throw an exception if zero) consistent with other repository
patterns in the module.
---
Nitpick comments:
In
`@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt`:
- Around line 82-85: Normalize the public URL assembly to avoid a double slash
when awsProperties.s3.imageOriginUrl may end with a trailing slash: in the code
that returns S3UploadResult (the s3Key and publicUrl construction), trim any
trailing slash from awsProperties.s3.imageOriginUrl (or ensure exactly one
separator) before concatenating with s3Key so publicUrl becomes
"${normalizedImageOriginUrl}/$s3Key" (use a helper or call like
.removeSuffix("/") on imageOriginUrl before building the URL).
In
`@pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerevent/request/FlowerEventCreateRequest.kt`:
- Around line 29-35: The init block in FlowerEventCreateRequest currently only
validates homepageUrl via URL_PATTERN; add checks for required and range/date
constraints: require name is not blank (validate name), require longitude in
-180..180 (validate longitude), require latitude in -90..90 (validate latitude),
and require startDate is not after endDate (validate startDate and endDate).
Implement these using require(...) with clear messages inside the same init
block (alongside the existing homepageUrl check) and reference the fields name,
longitude, latitude, startDate, endDate and existing URL_PATTERN to locate where
to add the validations.
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerevent/FlowerEventRepository.kt`:
- Around line 23-26: The updateThumbnailUrl method in FlowerEventRepository
currently returns Unit so callers can't tell if the DB update succeeded; change
updateThumbnailUrl to return a meaningful result (e.g., Int affectedRows or
Boolean) or throw an exception when no rows are updated, and update its
implementation to inspect the JDBC/ORM update result (the affected row count)
and either return that value or throw a NotFoundException; update callers to
handle the new return/exception behavior accordingly.
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerevent/NewFlowerEvent.kt`:
- Around line 7-16: Add domain invariant checks inside the NewFlowerEvent data
class (use an init block) to validate longitude is within -180..180 and latitude
within -90..90 and that startDate is not after endDate; if any check fails throw
an appropriate exception (e.g., IllegalArgumentException) with a clear message
referencing the offending property (longitude, latitude, startDate/endDate) so
the class enforces these invariants at construction.
In `@pida-core/core-domain/src/main/kotlin/com/pida/support/aws/ImageS3Caller.kt`:
- Around line 46-52: The uploadImage function performs blocking S3 I/O but is
declared non-suspend, so callers running in coroutines (e.g.,
FlowerEventAdminFacade.uploadThumbnail) should avoid blocking coroutine threads;
either change callers to wrap calls to ImageS3Caller.uploadImage in
withContext(Dispatchers.IO) or refactor uploadImage itself to be a suspend
function that wraps its blocking S3 PUT in withContext(Dispatchers.IO) before
executing the S3 client call; update all uses (including the pattern used for
createUploadUrl) to ensure the blocking call runs on the IO dispatcher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5404e912-6a76-4799-9df1-34882a44ce3a
📒 Files selected for processing (12)
pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.ktpida-clients/aws-client/src/main/kotlin/com/pida/client/aws/s3/AwsS3Client.ktpida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerevent/FlowerEventController.ktpida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerevent/request/FlowerEventCreateRequest.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerevent/FlowerEventAdminFacade.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerevent/FlowerEventAppender.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerevent/FlowerEventRepository.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerevent/NewFlowerEvent.ktpida-core/core-domain/src/main/kotlin/com/pida/support/aws/ImageS3Caller.ktpida-core/core-domain/src/main/kotlin/com/pida/support/aws/S3UploadResult.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerevent/FlowerEventCoreRepository.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerevent/FlowerEventCustomRepository.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotCafeFacade.kt (1)
12-12: Consider batch insert for improved performance.
addAllperforms N individualadd()calls, resulting in N separate database transactions. For bulk operations, a batch insert would be more efficient.♻️ Alternative: Add batch method to repository
// In FlowerSpotCafeRepository suspend fun saveAll(cafes: List<FlowerSpotCafe>): List<FlowerSpotCafe> // In FlowerSpotCafeFacade suspend fun addAll(requests: List<NewFlowerSpotCafe>) { val cafes = requests.map { it.toDomain() } flowerSpotCafeAppender.addAll(cafes) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotCafeFacade.kt` at line 12, The addAll implementation calls flowerSpotCafeAppender.add for each NewFlowerSpotCafe, causing N separate DB transactions; change it to perform a batch save by mapping requests to domain objects and delegating to a repository/appender batch method (e.g., addAll or saveAll) — add a suspend fun saveAll(cafes: List<FlowerSpotCafe>): List<FlowerSpotCafe> to FlowerSpotCafeRepository (or addAll to flowerSpotCafeAppender), then implement FlowerSpotCafeFacade.addAll to val cafes = requests.map { it.toDomain() } and call the new batch method to persist them in one operation.pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerspotcafe/request/FlowerSpotCafeCreateRequest.kt (1)
17-20: Consider adding coordinate range validation.
longitudeandlatitudeare accepted without range validation. Invalid coordinates (e.g., latitude > 90 or longitude > 180) would persist invalid geospatial data.💡 Optional: Add coordinate validation in init block
init { + require(latitude in -90.0..90.0) { "Latitude must be between -90 and 90: $latitude" } + require(longitude in -180.0..180.0) { "Longitude must be between -180 and 180: $longitude" } if (!mapUrl.isNullOrBlank()) { require(URL_PATTERN.matches(mapUrl)) { "Invalid map URL format: $mapUrl" } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerspotcafe/request/FlowerSpotCafeCreateRequest.kt` around lines 17 - 20, Add range validation for coordinates in the FlowerSpotCafeCreateRequest data class: in an init block (or a dedicated validate method) on the class, assert that latitude is between -90 and 90 and longitude is between -180 and 180 (use require/throw IllegalArgumentException on failure) so invalid values for the properties longitude and latitude are rejected before persistence.pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotCafeCustomRepository.kt (1)
14-26: Consider validating that the update affected a row.
executeUpdate()returns the number of affected rows. IfcafeIddoesn't exist, zero rows are updated with no error. While the facade validates existence beforehand viaflowerSpotCafeFinder.readBy(cafeId), a TOCTOU gap exists between the check and update. Consider returning or checking the affected row count for defensive integrity.💡 Optional: Return affected row count for validation
fun updateThumbnailUrl( cafeId: Long, thumbnailUrl: String, - ) { + ): Int { val query = jpql { update(entity(FlowerSpotCafeEntity::class)) .set(path(FlowerSpotCafeEntity::thumbnailUrl), thumbnailUrl) .where(path(FlowerSpotCafeEntity::id).eq(cafeId)) } - entityManager.createQuery(query, jdslRenderContext).executeUpdate() + return entityManager.createQuery(query, jdslRenderContext).executeUpdate() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotCafeCustomRepository.kt` around lines 14 - 26, The updateThumbnailUrl method in FlowerSpotCafeCustomRepository should check the result of entityManager.createQuery(...).executeUpdate(); capture the returned affected row count and treat zero as a failure (TOCTOU protection) — either return the count from updateThumbnailUrl or throw a NotFound/IllegalState exception if the count is 0; update the method signature (updateThumbnailUrl) accordingly and use the result of executeUpdate() to drive that behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerspotcafe/FlowerSpotCafeController.kt`:
- Around line 42-47: The uploadThumbnail endpoint (function uploadThumbnail)
must validate the MultipartFile before calling
flowerSpotCafeFacade.uploadThumbnail: check thumbnail.isEmpty() and return/throw
a 400 if empty, enforce a configurable max size (e.g., reject if thumbnail.size
> MAX_BYTES) to avoid loading large files into memory, validate the content type
(allow only accepted image MIME types like image/jpeg and image/png) and
normalize/derive the stored content-type instead of blindly assuming image/jpeg,
and only after these checks safely read the bytes/stream (avoid calling
thumbnail.bytes before validation); throw or return appropriate HTTP Bad Request
errors when validations fail.
- Around line 18-23: The controller exposes unsecured admin endpoints and
performs unsafe file handling; update FlowerSpotCafeController to require
authentication and restrict to admins (e.g., add class- or method-level security
like `@PreAuthorize`("hasRole('ADMIN')") or move the `@RequestMapping` from "/test"
to an authenticated admin path such as "/api/v1/admin/flowerspot"), add `@Valid`
to the request body parameter used in the controller methods so validation
annotations are enforced, and protect the multipart handling in the upload
method by validating file size before using thumbnail.bytes (reject if over
configured MAX_UPLOAD_SIZE) and stream the file to S3 via InputStream rather
than loading full bytes into memory; if these endpoints are purely test-only,
wrap the controller with a profile/conditional annotation (e.g.,
`@Profile`("!prod") or `@ConditionalOnProperty`) to disable in production.
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotCafeFacade.kt`:
- Around line 20-27: The facade currently hardcodes contentType = "image/jpeg"
when calling imageS3Caller.uploadImage (see uploadResult assignment), which
mislabels non-JPEG uploads; update the FlowerSpotCafeFacade.uploadThumbnail
signature to accept a contentType: String parameter and pass that value into
imageS3Caller.uploadImage's contentType argument, and update the controller call
to forward thumbnail.contentType ?: "application/octet-stream" into
uploadThumbnail so the real MultipartFile.contentType is preserved.
In
`@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotCafeCoreRepository.kt`:
- Around line 57-72: The code in save (override suspend fun save(cafe:
FlowerSpotCafe)) unsafely casts cafe.pinPoint with "as GeoJson.Point" inside the
tx.writer.coExecute block; replace that unsafe cast with an explicit type check
(use a safe cast or a when) and handle non-Point values explicitly—e.g., if
cafe.pinPoint is a GeoJson.Point extract coordinates and create
GEOMETRY_FACTORY.createPoint(...), otherwise throw a clear
IllegalArgumentException (or map other GeoJson types if intended) before
constructing FlowerSpotCafeEntity and calling
flowerSpotCafeJpaRepository.save(...).
---
Nitpick comments:
In
`@pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerspotcafe/request/FlowerSpotCafeCreateRequest.kt`:
- Around line 17-20: Add range validation for coordinates in the
FlowerSpotCafeCreateRequest data class: in an init block (or a dedicated
validate method) on the class, assert that latitude is between -90 and 90 and
longitude is between -180 and 180 (use require/throw IllegalArgumentException on
failure) so invalid values for the properties longitude and latitude are
rejected before persistence.
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotCafeFacade.kt`:
- Line 12: The addAll implementation calls flowerSpotCafeAppender.add for each
NewFlowerSpotCafe, causing N separate DB transactions; change it to perform a
batch save by mapping requests to domain objects and delegating to a
repository/appender batch method (e.g., addAll or saveAll) — add a suspend fun
saveAll(cafes: List<FlowerSpotCafe>): List<FlowerSpotCafe> to
FlowerSpotCafeRepository (or addAll to flowerSpotCafeAppender), then implement
FlowerSpotCafeFacade.addAll to val cafes = requests.map { it.toDomain() } and
call the new batch method to persist them in one operation.
In
`@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotCafeCustomRepository.kt`:
- Around line 14-26: The updateThumbnailUrl method in
FlowerSpotCafeCustomRepository should check the result of
entityManager.createQuery(...).executeUpdate(); capture the returned affected
row count and treat zero as a failure (TOCTOU protection) — either return the
count from updateThumbnailUrl or throw a NotFound/IllegalState exception if the
count is 0; update the method signature (updateThumbnailUrl) accordingly and use
the result of executeUpdate() to drive that behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72e59e8d-64e0-4300-825e-ab190f73dc75
📒 Files selected for processing (10)
pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerevent/FlowerEventController.ktpida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerspotcafe/FlowerSpotCafeController.ktpida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerspotcafe/request/FlowerSpotCafeCreateRequest.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerevent/FlowerEventFacade.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotCafeAppender.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotCafeFacade.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotCafeRepository.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/NewFlowerSpotCafe.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotCafeCoreRepository.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotCafeCustomRepository.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- pida-core/core-api/src/main/kotlin/com/pida/presentation/v2/flowerevent/FlowerEventController.kt
Comment on lines
+57
to
+72
| override suspend fun save(cafe: FlowerSpotCafe): FlowerSpotCafe = | ||
| tx.writer.coExecute { | ||
| val point = cafe.pinPoint as GeoJson.Point | ||
| val entity = | ||
| FlowerSpotCafeEntity( | ||
| flowerSpotId = cafe.flowerSpotId, | ||
| name = cafe.name, | ||
| address = cafe.address, | ||
| description = cafe.description, | ||
| thumbnailUrl = cafe.thumbnailUrl, | ||
| pinPoint = GEOMETRY_FACTORY.createPoint(Coordinate(point.coordinates[0], point.coordinates[1])), | ||
| region = cafe.region, | ||
| mapUrl = cafe.mapUrl, | ||
| ) | ||
| flowerSpotCafeJpaRepository.save(entity).toFlowerSpotCafe() | ||
| } |
Contributor
There was a problem hiding this comment.
Unsafe cast to GeoJson.Point may throw ClassCastException.
Line 59 uses as GeoJson.Point which will fail at runtime if cafe.pinPoint is a different GeoJson subtype. While currently all callers provide a Point, this is fragile if the domain model evolves.
🛡️ Safer approach with explicit type check
override suspend fun save(cafe: FlowerSpotCafe): FlowerSpotCafe =
tx.writer.coExecute {
- val point = cafe.pinPoint as GeoJson.Point
+ val point = cafe.pinPoint as? GeoJson.Point
+ ?: throw IllegalArgumentException("FlowerSpotCafe pinPoint must be a GeoJson.Point")
val entity =
FlowerSpotCafeEntity(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotCafeCoreRepository.kt`
around lines 57 - 72, The code in save (override suspend fun save(cafe:
FlowerSpotCafe)) unsafely casts cafe.pinPoint with "as GeoJson.Point" inside the
tx.writer.coExecute block; replace that unsafe cast with an explicit type check
(use a safe cast or a when) and handle non-Point values explicitly—e.g., if
cafe.pinPoint is a GeoJson.Point extract coordinates and create
GEOMETRY_FACTORY.createPoint(...), otherwise throw a clear
IllegalArgumentException (or map other GeoJson types if intended) before
constructing FlowerSpotCafeEntity and calling
flowerSpotCafeJpaRepository.save(...).
char-yb
approved these changes
Mar 25, 2026
This was referenced Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🌱 관련 이슈
📌 작업 내용 및 특이 사항
📝 참고
📌 체크 리스트
Summary by CodeRabbit