Optimization of Location Processing and Data Stability#279
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds name-based store matching with a compact StoreMatchResponse API, expands matching to use catalog and external IDs across routing and inventory lookups, propagates catalogId through item services/DTOs, and adjusts location clustering to use UPSERT and relaxed DBSCAN/accuracy thresholds. ChangesMulti-Name Store Matching Flow
Catalog Linking and DTO Propagation
Location Clustering Parameter Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/p2ps/lists/service/ItemService.java (1)
231-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing-ID AI results can silently drop items.
On Line 241, when AI returns an item without
id,trackingMap.get(dto.getId())isnulland the item is skipped. If all DTOs lose IDs, Line 202 saves an empty list and user items disappear. TreatnullIDs as hallucination and trigger the same fallback as unknown IDs.Suggested fix
for (ItemDTO dto : validatedDtos) { - // 💡 FIX 1: Protecție împotriva halucinațiilor de ID - if (dto.getId() != null && !trackingMap.containsKey(dto.getId())) { + if (dto.getId() == null || !trackingMap.containsKey(dto.getId())) { logger.error("AI hallucinated an unknown ID: {}. Triggering full fallback.", dto.getId()); - return new ArrayList<>(trackingMap.values()); // Fallback total la lista originală! + return new ArrayList<>(trackingMap.values()); }🤖 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 `@src/main/java/com/p2ps/lists/service/ItemService.java` around lines 231 - 243, In applyAiValidationResults, treat DTOs with null IDs as hallucinations the same as unknown IDs: before calling trackingMap.get(dto.getId()) check if dto.getId() == null || !trackingMap.containsKey(dto.getId()), log the hallucination (using logger.error with the DTO or explicit "null id") and return a full fallback new ArrayList<>(trackingMap.values()) so items aren't silently dropped; update the existing if block that currently only checks containsKey to include the null-ID check and ensure subsequent code (applyRefinedAiUpdates, itemsToSave population) only runs when originalItem != null.
🧹 Nitpick comments (1)
src/test/java/com/p2ps/controller/RoutingControllerTest.java (1)
111-150: ⚡ Quick winAdd a lookup test for
itemNames-only requests.Current lookup tests only validate ID-based inputs. Add one case with
itemIds == nulland non-blankitemNamesto lock down request counting and responsematchPercentagebehavior for the new API path.🤖 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 `@src/test/java/com/p2ps/controller/RoutingControllerTest.java` around lines 111 - 150, Add a new unit test in RoutingControllerTest (e.g., lookupStores_shouldHandleItemNamesOnly) that builds a StoreMatchRequest with itemIds set to null and itemNames set to a non-empty list, then mock StoreMatchingEngine.findOptimalStores to expect (47.15, 27.58, 5000, null, request.getItemNames()) and return a sample matches list; call controller.lookupStores(request) and assert the ResponseEntity status is 200, body is not null, list size matches the mock, the first StoreMatchResponse.storeId matches the first mock result and its matchPercentage equals the expected computed value, and verify any request counting/metrics behavior if present. Ensure you reference controller.lookupStores, StoreMatchRequest, StoreMatchingEngine.findOptimalStores, StoreMatchResponse when adding the test.
🤖 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 `@src/main/java/com/p2ps/lists/service/ItemService.java`:
- Around line 434-435: Normalize the item's name before calling
resolveCatalogMatch so trailing/leading spaces don't prevent finding an existing
catalog link: retrieve and trim the name from item (use a null-safe/trim
operation), pass that normalized name into resolveCatalogMatch(itemName,
item.getBrand(), item.getShoppingList().getUser()) instead of item.getName(),
then continue to call attachRoutableExternalItemId(item) as before.
In `@src/main/java/com/p2ps/repository/StoreInventoryMapRepository.java`:
- Around line 55-56: In StoreInventoryMapRepository, the CASE in the ORDER BY
uses WHEN requested_item.external_item_id IS NOT NULL AND
located_item.external_item_id = requested_item.external_item_id THEN 2 which can
still prioritize empty strings even though the WHERE filter enforces
external_item_id <> ''; update that CASE to mirror the non-empty guard used in
filtering by adding checks like requested_item.external_item_id <> '' and
located_item.external_item_id <> '' (e.g., WHEN requested_item.external_item_id
IS NOT NULL AND requested_item.external_item_id <> '' AND
located_item.external_item_id = requested_item.external_item_id AND
located_item.external_item_id <> '' THEN 2) so the ordering prefers non-empty
external_id matches consistent with the WHERE clause.
In `@src/main/java/com/p2ps/service/StoreMatchingEngine.java`:
- Around line 69-70: The current COALESCE(ri.id, sim.item_id) fallback causes
one logical requested item to be counted multiple times when a store maps that
request to several inventory variants; instead, add a dedicated "requested_key"
CTE (or column) built from the original input identifier(s) (IDs, catalog/name)
and use that key wherever you produce requested_item_id and compute
matched_items; replace COALESCE(ri.id, sim.item_id) with the canonical
requested_key reference in the SELECTs and change aggregations (e.g.,
matched_items) to COUNT(DISTINCT requested_key) so counts are per logical
request not per inventory row (apply same change to the other occurrence around
the matched_items aggregation).
---
Outside diff comments:
In `@src/main/java/com/p2ps/lists/service/ItemService.java`:
- Around line 231-243: In applyAiValidationResults, treat DTOs with null IDs as
hallucinations the same as unknown IDs: before calling
trackingMap.get(dto.getId()) check if dto.getId() == null ||
!trackingMap.containsKey(dto.getId()), log the hallucination (using logger.error
with the DTO or explicit "null id") and return a full fallback new
ArrayList<>(trackingMap.values()) so items aren't silently dropped; update the
existing if block that currently only checks containsKey to include the null-ID
check and ensure subsequent code (applyRefinedAiUpdates, itemsToSave population)
only runs when originalItem != null.
---
Nitpick comments:
In `@src/test/java/com/p2ps/controller/RoutingControllerTest.java`:
- Around line 111-150: Add a new unit test in RoutingControllerTest (e.g.,
lookupStores_shouldHandleItemNamesOnly) that builds a StoreMatchRequest with
itemIds set to null and itemNames set to a non-empty list, then mock
StoreMatchingEngine.findOptimalStores to expect (47.15, 27.58, 5000, null,
request.getItemNames()) and return a sample matches list; call
controller.lookupStores(request) and assert the ResponseEntity status is 200,
body is not null, list size matches the mock, the first
StoreMatchResponse.storeId matches the first mock result and its matchPercentage
equals the expected computed value, and verify any request counting/metrics
behavior if present. Ensure you reference controller.lookupStores,
StoreMatchRequest, StoreMatchingEngine.findOptimalStores, StoreMatchResponse
when adding the test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a5d02bb-94d7-4b57-a098-3f6180475fc0
📒 Files selected for processing (15)
src/main/java/com/p2ps/controller/RoutingController.javasrc/main/java/com/p2ps/controller/StoreMatchRequest.javasrc/main/java/com/p2ps/controller/StoreMatchResponse.javasrc/main/java/com/p2ps/lists/dto/ItemDTO.javasrc/main/java/com/p2ps/lists/service/ItemService.javasrc/main/java/com/p2ps/lists/service/ShoppingListService.javasrc/main/java/com/p2ps/repository/StoreInventoryMapRepository.javasrc/main/java/com/p2ps/service/LocationProcessorWorker.javasrc/main/java/com/p2ps/service/RoutingService.javasrc/main/java/com/p2ps/service/StoreMatchingEngine.javasrc/test/java/com/p2ps/controller/RoutingControllerTest.javasrc/test/java/com/p2ps/controller/RoutingControllerWebMvcTest.javasrc/test/java/com/p2ps/lists/service/ItemServiceTest.javasrc/test/java/com/p2ps/service/LocationProcessorWorkerTest.javasrc/test/java/com/p2ps/service/StoreMatchingEngineTest.java
# Conflicts: # src/main/java/com/p2ps/lists/dto/ItemDTO.java # src/main/java/com/p2ps/lists/service/ItemService.java # src/test/java/com/p2ps/lists/service/ItemServiceTest.java
…x/store-matching-bug
…ing/server into fix/store-matching-bug
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixed issues with telemetry-imported stores disappearing and improved overall location accuracy.
Data Stability: Refactored LocationProcessorWorker to use atomic UPSERT operations instead of a DELETE-and-REINSERT cycle. This prevents the transient "0 stores found" state and ensures that manually populated test data is preserved alongside telemetry updates.
Accuracy Thresholds: Increased the GPS accuracy threshold from 12m to 30m and adjusted the DBSCAN clustering radius (eps) to 30m. These changes allow the system to successfully ingest and cluster mobile telemetry data which typically has a higher error margin than indoor WiFi-RTT signals.
Low-Density Clustering: Synchronized clustering requirements (minpoints := 3) across background and on-demand jobs, ensuring that items with few telemetry pings are still correctly mapped to store shelves.
Test Alignment: Updated theLocationProcessorWorkerTest suite to reflect the architectural shift to UPSERT semantics, ensuring full CI/CD compatibility and SonarQube quality gate compliance.
Summary by CodeRabbit
New Features
Improvements
Tests