Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIn-memory caches and per-key locking removed; repository-backed persistence added for locations, messaging tokens, and riddles; new token/timestamp fields and scheduled cleanups introduced; login save gained conflict-recovery; riddle microservice, cache managers, and related controllers/services were deleted; push-notification token settings added. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant LocationService
participant UserRepository
participant LocationRepository
end
Client->>LocationService: pushLocation(LocationDto{token,userCmschId,...})
LocationService->>UserRepository: findByCmschId(userCmschId)
UserRepository-->>LocationService: Optional<UserEntity>
alt user missing or unauthorized
LocationService-->>Client: error/unauthorized
else
LocationService->>LocationRepository: findByUserId(userId)
LocationRepository-->>LocationService: Optional<LocationEntity>
alt not found by userId
LocationService->>LocationRepository: findByToken(token)
LocationRepository-->>LocationService: Optional<LocationEntity>
end
alt no existing entity
LocationService->>LocationRepository: save(new LocationEntity(token,...))
else
LocationService->>LocationRepository: save(updated LocationEntity)
end
LocationRepository-->>LocationService: saved entity
LocationService-->>Client: LocationResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt (1)
43-77: Extract a shared “find-or-create-and-save” flow to avoid drift.The same lookup/update/create/save structure is duplicated across three provider methods. Centralizing this flow (with provider-specific field mapping callbacks) will reduce divergence and make concurrency fixes consistent.
Also applies to: 79-113, 385-420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt` around lines 43 - 77, The fetchUserEntity flow duplicates a find-or-create-and-save pattern across provider methods; extract a shared helper (e.g., ensureUserForProfile) that encapsulates: find by internalId (users.findByInternalId), fallback find by email (users.findByEmailIgnoreCase), create a new UserEntity when neither exists, run a provider-specific field-mapping callback (replace updateFieldsForAuthsch/profile-specific updates), perform the read-only/read-write transactions around the update and users.save calls via transactionManager.transaction, and call adminMenuService.invalidateUser(user.internalId) before returning; update fetchUserEntity (and the other provider methods referenced) to call this helper with the appropriate mapping callback so the lookup/create/save concurrency logic is centralized.backend/src/main/kotlin/hu/bme/sch/cmsch/config/TestConfig.kt (1)
94-94: Remove the now no-op scheduled hook.After cache reset removal,
delayedInit()(Line 139–Line 144) only flips a boolean once and does no meaningful initialization. This can be safely removed along withinitedto simplify test config.♻️ Proposed cleanup
-import org.springframework.scheduling.annotation.Scheduled @@ - private var inited: Boolean = false @@ - `@Scheduled`(fixedDelay = 3000L) - fun delayedInit() { - if (inited) - return - inited = true - }Also applies to: 139-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/config/TestConfig.kt` at line 94, Remove the no-op scheduled hook and its backing boolean: delete the private var inited: Boolean and remove the delayedInit() scheduled method (and any references to inited) because delayedInit only flips the flag and performs no initialization; ensure no other code depends on inited or delayedInit and run tests to confirm no references remain.backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMappingRepository.kt (1)
22-36: Please verify indexes for the new repository-backed query shapes.These methods now sit on the request path after the cache removal, and they repeatedly filter on
ownerUserId/ownerGroupId,completed,skipped, andriddleId. IfRiddleMappingEntityis not backed by matching composite indexes, these lookups/counts will degrade into table scans under load. At minimum, theowner + riddleIdlookups and thecompleted/skipped + riddleIdcount path need coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMappingRepository.kt` around lines 22 - 36, Queries in RiddleMappingRepository (e.g., findAllByOwnerUserIdAndRiddleId, findAllByOwnerGroupIdAndRiddleId, countAllByCompletedTrueAndRiddleIdIn, countAllByCompletedTrueAndSkippedFalseAndRiddleId) will perform poorly without matching composite indexes on the RiddleMappingEntity; add database indexes that cover the common filter combinations such as (owner_user_id, riddle_id), (owner_group_id, riddle_id), (completed, riddle_id) and a more selective (completed, skipped, riddle_id) and optionally (owner_user_id, completed, riddle_id) / (owner_group_id, completed, riddle_id) so the repository methods use index lookups instead of table scans.backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt (1)
152-153: Use the newowner + riddleIdrepository methods here.Lines 152-153, 182-183, 223-224, and 309-310 still load every mapping for the owner and then filter in memory, even though
RiddleMappingRepositorynow exposes exact-match methods for these lookups. On the serializable hint/submit paths, narrowing the query will cut read volume and reduce unnecessary transaction conflicts.♻️ Suggested change
- val submission = riddleMappingRepository.findAllByOwnerUserId(user.id) - .firstOrNull { it.riddleId == riddleId } + val submission = riddleMappingRepository.findAllByOwnerUserIdAndRiddleId(user.id, riddleId) + .firstOrNull()- val submission = riddleMappingRepository.findAllByOwnerGroupId(groupId) - .firstOrNull { it.riddleId == riddleId } + val submission = riddleMappingRepository.findAllByOwnerGroupIdAndRiddleId(groupId, riddleId) + .firstOrNull()Also applies to: 182-183, 223-224, 309-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt` around lines 152 - 153, Replace in-memory filtering of mappings with the exact-match repository methods: instead of calling riddleMappingRepository.findAllByOwnerUserId(user.id).firstOrNull { it.riddleId == riddleId } (and the analogous patterns at the other locations), call the repository method that matches both owner and riddle id directly (e.g. riddleMappingRepository.findByOwnerUserIdAndRiddleId(user.id, riddleId) or findAllByOwnerUserIdAndRiddleId(...) depending on return type); update the code paths at the occurrences that currently use findAllByOwnerUserId(...).filter/firstOrNull (lines shown in the review) to use these owner+riddleId methods so the query is narrowed and avoids loading all mappings into memory.backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/MessagingTokenRepository.kt (1)
32-34: IndexupdatedAtfor the new cleanup queries.These methods introduce range scans on
updatedAt, but the table definition still only indexesuserIdand(userId, token). Add an index onupdatedAtbefore shipping the scheduled cleanup, or stale-token deletion will full-scanmessaging_tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/MessagingTokenRepository.kt` around lines 32 - 34, The new repository methods deleteByUpdatedAtBefore and findByUpdatedAtBefore will cause range scans on the updatedAt column, so add a database schema migration that creates an index on the messaging_tokens.updated_at column (e.g., CREATE INDEX idx_messaging_tokens_updated_at ON messaging_tokens(updated_at)); include a corresponding down/rollback statement, choose a clear index name, and apply the migration before deploying the scheduled cleanup so these queries use the index instead of full-table scans.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationEntity.kt`:
- Around line 34-37: The token property on LocationEntity is currently exposed
via JsonView (Edit and FullDetails) and leaks a reusable credential; remove
token from those views so it is not serialized (e.g., delete or replace
`@field`:JsonView on var token with a non-serialized annotation such as
`@JsonIgnore` or a dedicated internal-only view) while keeping the
column/persistence intact so LocationService.kt can still lookup it; update the
LocationEntity token declaration (symbol: token in class LocationEntity)
accordingly so API responses no longer include the token.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt`:
- Around line 42-69: The upsert currently only checks
locationRepository.findByToken(token) and then creates a new LocationEntity
which can violate the unique constraint on userId; update the logic in
LocationService to also resolve existing rows by userId (e.g. query
locationRepository.findByUserId(userEntity.id>) and prefer that entity when
present) or replace this block with a proper DB-level upsert/retry strategy (use
the repository/ORM upsert or catch unique-key exceptions on
locationRepository.save(entity) and re-fetch by token/userId then retry). Ensure
you still set the same fields (longitude, latitude, timestamp, broadcast via
shareLocationAllowed(token)) on the resolved entity before calling
locationRepository.save.
- Around line 107-115: refresh() updates groupName but doesn't update
markerColor, causing stale colors after a group change; modify the refresh logic
in LocationService (the block using locationRepository.findAll() and
userRepository.findByCmschId) to also recompute and set location.markerColor
whenever you update location.groupName (reuse the same helper used on insert —
e.g., the existing compute/get marker color function — or implement the
derivation inline) before calling locationRepository.save(location).
- Around line 181-195: getRecentLocations and cleanupStaleLocations are using
inverted time comparisons: change getRecentLocations to query for timestamps
greater than (now - 600) so it returns recent entries (use
clock.getTimeInSeconds() - 600), and change cleanupStaleLocations to delete
entries older than the cutoff by calling a new repository method
findAllByTimestampLessThan(cutoff) (cutoff = clock.getTimeInSeconds() -
locationComponent.visibleDuration) instead of findAllByTimestampGreaterThan; add
fun findAllByTimestampLessThan(timestamp: Long): List<LocationEntity> to
LocationRepository.
In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt`:
- Around line 45-75: Concurrent first-login requests can race: both threads can
pass the read phase and one will fail on DB unique constraints
(internalId/email), so replace the direct users.save(user) call with a
conflict-recovery helper (e.g., saveUserWithConflictRecovery) that attempts to
persist the user inside a transaction, catches unique-constraint exceptions,
re-fetches the canonical user by internalId and/or email, merges necessary
fields (or updates internalId/email if this thread found the other way), and
retries or returns the resolved entity; specifically modify the Authsch flow
around updateFieldsForAuthsch and users.save in LoginService (the block creating
UserEntity and the two other analogous upsert flows referenced in the review) to
call saveUserWithConflictRecovery(user, profile) instead, ensure transactions
are used correctly inside the helper (retry semantics, read+write transactions
as needed), and keep the final adminMenuService.invalidateUser call using the
resolved user's internalId.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/MessagingTokenEntity.kt`:
- Around line 24-28: The new non-nullable timestamp fields in
MessagingTokenEntity (createdAt, updatedAt) will cause existing legacy rows to
have updatedAt = 0 and be removed by the daily cleanupStaleTokens() job; add or
confirm a DB migration/backfill that sets sensible createdAt/updatedAt values
for all existing rows (e.g., set both to created_at or now) before rolling this
change, or alternatively make the fields nullable/default-to-now in the schema
and then run a one-off backfill, and ensure the migration is applied prior to
enabling the cleanupStaleTokens() scheduler.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt`:
- Around line 127-142: The read-then-insert in PushNotificationService around
messagingTokenRepository.findByUserIdAndToken(...) causes a race; change it to
perform an atomic upsert or handle duplicate-key errors: either use your
repository/SQL upsert (INSERT ... ON CONFLICT DO UPDATE) to set updatedAt = now
when (userId, token) exists, or wrap the current logic in a transaction and
catch the duplicate-key/DataIntegrityViolationException thrown by
messagingTokenRepository.save(...) and in that catch reload the existing
MessagingTokenEntity and update its updatedAt. Ensure the code references the
same methods (messagingTokenRepository.save, findByUserIdAndToken) and updates
updatedAt consistently so concurrent calls become idempotent.
- Around line 91-98: The warn logs in PushNotificationService (around the
invalidToken handling) currently emit the full FCM registration token variable
token; modify the logging to redact the token by replacing it with a short
deterministic identifier (e.g., last N chars or a hash) before logging. Locate
the block that computes invalidToken and the two log.warn calls and change the
logged token argument to a redactedToken computed from token (e.g.,
token.takeLast(6) or a short SHA-256/MD5 hex substring) so the code still
identifies the device in logs without exposing the full registration token.
- Around line 151-154: The scheduled cleanup in cleanupStaleTokens uses
pushNotificationComponent.tokenStaleDays directly which can be zero or negative
and make cutoff = clock.getTimeInSeconds() - staleThreshold wipe active tokens;
guard this by validating tokenStaleDays first (e.g. if tokenStaleDays < 1 then
either set staleDays = 1 or return/skip cleanup) and compute staleThreshold from
that validated value before calculating cutoff; update references in
cleanupStaleTokens (pushNotificationComponent.tokenStaleDays, staleThreshold,
cutoff, clock.getTimeInSeconds()) accordingly so the job never uses a
non-positive day value.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`:
- Around line 27-31: The current read path in RiddleBusinessLogicService
performs N+1 lookups
(riddleMappingRepository.findAllByOwnerUserIdAndCompletedTrue calling
riddleEntityRepository.findById per submission) and repeated full-table scans
(riddleCategoryRepository.findAll / riddleEntityRepository.findAll) causing poor
scaling; fix by preloading required Riddle entities in bulk (e.g., collect all
riddleIds from riddleMappingRepository.findAllByOwnerUserIdAndCompletedTrue and
call a single repository method that returns all by ids, or add repository
queries like findAllByIdIn(ids) and findAllByCategoryIdIn(categoryIds) so
filtering happens in the DB), replace per-item riddleEntityRepository.findById
calls with lookups into the preloaded map, and apply the same bulk/preload
approach to the other places mentioned (the blocks using
riddleEntityRepository.findAll / riddleCategoryRepository.findAll at the other
locations) to eliminate full-table rescans.
In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/team/TeamService.kt`:
- Around line 387-395: The percentage denominator is incorrectly using
groupRepository.count() (total teams) instead of the total number of riddles;
update the calculation in TeamService (where riddleMappingRepository is used to
compute solved/skipped and TeamStatView is constructed) to fetch the total
eligible riddles from the appropriate riddles repository (e.g., call
riddleRepository.count() or the repo method that returns total available
riddles) and use that value as the denominator (with the same zero-check) so the
percentage = solved.toFloat() / totalRiddles.toFloat().
---
Nitpick comments:
In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt`:
- Around line 43-77: The fetchUserEntity flow duplicates a
find-or-create-and-save pattern across provider methods; extract a shared helper
(e.g., ensureUserForProfile) that encapsulates: find by internalId
(users.findByInternalId), fallback find by email (users.findByEmailIgnoreCase),
create a new UserEntity when neither exists, run a provider-specific
field-mapping callback (replace updateFieldsForAuthsch/profile-specific
updates), perform the read-only/read-write transactions around the update and
users.save calls via transactionManager.transaction, and call
adminMenuService.invalidateUser(user.internalId) before returning; update
fetchUserEntity (and the other provider methods referenced) to call this helper
with the appropriate mapping callback so the lookup/create/save concurrency
logic is centralized.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/MessagingTokenRepository.kt`:
- Around line 32-34: The new repository methods deleteByUpdatedAtBefore and
findByUpdatedAtBefore will cause range scans on the updatedAt column, so add a
database schema migration that creates an index on the
messaging_tokens.updated_at column (e.g., CREATE INDEX
idx_messaging_tokens_updated_at ON messaging_tokens(updated_at)); include a
corresponding down/rollback statement, choose a clear index name, and apply the
migration before deploying the scheduled cleanup so these queries use the index
instead of full-table scans.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`:
- Around line 152-153: Replace in-memory filtering of mappings with the
exact-match repository methods: instead of calling
riddleMappingRepository.findAllByOwnerUserId(user.id).firstOrNull { it.riddleId
== riddleId } (and the analogous patterns at the other locations), call the
repository method that matches both owner and riddle id directly (e.g.
riddleMappingRepository.findByOwnerUserIdAndRiddleId(user.id, riddleId) or
findAllByOwnerUserIdAndRiddleId(...) depending on return type); update the code
paths at the occurrences that currently use
findAllByOwnerUserId(...).filter/firstOrNull (lines shown in the review) to use
these owner+riddleId methods so the query is narrowed and avoids loading all
mappings into memory.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMappingRepository.kt`:
- Around line 22-36: Queries in RiddleMappingRepository (e.g.,
findAllByOwnerUserIdAndRiddleId, findAllByOwnerGroupIdAndRiddleId,
countAllByCompletedTrueAndRiddleIdIn,
countAllByCompletedTrueAndSkippedFalseAndRiddleId) will perform poorly without
matching composite indexes on the RiddleMappingEntity; add database indexes that
cover the common filter combinations such as (owner_user_id, riddle_id),
(owner_group_id, riddle_id), (completed, riddle_id) and a more selective
(completed, skipped, riddle_id) and optionally (owner_user_id, completed,
riddle_id) / (owner_group_id, completed, riddle_id) so the repository methods
use index lookups instead of table scans.
In `@backend/src/main/kotlin/hu/bme/sch/cmsch/config/TestConfig.kt`:
- Line 94: Remove the no-op scheduled hook and its backing boolean: delete the
private var inited: Boolean and remove the delayedInit() scheduled method (and
any references to inited) because delayedInit only flips the flag and performs
no initialization; ensure no other code depends on inited or delayedInit and run
tests to confirm no references remain.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 877017d2-bdae-405d-bf55-23cec21a27ee
📒 Files selected for processing (30)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationEntity.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationRepository.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/login/InternalIdLocks.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/MessagingTokenEntity.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/MessagingTokenRepository.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationComponent.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/ConcurrentRiddleService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleApiController.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleCacheManager.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleCategoryController.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleComponent.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleController.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMappingEntity.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMappingRepository.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMicroserviceController.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMicroserviceDashboard.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddlePersistenceService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleReadonlyService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/team/TeamService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/config/StartupPropertyConfig.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/config/TestConfig.ktbackend/src/main/resources/config/application-env.propertiesbackend/src/main/resources/config/application-microservice.propertiesbackend/src/main/resources/config/application.propertieshelm/cmsch/templates/cmsch-config.ymlhelm/cmsch/values.yaml
💤 Files with no reviewable changes (11)
- backend/src/main/resources/config/application-microservice.properties
- backend/src/main/kotlin/hu/bme/sch/cmsch/config/StartupPropertyConfig.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/InternalIdLocks.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleReadonlyService.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMappingEntity.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddlePersistenceService.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleComponent.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMicroserviceController.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/ConcurrentRiddleService.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleMicroserviceDashboard.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleCacheManager.kt
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationEntity.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt
Outdated
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt
Outdated
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt
Outdated
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/team/TeamService.kt
Outdated
Show resolved
Hide resolved
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 8 file(s) based on 11 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 8 file(s) based on 11 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt (1)
125-155:⚠️ Potential issue | 🟠 MajorBatch skip-threshold checks instead of counting per riddle.
findNextTo()maps every candidate throughmapRiddleView(), andmapRiddleView()now callscannotSkip(), which hits the repository. That turns listing and post-submit responses into one extraCOUNT(*)per rendered riddle.♻️ Suggested direction
-private fun mapRiddleView(submissions: List<RiddleMappingEntity>, riddle: RiddleEntity): RiddleView { +private fun mapRiddleView( + submissions: List<RiddleMappingEntity>, + riddle: RiddleEntity, + nonSkippableRiddleIds: Set<Int> +): RiddleView { ... - skipPermitted = !submission.completed && !cannotSkip(riddle) + skipPermitted = !submission.completed && riddle.id !in nonSkippableRiddleIds ... - skipPermitted = !cannotSkip(riddle) + skipPermitted = riddle.id !in nonSkippableRiddleIdsBuild
nonSkippableRiddleIdsonce per candidate set with a grouped repository query instead of callingcountAllByCompletedTrueAndSkippedFalseAndRiddleId(...)inside the loop.Also applies to: 383-385, 460-470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt` around lines 125 - 155, mapRiddleView calls cannotSkip which issues a COUNT query per riddle (via countAllByCompletedTrueAndSkippedFalseAndRiddleId), causing N extra DB calls when findNextTo maps candidates; instead, run a single grouped repository query to compute the set of nonSkippableRiddleIds for the candidate set and change mapRiddleView to accept that Set<Long> (or add an overload) so it checks membership (nonSkippableRiddleIds.contains(riddle.id)) rather than calling cannotSkip; apply the same batching refactor where cannotSkip/COUNT is used elsewhere (references: mapRiddleView, cannotSkip, findNextTo, countAllByCompletedTrueAndSkippedFalseAndRiddleId, nonSkippableRiddleIds).
♻️ Duplicate comments (3)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt (2)
450-452:⚠️ Potential issue | 🟠 MajorKeep the Keycloak recovery path inside a write transaction.
Unlike Line 108 and Line 144, Line 451 calls
saveUserWithConflictRecovery(user)directly. The fallback path does re-read → merge → save, so this Keycloak flow is still non-atomic under concurrent first-login requests.💡 Minimal fix
updateFieldsForKeycloak(profile, user) - val savedUser = saveUserWithConflictRecovery(user) + val savedUser = transactionManager.transaction(readOnly = false) { + saveUserWithConflictRecovery(user) + } adminMenuService.invalidateUser(savedUser.internalId) return savedUser🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt` around lines 450 - 452, The Keycloak first-login path calls updateFieldsForKeycloak(profile, user) then saveUserWithConflictRecovery(user) outside a write transaction, making the re-read/merge/save fallback non-atomic; wrap the entire Keycloak flow (the call to updateFieldsForKeycloak, the subsequent saveUserWithConflictRecovery(user), and the adminMenuService.invalidateUser(savedUser.internalId) follow-up) inside the same write transaction used elsewhere so the initial save and any conflict-recovery retry happen atomically — locate the Keycloak flow that invokes updateFieldsForKeycloak and ensure it executes within the repository/DB write transaction block (the same transaction pattern used around the other saveUserWithConflictRecovery calls).
45-46:⚠️ Potential issue | 🟠 MajorVerify that
users.save(...)flushes before this helper returns.This recovery only works if Line 46 actually forces the insert/update to hit the database. If
UserService.saveis just a thinrepository.save(...), the unique-key violation will be raised on commit instead, so this catch block never runs and the first-login race still fails.🔍 Read-only verification
#!/bin/bash set -euo pipefail user_service="$(fd -a '^UserService\.kt$' backend/src/main/kotlin | head -n1)" echo "Inspecting: $user_service" sed -n '1,220p' "$user_service" echo echo "Looking for flush-capable persistence in UserService.save..." rg -n -C3 'fun save\(|saveAndFlush|flush\(' "$user_service"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt` around lines 45 - 46, The catch for unique-key recovery will never trigger if users.save(user) doesn't flush to the DB, so change the helper to force a flush after persisting: replace users.save(user) with a flush-capable call (e.g. users.saveAndFlush(user) or call entityManager.flush() after users.save) inside the same method (the place where users.save(user) is called in LoginService/ UserService), so the unique constraint violation is raised immediately and caught by the existing recovery logic.backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt (1)
139-139:⚠️ Potential issue | 🟠 MajorRedact the token in the registration log too.
The send path now redacts tokens, but Line 139 still prints the raw FCM token. That is still a stable device identifier in logs.
Minimal redaction
- log.debug("Inserting messaging token for user: {}, token: {}", userId, token) + log.debug("Inserting messaging token for user: {}, token: …{}", userId, token.takeLast(6))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt` at line 139, The debug log in PushNotificationService that currently logs the raw FCM token ("Inserting messaging token for user: {}, token: {}" using userId and token) must redact the token before logging; update the insertion path to compute a masked token (reuse the existing redaction helper used in the send path or add a small maskToken/redactToken function) and log that maskedToken instead of token to avoid writing stable device identifiers to logs.
🧹 Nitpick comments (3)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt (1)
133-152: Consider using a repository query instead of in-memory filtering.
locationRepository.findAll()followed by in-memory filtering loads all locations before applying the filter conditions. For large datasets, this could impact performance.Consider adding a dedicated repository query that filters by timestamp at the database level, then apply the remaining
groupName/broadcastfilter in memory (or combine into a custom@Query).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt` around lines 133 - 152, The current use of locationRepository.findAll() in LocationService followed by in-memory filters (timestamp vs currentTime and group/broadcast) causes full table loads; change this to use a repository query that filters by timestamp at the DB level (e.g., add a method in the repository like findByTimestampGreaterThan(timestamp) or a custom `@Query` that accepts the cutoff and optionally group/broadcast), then in LocationService call that repository method instead of findAll() and keep only the remaining groupName==group || broadcast in-memory before mapping to MapMarker via mapDisplayName; ensure you reference the existing visibilityDuration and currentTime when computing the timestamp cutoff.backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationRepository.kt (1)
18-24: Remove unused repository methods.
findAllByGroupNameOrderByGroupNameAscanddeleteByUserIdare defined but unused. Additionally,findAllByGroupNameOrderByGroupNameAschas a redundant design—ordering results bygroupNamewhen filtering by a specificgroupNamevalue produces no meaningful ordering benefit since all results share the same group name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationRepository.kt` around lines 18 - 24, Remove the unused repository methods by deleting the declarations fun findAllByGroupNameOrderByGroupNameAsc(groupName: String): List<LocationEntity> and fun deleteByUserId(userId: Int) from the LocationRepository interface; also search for and remove or update any callers that reference these methods (or replace them with the appropriate existing methods like findAllByTimestampGreaterThan / findAllByTimestampLessThan) and run a build to ensure no references remain.backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt (1)
83-100: Let coroutine cancellation propagate.
runCatchinghere also trapsCancellationException, so a cancelled batch gets downgraded to a per-token warning and keeps running. Re-throw cancellation before classifying FCM failures.Minimal fix
@@ -import kotlinx.coroutines.async +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.async @@ - }.getOrElse { e -> + }.getOrElse { e -> + if (e is CancellationException) throw e val invalidToken = e is WebClientResponseException.NotFound || (e is WebClientResponseException.BadRequest && e.responseBodyAsString.contains("UNREGISTERED", ignoreCase = true))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt` around lines 83 - 100, The runCatching block in PushNotificationService.kt (inside the send loop that returns SendResult) is swallowing CancellationException; update the getOrElse/error handling to immediately rethrow CancellationException (and optionally InterruptedException) before classifying FCM failures so coroutine cancellation propagates. Concretely, inside the lambda handling the exception from runCatching, check if the caught exception is a CancellationException and if so rethrow it; otherwise continue the existing logic that detects invalidToken (WebClientResponseException.NotFound or BadRequest containing "UNREGISTERED"), logs with the redacted token, and returns SendResult(false, token, invalidToken).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt`:
- Around line 70-79: The broadcast permission check causes a redundant DB
lookup: LocationService loads userEntity earlier (userEntity) but
shareLocationAllowed(token) calls userRepository.findByCmschId again; change the
call to reuse the already-loaded userEntity instead of token lookup — either
refactor shareLocationAllowed to accept a UserEntity (e.g.,
shareLocationAllowed(userEntity): Boolean) and use userEntity.hasPermission(...)
or add an overload shareLocationAllowed(token: String) and
shareLocationAllowed(user: UserEntity) so the LocationService call that sets
entity.broadcast uses the UserEntity variant; ensure other callers still work if
the method is used elsewhere.
In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt`:
- Around line 44-47: The helper saveUserWithConflictRecovery currently calls
users.save(user) but does not capture and return the saved entity, causing a
type mismatch; change the implementation to assign the result of
users.save(user) to a variable (e.g., canonical = users.save(user)) and return
that canonical value; do the same for the recovery path inside the catch (assign
the result of users.save(canonical) and return it). Apply the same fix to the
other similar save-with-recovery call in this file so each try/catch returns the
saved UserEntity explicitly.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt`:
- Around line 146-152: The catch for DataIntegrityViolationException in register
token handling is swallowing all integrity errors; after re-reading with
messagingTokenRepository.findByUserIdAndToken(userId, token) and updating
existingAfterConflict when present, add an else branch to re-throw the original
exception (e) so non-(userId,token) constraint violations are not silently
ignored; locate the catch block handling DataIntegrityViolationException around
the messagingTokenRepository usage (existingAfterConflict, updatedAt,
messagingTokenRepository.save) and append `throw e` in the else path.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`:
- Around line 29-34: The issue is that listRiddlesForUser() and
listRiddlesForGroup() currently fetch only completed mappings (using
riddleMappingRepository.findAllByOwnerUserIdAndCompletedTrue /
findAllByGroupIdAndCompletedTrue) which drops in-progress mapping state needed
by mapRiddleView (e.g., hintUsed); change those repository calls to fetch
mappings regardless of completed status (e.g., remove the CompletedTrue filter
or call the repository method that returns all mappings for the user/group),
keep the subsequent grouping/association logic the same, and ensure both user
and group code paths (and the other occurrences noted around the other ranges)
use the non-completed-only query so mapRiddleView sees hintUsed and other
in-progress fields.
---
Outside diff comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`:
- Around line 125-155: mapRiddleView calls cannotSkip which issues a COUNT query
per riddle (via countAllByCompletedTrueAndSkippedFalseAndRiddleId), causing N
extra DB calls when findNextTo maps candidates; instead, run a single grouped
repository query to compute the set of nonSkippableRiddleIds for the candidate
set and change mapRiddleView to accept that Set<Long> (or add an overload) so it
checks membership (nonSkippableRiddleIds.contains(riddle.id)) rather than
calling cannotSkip; apply the same batching refactor where cannotSkip/COUNT is
used elsewhere (references: mapRiddleView, cannotSkip, findNextTo,
countAllByCompletedTrueAndSkippedFalseAndRiddleId, nonSkippableRiddleIds).
---
Duplicate comments:
In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt`:
- Around line 450-452: The Keycloak first-login path calls
updateFieldsForKeycloak(profile, user) then saveUserWithConflictRecovery(user)
outside a write transaction, making the re-read/merge/save fallback non-atomic;
wrap the entire Keycloak flow (the call to updateFieldsForKeycloak, the
subsequent saveUserWithConflictRecovery(user), and the
adminMenuService.invalidateUser(savedUser.internalId) follow-up) inside the same
write transaction used elsewhere so the initial save and any conflict-recovery
retry happen atomically — locate the Keycloak flow that invokes
updateFieldsForKeycloak and ensure it executes within the repository/DB write
transaction block (the same transaction pattern used around the other
saveUserWithConflictRecovery calls).
- Around line 45-46: The catch for unique-key recovery will never trigger if
users.save(user) doesn't flush to the DB, so change the helper to force a flush
after persisting: replace users.save(user) with a flush-capable call (e.g.
users.saveAndFlush(user) or call entityManager.flush() after users.save) inside
the same method (the place where users.save(user) is called in LoginService/
UserService), so the unique constraint violation is raised immediately and
caught by the existing recovery logic.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt`:
- Line 139: The debug log in PushNotificationService that currently logs the raw
FCM token ("Inserting messaging token for user: {}, token: {}" using userId and
token) must redact the token before logging; update the insertion path to
compute a masked token (reuse the existing redaction helper used in the send
path or add a small maskToken/redactToken function) and log that maskedToken
instead of token to avoid writing stable device identifiers to logs.
---
Nitpick comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationRepository.kt`:
- Around line 18-24: Remove the unused repository methods by deleting the
declarations fun findAllByGroupNameOrderByGroupNameAsc(groupName: String):
List<LocationEntity> and fun deleteByUserId(userId: Int) from the
LocationRepository interface; also search for and remove or update any callers
that reference these methods (or replace them with the appropriate existing
methods like findAllByTimestampGreaterThan / findAllByTimestampLessThan) and run
a build to ensure no references remain.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt`:
- Around line 133-152: The current use of locationRepository.findAll() in
LocationService followed by in-memory filters (timestamp vs currentTime and
group/broadcast) causes full table loads; change this to use a repository query
that filters by timestamp at the DB level (e.g., add a method in the repository
like findByTimestampGreaterThan(timestamp) or a custom `@Query` that accepts the
cutoff and optionally group/broadcast), then in LocationService call that
repository method instead of findAll() and keep only the remaining
groupName==group || broadcast in-memory before mapping to MapMarker via
mapDisplayName; ensure you reference the existing visibilityDuration and
currentTime when computing the timestamp cutoff.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt`:
- Around line 83-100: The runCatching block in PushNotificationService.kt
(inside the send loop that returns SendResult) is swallowing
CancellationException; update the getOrElse/error handling to immediately
rethrow CancellationException (and optionally InterruptedException) before
classifying FCM failures so coroutine cancellation propagates. Concretely,
inside the lambda handling the exception from runCatching, check if the caught
exception is a CancellationException and if so rethrow it; otherwise continue
the existing logic that detects invalidToken
(WebClientResponseException.NotFound or BadRequest containing "UNREGISTERED"),
logs with the redacted token, and returns SendResult(false, token,
invalidToken).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a351587-c9bf-4fc2-8fda-103aa01419cc
📒 Files selected for processing (8)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationEntity.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationRepository.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/MessagingTokenEntity.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/team/TeamService.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationEntity.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/MessagingTokenEntity.kt
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/team/TeamService.kt
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt
Outdated
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt
Outdated
Show resolved
Hide resolved
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt (1)
42-72:⚠️ Potential issue | 🔴 CriticalThe manual upsert still races on concurrent first writes.
findByUserId()/findByToken()andsave()are separate DB round-trips. Two simultaneous first uploads for the same staff member can still both take the insert path, and one will fail on the unique keys. This needs a real DB upsert, or a catch/reload/retry path aroundlocationRepository.save(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt` around lines 42 - 72, The current manual upsert using findByUserId()/findByToken() then locationRepository.save(entity) can race on concurrent first writes; change to a real upsert or add a retry-on-constraint-violation: wrap the save in a try/catch for the DB unique constraint/DataIntegrityViolation exception around locationRepository.save(entity), and in the catch reload the existing entity via findByUserId()/findByToken() (or findByToken first), merge the incoming fields (longitude, latitude, altitude, accuracy, speed, altitudeAccuracy, heading, timestamp, broadcast using shareLocationAllowed(userEntity)), then save again; alternatively replace the logic with a repository-level upsert/merge query if supported to atomically insert-or-update LocationEntity.backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt (2)
60-61:⚠️ Potential issue | 🟠 MajorRepeated full-table scans remain in request paths.
These blocks still do
findAll()and filter in memory on each request. After cache removal, this keeps read complexity tied to total riddle/category table size and will degrade under load.💡 Direction for fix
- val allRiddles = riddleEntityRepository.findAll().toList() - val riddleIdsInCategory = allRiddles.filter { it.categoryId == riddle.categoryId }.map { it.id } + val riddleIdsInCategory = riddleEntityRepository.findAllIdsByCategoryId(riddle.categoryId) - val allCategories = riddleCategoryRepository.findAll().toList() - val categoryIds = allCategories.filter { it.visible && it.minRole.value <= user.role.value }.map { it.categoryId } + val categoryIds = riddleCategoryRepository.findVisibleCategoryIdsByMinRole(user.role.value)Use repository-level predicates/ID projections so filtering happens in SQL.
Also applies to: 86-91, 106-111, 441-447, 451-457, 487-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt` around lines 60 - 61, The code is performing full-table reads via riddleEntityRepository.findAll() and then filtering in memory (variables allRiddles and riddlesByCategory); replace these with repository-level queries that push filtering to the database (e.g., add/find methods like findByCategoryId(categoryId), findAllByIdIn(idList), or custom queries returning only ID projections) and use those in the methods that currently call findAll() (the blocks around the variables allRiddles/riddlesByCategory and the similar blocks at the other noted locations). Update service calls to accept categoryId or id lists and consume the repository-returned results directly so SQL does the filtering and reduces read complexity.
66-73:⚠️ Potential issue | 🟠 MajorCompleted count is inflated by in-progress mappings.
submissionsnow includes unfinished rows (needed forhintUsed), but Line 72 counts all rows, not only solved ones. Category progress becomes incorrect.💡 Suggested fix
- completed = submissions[category.categoryId]?.size ?: 0, + completed = submissions[category.categoryId]?.count { it.completed } ?: 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt` around lines 66 - 73, The completed count is currently using submissions[category.categoryId]?.size which includes in-progress rows (kept for hintUsed); change the calculation for RiddleCategoryDto.completed to count only finished/solved submissions for that category (e.g., replace the size use with submissions[category.categoryId]?.count { it.solved || it.correct || it.isFinished } ?: 0) so only solved entries are tallied while leaving the submissions map intact for hintUsed handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt`:
- Around line 81-83: The helper shareLocationAllowed references UserEntity which
is not imported; either add the missing import for the entity type returned by
UserRepository (UserEntity) at the top of LocationService.kt or remove this
helper and inline the permission check where it's used (replace calls to
shareLocationAllowed(user) with
user.hasPermission(StaffPermissions.PERMISSION_BROADCAST_LOCATION.permissionString));
ensure all usages and signatures are updated accordingly so the compiler no
longer complains about an unknown UserEntity type.
- Around line 49-73: When updating an existing LocationEntity you must refresh
its persisted user metadata before saving to avoid stale token/groupName etc.;
set entity.token = locationDto.token (or userEntity-derived token rotation),
entity.userId = userEntity.id, entity.userName = userEntity.fullName,
entity.alias = userEntity.alias, entity.groupName = userEntity.groupName and
entity.markerColor = resolveColor(userEntity.groupName) (and recalc
entity.broadcast as you already do with shareLocationAllowed(userEntity)) prior
to calling locationRepository.save(entity) so LocationResponse(if
(entity.broadcast) ...) returns up-to-date values.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`:
- Around line 216-217: The `@Retryable` annotations on RiddleBusinessLogicService
(and other services) are currently inert because the Resilience4j Spring support
isn't enabled and AOP is missing; add `@EnableResilientMethods` to the main
application class (CMSchApplication.kt) and add the spring-boot-starter-aop
dependency in build.gradle.kts so the `@Retryable` proxying works, then update the
`@Retryable` declarations (e.g., on methods in RiddleBusinessLogicService
annotated with `@Retryable`(value = [SQLException::class], ...)) to catch Spring
data-access exceptions instead of raw SQLException — either change the value to
include org.springframework.dao.DataAccessException (or specific subclasses like
SerializationFailureException/CannotAcquireLockException) or supply a custom
retry predicate that treats transaction serialization/lock failures as
retryable.
---
Duplicate comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt`:
- Around line 42-72: The current manual upsert using
findByUserId()/findByToken() then locationRepository.save(entity) can race on
concurrent first writes; change to a real upsert or add a
retry-on-constraint-violation: wrap the save in a try/catch for the DB unique
constraint/DataIntegrityViolation exception around
locationRepository.save(entity), and in the catch reload the existing entity via
findByUserId()/findByToken() (or findByToken first), merge the incoming fields
(longitude, latitude, altitude, accuracy, speed, altitudeAccuracy, heading,
timestamp, broadcast using shareLocationAllowed(userEntity)), then save again;
alternatively replace the logic with a repository-level upsert/merge query if
supported to atomically insert-or-update LocationEntity.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`:
- Around line 60-61: The code is performing full-table reads via
riddleEntityRepository.findAll() and then filtering in memory (variables
allRiddles and riddlesByCategory); replace these with repository-level queries
that push filtering to the database (e.g., add/find methods like
findByCategoryId(categoryId), findAllByIdIn(idList), or custom queries returning
only ID projections) and use those in the methods that currently call findAll()
(the blocks around the variables allRiddles/riddlesByCategory and the similar
blocks at the other noted locations). Update service calls to accept categoryId
or id lists and consume the repository-returned results directly so SQL does the
filtering and reduces read complexity.
- Around line 66-73: The completed count is currently using
submissions[category.categoryId]?.size which includes in-progress rows (kept for
hintUsed); change the calculation for RiddleCategoryDto.completed to count only
finished/solved submissions for that category (e.g., replace the size use with
submissions[category.categoryId]?.count { it.solved || it.correct ||
it.isFinished } ?: 0) so only solved entries are tallied while leaving the
submissions map intact for hintUsed handling.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d106d0d-a31e-4073-a4b1-8c3f8049bdad
📒 Files selected for processing (4)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt
✅ Files skipped from review due to trivial changes (1)
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/pushnotification/PushNotificationService.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/login/LoginService.kt
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt
Show resolved
Hide resolved
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt
Show resolved
Hide resolved
Comment on lines
+216
to
+217
| @Retryable(value = [SQLException::class], maxRetries = 5, delay = 500L, multiplier = 1.5) | ||
| @Transactional(readOnly = false, isolation = Isolation.SERIALIZABLE, propagation = Propagation.REQUIRES_NEW) | ||
| @Transactional(readOnly = false, isolation = Isolation.SERIALIZABLE) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Retryable imports used in Kotlin sources"
rg -n 'import org\.springframework\..*Retryable' --type=kt
echo
echo "2) Methods combining Retryable(SQLException) + SERIALIZABLE"
rg -nPU '@Retryable\(value\s*=\s*\[SQLException::class\][\s\S]{0,200}?@Transactional\([^)]*Isolation\.SERIALIZABLE' backend/src/main/kotlin
echo
echo "3) Retry-related dependencies"
fd -i 'build.gradle*' | xargs -r rg -n 'spring-retry|resilience|retry'
fd -i 'pom.xml' | xargs -r rg -n 'spring-retry|resilience|retry'
echo
echo "4) Existing handling of lock/serialization exceptions"
rg -n 'CannotAcquireLockException|DeadlockLoserDataAccessException|PessimisticLockingFailureException|SerializationFailureException|TransientDataAccessException' backend/src/main/kotlinRepository: kir-dev/cmsch
Length of output: 144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Checking RiddleBusinessLogicService.kt for `@Retryable` annotations ==="
if [ -f "backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt" ]; then
echo "File found. Checking lines 210-225 and 290-305:"
sed -n '210,225p' "backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt" | cat -n
echo ""
sed -n '290,305p' "backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt" | cat -n
else
echo "File not found"
fi
echo
echo "=== 2) Find all `@Retryable` + `@Transactional` combinations in Kotlin ==="
find backend/src/main/kotlin -name "*.kt" -type f | head -20 | xargs -I {} sh -c 'grep -l "@Retryable" "{}" && echo "File: {}"' 2>/dev/null || echo "No matches found"
echo
echo "=== 3) Check spring-retry dependency ==="
find . -name "build.gradle*" -o -name "pom.xml" | head -5 | xargs -r grep -l "spring-retry" 2>/dev/null || echo "No spring-retry found"
echo
echo "=== 4) Search for exception handling patterns ==="
find backend/src/main/kotlin -name "*.kt" -type f | xargs grep -l "DataAccessException\|SerializationFailureException\|DeadlockLoserDataAccessException" 2>/dev/null | head -10Repository: kir-dev/cmsch
Length of output: 2016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) All build.gradle files ==="
find . -name "build.gradle*" -type f | head -10
echo
echo "=== 2) Contents of main build files ==="
for f in build.gradle build.gradle.kts; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat "$f" | head -100
fi
done
echo
echo "=== 3) Search for spring-retry in all gradle/maven files ==="
find . \( -name "*.gradle" -o -name "*.gradle.kts" -o -name "pom.xml" \) -type f -exec grep -l "spring-retry\|springframework.*retry" {} \;
echo
echo "=== 4) Check for `@EnableRetry` annotation ==="
find backend/src -name "*.kt" -type f | xargs grep -n "@EnableRetry" 2>/dev/null || echo "No `@EnableRetry` found"
echo
echo "=== 5) Import statements in RiddleBusinessLogicService.kt ==="
head -50 "backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt" | grep -E "^import"Repository: kir-dev/cmsch
Length of output: 821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Content of backend/build.gradle.kts ==="
cat backend/build.gradle.kts | head -200
echo
echo "=== 2) Check for resilience4j dependencies ==="
cat backend/build.gradle.kts | grep -i "resilience"
echo
echo "=== 3) Other `@Retryable` usages in the codebase ==="
find backend/src -name "*.kt" -type f | xargs grep -n "@Retryable" 2>/dev/null | head -20
echo
echo "=== 4) Check NovaIntegrationService.kt mentioned in earlier search ==="
grep -A5 -B5 "@Retryable" backend/src/main/kotlin/hu/bme/sch/cmsch/addon/nova/NovaIntegrationService.kt 2>/dev/null | head -30Repository: kir-dev/cmsch
Length of output: 5796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Check for application configuration files ==="
find backend/src -name "application*.yml" -o -name "application*.properties" | head -10
echo
echo "=== 2) Search for resilience config ==="
find backend -name "*resilience*" -o -name "*retry*" 2>/dev/null | head -10
echo
echo "=== 3) Look for `@EnableRetry` or any retry-related annotations ==="
find backend/src -name "*.kt" -type f | xargs grep -n "EnableRetry\|@Bean.*Retry\|retryTemplate\|retryPolicy" 2>/dev/null | head -20
echo
echo "=== 4) Check Spring Boot configuration classes ==="
find backend/src -name "*.kt" -type f | xargs grep -l "@Configuration\|@SpringBootApplication" 2>/dev/null | head -10
echo
echo "=== 5) Look at what exceptions can be thrown in submitRiddleForUser ==="
grep -A50 "override fun submitRiddleForUser" backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt | head -60Repository: kir-dev/cmsch
Length of output: 4843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Check CMSchApplication.kt for any retry/resilience configuration ==="
cat backend/src/main/kotlin/hu/bme/sch/cmsch/CMSchApplication.kt
echo
echo "=== 2) Search for Resilience configuration beans ==="
find backend/src -name "*.kt" -type f | xargs grep -l "Resilience\|resilience" 2>/dev/null | head -10
echo
echo "=== 3) Check the complete imports from RiddleBusinessLogicService.kt ==="
head -100 backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt | grep "^import"
echo
echo "=== 4) Look at submitRiddleForGroup method as well ==="
sed -n '296,340p' backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.ktRepository: kir-dev/cmsch
Length of output: 4389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Check all application.properties files for resilience config ==="
cat backend/src/main/resources/config/application.properties
echo "---"
cat backend/src/main/resources/config/application-prod.properties
echo
echo "=== 2) Look for any Resilience4j or retry-related properties ==="
find backend/src/main/resources -name "*.properties" -o -name "*.yml" | xargs grep -h "resilience\|retry" 2>/dev/null | head -20
echo
echo "=== 3) Check other services using `@Retryable` from resilience ==="
find backend/src -name "*.kt" -type f | xargs grep -B2 -A10 "@Retryable" 2>/dev/null | head -80
echo
echo "=== 4) Check if there are exception handlers or recovery methods ==="
find backend/src -name "*.kt" -type f | xargs grep -n "Recover\|@Recover" 2>/dev/nullRepository: kir-dev/cmsch
Length of output: 8408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Find all `@Retryable` usages ==="
find backend/src -name "*.kt" -type f -exec grep -l "@Retryable" {} \;
echo
echo "=== 2) Show context around `@Retryable` in each file ==="
find backend/src -name "*.kt" -type f -exec grep -B3 -A15 "@Retryable" {} + | head -150
echo
echo "=== 3) Check import statements for exception handling ==="
grep -r "DataAccessException\|SQLException" backend/src/main/kotlin --include="*.kt" | head -20Repository: kir-dev/cmsch
Length of output: 18730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Check Spring Boot version more thoroughly ==="
cat backend/build.gradle.kts | grep -i "spring.boot\|kotlin"
echo
echo "=== 2) Look for documentation or comments about the Retryable annotation ==="
find backend/src -name "*.kt" | xargs grep -B5 "@Retryable" | grep -E "//|/\*" | head -20
echo
echo "=== 3) Check for any exception handling or recovery logic ==="
find backend/src -name "*.kt" | xargs grep -A20 "@Retryable" | grep -E "catch|try|Exception|Recover" | head -30
echo
echo "=== 4) Verify the exact behavior by checking Spring documentation artifacts ==="
# Check if there's a Spring Boot BOM or version file that shows what version of what libraries
cat backend/build.gradle.kts | grep -i "platform\|bom" | head -10Repository: kir-dev/cmsch
Length of output: 2136
🌐 Web query:
Spring Boot 4 org.springframework.resilience.annotation.Retryable exception handling
💡 Result:
In Spring Boot 4 (Spring Framework 7), org.springframework.resilience.annotation.Retryable retries a method invocation when it throws an exception, and if retries are exhausted the last exception is propagated to the caller (so you handle it with normal try/catch at the call site, an @ControllerAdvice, etc.). By default it retries any exception, with maxRetries = 3 and delay = 1s. [1]
Controlling which exceptions trigger a retry
Use includes / excludes (or the value alias for includes) to decide what is retried:
includes: exception types that should be retriedexcludes: exception types that must not be retried- Matching is done against the thrown exception and nested causes. [1][2]
Example:
`@EnableResilientMethods`
`@Configuration`
class ResilienceConfig {}
`@Service`
class Client {
`@Retryable`(
includes = { java.io.IOException.class },
excludes = { IllegalArgumentException.class },
maxRetries = 4,
delay = 200
)
void callRemote() { ... }
}Custom exception logic (beyond type matching)
For advanced filtering (e.g., retry only when message contains something, or when a specific HTTP status is present in the cause chain), provide a custom MethodRetryPredicate via predicate. [1][2]
Common “why isn’t it retrying?” gotchas
@Retryableapplies to proxy-invoked methods; self-invocation won’t trigger retries (same limitation as other Spring AOP-style annotations). [1]- You must enable processing via
@EnableResilientMethods(or register the corresponding bean post-processor). [1]
Sources: Spring Framework “Resilience Features” reference docs [1], @Retryable javadoc (Spring Framework 7.x) [2].
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Search for `@EnableResilientMethods` ==="
find backend/src -name "*.kt" -type f | xargs grep -r "EnableResilientMethods" 2>/dev/null || echo "NOT FOUND"
echo
echo "=== 2) Check if there's autoconfiguration or alternative setup ==="
find backend/src -name "*.kt" | xargs grep -i "resilience\|RetryableAspect\|EnableAspectJAutoProxy" 2>/dev/null | head -20
echo
echo "=== 3) Check Spring Boot auto-configuration presence ==="
# In Spring Boot 4, there might be auto-configuration for resilience
grep -r "org.springframework.boot.autoconfigure" backend/src/main/resources 2>/dev/null || echo "No explicit autoconfiguration file found"
echo
echo "=== 4) Double-check the Spring Boot version and its dependencies ==="
head -20 backend/build.gradle.kts | grep -E "SpringBootPlugin|4.0|version"Repository: kir-dev/cmsch
Length of output: 2996
🌐 Web query:
Spring Boot 4.0.4 @EnableResilientMethods auto-configuration org.springframework.resilience
💡 Result:
In Spring Boot 4.0.4, @EnableResilientMethods is not a Spring Boot auto-configuration (there’s no Boot @AutoConfiguration in an org.springframework.resilience package that turns it on automatically).
Instead, it’s a Spring Framework 7 annotation in:
org.springframework.resilience.annotation.EnableResilientMethods[1]
What it does:
- Enables Spring’s built-in resilience annotations
@Retryableand@ConcurrencyLimitby importingResilientMethodsConfiguration[1]. - Alternative to the meta-annotation: explicitly register
RetryAnnotationBeanPostProcessor/ConcurrencyLimitBeanPostProcessorbeans [2].
What you need in a Boot app:
- Add
@EnableResilientMethodson one of your@Configurationclasses (often your@SpringBootApplication) [2]. - Ensure you have AOP enabled on the classpath (typically
spring-boot-starter-aop), since these features are applied via proxying/post-processors (implied by the import + BPP model) [1].
Sources
[1] Spring Framework 7.0.6 Javadoc — EnableResilientMethods (package org.springframework.resilience.annotation)
[2] Spring Framework Reference — “Resilience Features” / “Enabling Resilient Methods”
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Check for spring-boot-starter-aop dependency ==="
grep "spring-boot-starter-aop\|spring-aop" backend/build.gradle.kts || echo "AOP DEPENDENCY NOT FOUND"
echo
echo "=== 2) Search all configuration classes for EnableResilientMethods ==="
find backend/src -name "*Config*.kt" -o -name "*Configuration*.kt" | xargs grep -l "EnableResilientMethods\|@Configuration" 2>/dev/null | head -20
echo
echo "=== 3) Check CMSchApplication one more time ==="
cat backend/src/main/kotlin/hu/bme/sch/cmsch/CMSchApplication.kt
echo
echo "=== 4) Look for any component scan or autoconfiguration setup ==="
find backend/src -name "*.kt" | xargs grep "@ComponentScan\|@EnableAutoConfiguration\|@Import.*Resilience" 2>/dev/null || echo "None found"Repository: kir-dev/cmsch
Length of output: 3064
Critical: @Retryable annotations are non-functional without @EnableResilientMethods configuration.
All 16 services using @org.springframework.resilience.annotation.Retryable (including lines 216-217 and 296-297) will not actually retry because @EnableResilientMethods is missing from the application configuration. Additionally, spring-boot-starter-aop is not in the dependencies.
Add @EnableResilientMethods to CMSchApplication.kt and include spring-boot-starter-aop in build.gradle.kts.
After enabling retries, address the secondary issue: @Retryable(value = [SQLException::class]) combined with Isolation.SERIALIZABLE will miss transaction conflict exceptions. With SERIALIZABLE isolation, lock/serialization failures are wrapped as Spring DataAccessException subtypes (e.g., SerializationFailureException, CannotAcquireLockException), not raw SQLException. Broaden the retry exception types to catch Spring's data access exceptions or use a custom retry predicate that handles transaction failures.
Also applies to: 296-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt`
around lines 216 - 217, The `@Retryable` annotations on RiddleBusinessLogicService
(and other services) are currently inert because the Resilience4j Spring support
isn't enabled and AOP is missing; add `@EnableResilientMethods` to the main
application class (CMSchApplication.kt) and add the spring-boot-starter-aop
dependency in build.gradle.kts so the `@Retryable` proxying works, then update the
`@Retryable` declarations (e.g., on methods in RiddleBusinessLogicService
annotated with `@Retryable`(value = [SQLException::class], ...)) to catch Spring
data-access exceptions instead of raw SQLException — either change the value to
include org.springframework.dao.DataAccessException (or specific subclasses like
SerializationFailureException/CannotAcquireLockException) or supply a custom
retry predicate that treats transaction serialization/lock failures as
retryable.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt (1)
43-49:⚠️ Potential issue | 🟠 MajorNon-atomic upsert can still fail with unique-key races.
Line 43–49 does a read-then-create path, and Line 81 saves later. Concurrent first writes for the same user/token can still pass the reads and collide on save.
🛠️ Suggested hardening (retry-on-conflict)
+import org.springframework.dao.DataIntegrityViolationException +import org.springframework.transaction.annotation.Transactional ... + `@Transactional` fun pushLocation(locationDto: LocationDto): LocationResponse { ... - locationRepository.save(entity) + try { + locationRepository.save(entity) + } catch (_: DataIntegrityViolationException) { + val resolved = locationRepository.findByUserId(userEntity.id) + .orElseGet { locationRepository.findByToken(locationDto.token).orElseThrow() } + resolved.token = locationDto.token + resolved.userId = userEntity.id + resolved.userName = userEntity.fullName + resolved.alias = userEntity.alias + resolved.groupName = userEntity.groupName + resolved.markerColor = resolveColor(userEntity.groupName) + resolved.longitude = locationDto.longitude + resolved.latitude = locationDto.latitude + resolved.altitude = locationDto.altitude + resolved.accuracy = locationDto.accuracy + resolved.speed = locationDto.speed + resolved.altitudeAccuracy = locationDto.altitudeAccuracy + resolved.heading = locationDto.heading + resolved.timestamp = clock.getTimeInSeconds() + resolved.broadcast = locationDto.broadcastEnabled && shareLocationAllowed(userEntity) + locationRepository.save(resolved) + }Also applies to: 81-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt` around lines 43 - 49, The current read-then-create flow in LocationService (using locationRepository.findByUserId, findByToken and later saving the entity) is vulnerable to unique-key races; change the upsert to be conflict-tolerant by catching the DB uniqueness/DataIntegrityViolation exception around the save (the code path that uses the local variable entity and the later save call), then re-query the repository (findByUserId/findByToken) and retry merge/save a bounded number of times (e.g., 3 retries) before failing; alternatively implement the upsert as a single atomic DB operation if supported by the repository, but ensure the save branch handles concurrent insert conflicts and merges the existing record instead of always failing.
🧹 Nitpick comments (3)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt (3)
85-92: Remove or repurpose the unused token-based permission helper.After Line 79 switched to
shareLocationAllowed(userEntity), the token overload is dead in this file and adds duplicate permission paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt` around lines 85 - 92, The token-based overload shareLocationAllowed(token: String) is now unused and duplicates the permission check implemented by shareLocationAllowed(user: UserEntity); remove the dead method (shareLocationAllowed(token: String)) and any direct references to startupPropertyConfig.profileQrPrefix and userRepository usage in it, or if you need token-to-user resolution elsewhere, move that logic into a clearly named helper (e.g., resolveUserFromToken) and have callers use shareLocationAllowed(user: UserEntity) to check StaffPermissions.PERMISSION_BROADCAST_LOCATION.permissionString; ensure no remaining callers reference the removed overload.
136-138: Method naming is inconsistent with the predicate.
findLocationsOfGroup(groupId: Int)filters byuserIdon Line 138. Either rename to user semantics or adjust logic to true group semantics to prevent misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt` around lines 136 - 138, The method findLocationsOfGroup(groupId: Int) currently filters by userId (locationRepository.findAll().filter { it.userId == groupId }), causing a naming/behavior mismatch; either rename the function to findLocationsOfUser and update callers to reflect user semantics, or change the predicate to use the group relation (e.g., compare against the entity's groupId or navigate its group property) so the method truly returns locations for a group—update the function signature and all callers of findLocationsOfGroup accordingly and ensure the entity field referenced (userId vs groupId or group) matches the chosen semantics.
146-149: Avoid full-table scan for active marker queries.Line 146 currently loads all rows and filters in memory. You already have a timestamp-range repository method; using it first will reduce read volume.
⚡ Suggested query narrowing
- locations.addAll(locationRepository.findAll() + val cutoff = currentTime - visibilityDuration + locations.addAll(locationRepository.findAllByTimestampGreaterThan(cutoff) .filter { it.groupName == group || it.broadcast } - .filter { it.timestamp + visibilityDuration > currentTime } .map { MapMarker(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt` around lines 146 - 149, Replace the full-table in-memory filter starting at locationRepository.findAll() with the repository's timestamp-range query (e.g. the existing method that returns rows where timestamp is within the visibility window) so you first restrict by timestamp (use visibilityDuration and currentTime to compute the lower bound) and then apply the remaining filters for groupName == group or broadcast and the map transformation; i.e., call the timestamp-range repository method instead of locationRepository.findAll(), then .filter { it.groupName == group || it.broadcast } and .map { ... } in LocationService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt`:
- Around line 43-49: The current read-then-create flow in LocationService (using
locationRepository.findByUserId, findByToken and later saving the entity) is
vulnerable to unique-key races; change the upsert to be conflict-tolerant by
catching the DB uniqueness/DataIntegrityViolation exception around the save (the
code path that uses the local variable entity and the later save call), then
re-query the repository (findByUserId/findByToken) and retry merge/save a
bounded number of times (e.g., 3 retries) before failing; alternatively
implement the upsert as a single atomic DB operation if supported by the
repository, but ensure the save branch handles concurrent insert conflicts and
merges the existing record instead of always failing.
---
Nitpick comments:
In
`@backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt`:
- Around line 85-92: The token-based overload shareLocationAllowed(token:
String) is now unused and duplicates the permission check implemented by
shareLocationAllowed(user: UserEntity); remove the dead method
(shareLocationAllowed(token: String)) and any direct references to
startupPropertyConfig.profileQrPrefix and userRepository usage in it, or if you
need token-to-user resolution elsewhere, move that logic into a clearly named
helper (e.g., resolveUserFromToken) and have callers use
shareLocationAllowed(user: UserEntity) to check
StaffPermissions.PERMISSION_BROADCAST_LOCATION.permissionString; ensure no
remaining callers reference the removed overload.
- Around line 136-138: The method findLocationsOfGroup(groupId: Int) currently
filters by userId (locationRepository.findAll().filter { it.userId == groupId
}), causing a naming/behavior mismatch; either rename the function to
findLocationsOfUser and update callers to reflect user semantics, or change the
predicate to use the group relation (e.g., compare against the entity's groupId
or navigate its group property) so the method truly returns locations for a
group—update the function signature and all callers of findLocationsOfGroup
accordingly and ensure the entity field referenced (userId vs groupId or group)
matches the chosen semantics.
- Around line 146-149: Replace the full-table in-memory filter starting at
locationRepository.findAll() with the repository's timestamp-range query (e.g.
the existing method that returns rows where timestamp is within the visibility
window) so you first restrict by timestamp (use visibilityDuration and
currentTime to compute the lower bound) and then apply the remaining filters for
groupName == group or broadcast and the map transformation; i.e., call the
timestamp-range repository method instead of locationRepository.findAll(), then
.filter { it.groupName == group || it.broadcast } and .map { ... } in
LocationService.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef867a8b-21e4-4cab-9156-b49da4343e55
📒 Files selected for processing (4)
backend/build.gradle.ktsbackend/src/main/kotlin/hu/bme/sch/cmsch/CMSchApplication.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.ktbackend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/kotlin/hu/bme/sch/cmsch/component/riddle/RiddleBusinessLogicService.kt
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
1 participant
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
New Features
Bug Fixes
Chores