[Phase 5] feat(safety): 매물 안전 점수 계산 배치 구현 - #77
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a scheduled batch that recalculates property safety scores from nearby safety facilities, upserts the results into ChangesProperty Safety Score Calculation Batch
Sequence Diagram(s)sequenceDiagram
participant Cron as Cron trigger
participant Scheduler as PropertySafetyScoreScheduler
participant Service as PropertySafetyScoreServiceImpl
participant PropertyDao as PropertyDao
participant FacilityDao as SafetyFacilityDao
Cron->>Scheduler: runMonthlyRecalculation()
Scheduler->>Service: recalculateAll()
Service->>PropertyDao: findActivePropertiesForSafetyScoring()
PropertyDao-->>Service: active properties
Service->>FacilityDao: findInBounds(...)
FacilityDao-->>Service: nearby facilities
Service->>Service: count by radius + calculateScore()
Service->>PropertyDao: upsertSafetyScoreStats(results)
Scheduler->>Scheduler: log result count
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
backend/src/main/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDao.java (1)
385-393: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrap the bulk upsert in a single transaction.
upsertSafetyScoreStatsperforms an independent UPDATE/INSERT per result with no surrounding transaction. If the batch fails partway,property_score_statis left with some rows recalculated and others stale, producing an inconsistent snapshot. Annotate the bulk entry point with@Transactionalso the recalculation commits atomically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDao.java` around lines 385 - 393, The bulk upsert in upsertSafetyScoreStats currently runs each upsert independently, so a mid-batch failure can leave property_score_stat partially updated. Add a transactional boundary to the bulk entry point by annotating upsertSafetyScoreStats with `@Transactional` so all per-item upserts execute and commit atomically, keeping the recalculated snapshot consistent.backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java (1)
51-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPer-property facility query creates an N+1 access pattern in the batch.
recalculateAll()maps every active property throughcalculateForProperty→findNearbyCandidates, which issues onesafetyFacilityDao.findInBounds(...)call per property. For a batch over the full active inventory this is one DB round-trip per row, which will dominate runtime as the property count grows.Consider loading facilities once (or in spatial/grid chunks) and filtering in memory, or pushing the radius count down into a single spatial SQL aggregation. Acceptable for a small dataset, but worth addressing before inventory scales.
Also applies to: 116-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java` around lines 51 - 58, recalculateAll() is triggering an N+1 pattern because calculateForProperty() calls findNearbyCandidates(), which in turn hits safetyFacilityDao.findInBounds() once per property. Update PropertySafetyScoreServiceImpl so the batch does not query facilities per row: load facilities once or in larger spatial chunks and reuse them in memory, or replace the per-property lookup with a single aggregated spatial query. Keep the recalculation flow and upsertSafetyScoreStats() logic intact while changing the facility lookup path used by calculateForProperty()/findNearbyCandidates().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/main/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDao.java`:
- Around line 395-425: The current upsertSafetyScoreStat flow in JdbcPropertyDao
uses separate UPDATE and INSERT statements, which can race under concurrent
writes. Replace this with a single PostgreSQL native INSERT ... ON CONFLICT
(property_id) DO UPDATE using the same params map, and keep the conflict update
limited to the safety score/stat columns so unrelated fields like price_score
are not modified.
In `@docs/09_BATCH_INGESTION.md`:
- Line 181: The section heading for the Property Safety Score Calculation
Scheduler is mislabeled with a duplicate F-4 tag; update the heading text to use
F-5 so it matches the Phase 5 content and the numbering used by the surrounding
scheduler sections. Locate the markdown heading for “Property Safety Score
Calculation Scheduler” and correct the identifier while keeping the rest of the
section unchanged.
---
Nitpick comments:
In
`@backend/src/main/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDao.java`:
- Around line 385-393: The bulk upsert in upsertSafetyScoreStats currently runs
each upsert independently, so a mid-batch failure can leave property_score_stat
partially updated. Add a transactional boundary to the bulk entry point by
annotating upsertSafetyScoreStats with `@Transactional` so all per-item upserts
execute and commit atomically, keeping the recalculated snapshot consistent.
In
`@backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java`:
- Around line 51-58: recalculateAll() is triggering an N+1 pattern because
calculateForProperty() calls findNearbyCandidates(), which in turn hits
safetyFacilityDao.findInBounds() once per property. Update
PropertySafetyScoreServiceImpl so the batch does not query facilities per row:
load facilities once or in larger spatial chunks and reuse them in memory, or
replace the per-property lookup with a single aggregated spatial query. Keep the
recalculation flow and upsertSafetyScoreStats() logic intact while changing the
facility lookup path used by calculateForProperty()/findNearbyCandidates().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b30edf2a-5ed3-41b5-bbbf-5943993b9d1f
📒 Files selected for processing (13)
backend/src/main/java/com/ssafy/salmanhae/batch/PropertySafetyScoreScheduler.javabackend/src/main/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDao.javabackend/src/main/java/com/ssafy/salmanhae/model/dao/property/PropertyDao.javabackend/src/main/java/com/ssafy/salmanhae/model/dto/safety/PropertySafetyScoreInput.javabackend/src/main/java/com/ssafy/salmanhae/model/dto/safety/PropertySafetyScoreResult.javabackend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreService.javabackend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.javabackend/src/main/resources/application.propertiesbackend/src/test/java/com/ssafy/salmanhae/batch/PropertySafetyScoreSchedulerTest.javabackend/src/test/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDaoSafetyScoreTest.javabackend/src/test/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceTest.javabackend/src/test/resources/application-test.propertiesdocs/09_BATCH_INGESTION.md
| rows are accumulated through `SafetyFacilityDao.upsertAll`, so rerunning the batch is idempotent for | ||
| the unique `(type, source, source_id)` safety facility key. | ||
|
|
||
| ## F-4 Property Safety Score Calculation Scheduler |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Heading mislabeled F-4 (duplicate); should be F-5.
The section body refers to "Phase 5", but the heading reuses F-4 from the prior Ingestion Scheduler section.
📝 Proposed fix
-## F-4 Property Safety Score Calculation Scheduler
+## F-5 Property Safety Score Calculation Scheduler📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## F-4 Property Safety Score Calculation Scheduler | |
| ## F-5 Property Safety Score Calculation Scheduler |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/09_BATCH_INGESTION.md` at line 181, The section heading for the Property
Safety Score Calculation Scheduler is mislabeled with a duplicate F-4 tag;
update the heading text to use F-5 so it matches the Phase 5 content and the
numbering used by the surrounding scheduler sections. Locate the markdown
heading for “Property Safety Score Calculation Scheduler” and correct the
identifier while keeping the rest of the section unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java (1)
34-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the bounding box conservative before the haversine check.
111_320.0is slightly larger than the meters-per-degree implied byEARTH_RADIUS_M, so the prefilter can reject facilities thatdistanceMeters(...)would still count near the 500m boundary.Suggested adjustment
- private static final double METERS_PER_LATITUDE_DEGREE = 111_320.0; + private static final double METERS_PER_DEGREE = Math.toRadians(1.0) * EARTH_RADIUS_M; + private static final double BOUNDING_BOX_MARGIN_M = 1.0; ... - double latitudeDelta = POLICE_RADIUS_M / METERS_PER_LATITUDE_DEGREE; - double longitudeMetersPerDegree = METERS_PER_LATITUDE_DEGREE * Math.cos(Math.toRadians(latitude.doubleValue())); - double longitudeDelta = POLICE_RADIUS_M / Math.max(1.0, longitudeMetersPerDegree); + double candidateRadiusM = POLICE_RADIUS_M + BOUNDING_BOX_MARGIN_M; + double latitudeDelta = candidateRadiusM / METERS_PER_DEGREE; + double longitudeMetersPerDegree = METERS_PER_DEGREE * Math.cos(Math.toRadians(latitude.doubleValue())); + double longitudeDelta = candidateRadiusM / Math.max(1.0, longitudeMetersPerDegree);Also applies to: 128-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java` at line 34, The bounding-box prefilter in PropertySafetyScoreServiceImpl is slightly too tight because METERS_PER_LATITUDE_DEGREE uses a value larger than the haversine-derived meters-per-degree, which can drop valid facilities near the 500m cutoff. Update the constants and the bounding-box calculation used before distanceMeters(...) so the prefilter is conservative and never excludes points that the final haversine check would accept; apply the same adjustment wherever the latitude/longitude deltas are computed in this service.
🧹 Nitpick comments (1)
backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java (1)
52-60: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid the global facility cross-product in the batch.
findInBounds(... -180..180/-90..90)materializes every scoring facility, then each property scans the full list. For real batch sizes this becomesactiveProperties × facilities; consider fetching properties first and using a DB/spatial-grid candidate strategy per property envelope.Also applies to: 90-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java` around lines 52 - 60, The batch in PropertySafetyScoreServiceImpl is doing a global facility cross-product by loading all scoring facilities with safetyFacilityDao.findInBounds(...) and then re-scanning that full list for every property in calculateForProperty. Refactor the flow so propertyDao.findActivePropertiesForSafetyScoring() drives the batch first, then fetch only candidate facilities per property using a tighter envelope/spatial-grid query or similar bounded lookup instead of reusing one all-facilities list. Update the batching logic around calculateForProperty and the facility lookup path to avoid activeProperties × facilities behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/main/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDao.java`:
- Around line 406-410: The upsert logic in JdbcPropertyDao is probing
unsupported ON CONFLICT SQL inside the transaction, which can poison the
transaction before the fallback runs. Refactor the code path around
upsertSafetyScoreStat so the database-specific SQL is chosen before executing
any write, using a separate H2-compatible branch instead of catching
BadSqlGrammarException after upsertSafetyScoreStatWithOnConflict fails. Keep the
fallback logic in the same DAO method flow, but route H2 directly to
upsertSafetyScoreStatWithUpdateInsert and only use
upsertSafetyScoreStatWithOnConflict for databases that support it.
---
Outside diff comments:
In
`@backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java`:
- Line 34: The bounding-box prefilter in PropertySafetyScoreServiceImpl is
slightly too tight because METERS_PER_LATITUDE_DEGREE uses a value larger than
the haversine-derived meters-per-degree, which can drop valid facilities near
the 500m cutoff. Update the constants and the bounding-box calculation used
before distanceMeters(...) so the prefilter is conservative and never excludes
points that the final haversine check would accept; apply the same adjustment
wherever the latitude/longitude deltas are computed in this service.
---
Nitpick comments:
In
`@backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java`:
- Around line 52-60: The batch in PropertySafetyScoreServiceImpl is doing a
global facility cross-product by loading all scoring facilities with
safetyFacilityDao.findInBounds(...) and then re-scanning that full list for
every property in calculateForProperty. Refactor the flow so
propertyDao.findActivePropertiesForSafetyScoring() drives the batch first, then
fetch only candidate facilities per property using a tighter
envelope/spatial-grid query or similar bounded lookup instead of reusing one
all-facilities list. Update the batching logic around calculateForProperty and
the facility lookup path to avoid activeProperties × facilities behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5856358c-30e7-4b25-ad08-070fc9db3051
📒 Files selected for processing (4)
backend/src/main/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDao.javabackend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.javabackend/src/test/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceTest.javadocs/09_BATCH_INGESTION.md
✅ Files skipped from review due to trivial changes (2)
- backend/src/test/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceTest.java
- docs/09_BATCH_INGESTION.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java (1)
57-60: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep chunk bounds spatially local before merging.
findActivePropertiesForSafetyScoring()returns rows byid ASC, so each 100-property chunk can span unrelated regions. Merging those bounds can fetch a very large facility rectangle, then compare every fetched facility against every property in the chunk. Sort/group properties spatially before chunking, or query candidates per spatial tile.♻️ Minimal spatial ordering improvement
import java.util.ArrayList; +import java.util.Comparator; @@ public List<PropertySafetyScoreResult> recalculateAll() { List<PropertyRow> properties = propertyDao.findActivePropertiesForSafetyScoring(); - List<PropertySafetyScoreResult> results = new ArrayList<>(properties.size()); - for (int start = 0; start < properties.size(); start += PROPERTY_CHUNK_SIZE) { - List<PropertyRow> chunk = properties.subList(start, Math.min(start + PROPERTY_CHUNK_SIZE, properties.size())); + List<PropertyRow> spatiallyOrderedProperties = properties.stream() + .sorted(Comparator.comparing(PropertyRow::latitude) + .thenComparing(PropertyRow::longitude) + .thenComparing(PropertyRow::id)) + .toList(); + List<PropertySafetyScoreResult> results = new ArrayList<>(spatiallyOrderedProperties.size()); + for (int start = 0; start < spatiallyOrderedProperties.size(); start += PROPERTY_CHUNK_SIZE) { + List<PropertyRow> chunk = spatiallyOrderedProperties.subList( + start, + Math.min(start + PROPERTY_CHUNK_SIZE, spatiallyOrderedProperties.size()) + ); List<SafetyFacilityRow> facilities = findNearbyCandidates(chunk);Also applies to: 128-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java` around lines 57 - 60, The chunking in findActivePropertiesForSafetyScoring() and the nearby-candidate lookup in findNearbyCandidates() currently use id-ordered batches, which can merge distant properties into one large facility search area. Update the PropertySafetyScoreServiceImpl flow so properties are grouped or sorted by spatial proximity before batching, or split candidate queries per spatial tile, and keep the chunk bounds local before calling findNearbyCandidates() and merging results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@backend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.java`:
- Around line 57-60: The chunking in findActivePropertiesForSafetyScoring() and
the nearby-candidate lookup in findNearbyCandidates() currently use id-ordered
batches, which can merge distant properties into one large facility search area.
Update the PropertySafetyScoreServiceImpl flow so properties are grouped or
sorted by spatial proximity before batching, or split candidate queries per
spatial tile, and keep the chunk bounds local before calling
findNearbyCandidates() and merging results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f4edbcf7-aeaf-4eee-84ee-0bf12e0c4b35
📒 Files selected for processing (3)
backend/src/main/java/com/ssafy/salmanhae/model/dao/property/JdbcPropertyDao.javabackend/src/main/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceImpl.javabackend/src/test/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/test/java/com/ssafy/salmanhae/service/safety/PropertySafetyScoreServiceTest.java
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.
변경 내용
safety_facilityrows를 기반으로 매물별 CCTV/비상벨/보안등 300m, 경찰 500m 개수를 계산합니다.safety_score를 산출합니다.property_score_statupsert는safety_score와 안전시설 counts만 갱신하고 기존price_score는 보존합니다.연결 이슈
테스트 체크리스트
./mvnw.cmd -Dtest=PropertySafetyScoreServiceTest,PropertySafetyScoreSchedulerTest,JdbcPropertyDaoSafetyScoreTest test./mvnw.cmd test리뷰 포인트
GET /api/v1/properties/{id}/safety-summary는 기존처럼property_score_stat을 읽기 때문에, 이번 배치가 갱신한 score/counts가 별도 Controller 변경 없이 응답에 반영됩니다.INSERT ... ON CONFLICT로 upsert하고, H2 테스트 환경에서만 호환 fallback을 사용합니다.Summary by CodeRabbit