Repository navigation
Chore/docs cleanup and stage patch - #23
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements Meilisearch-backed product search with a MySQL fallback, integrates logistics tracking via Kdniao and map visualization via Amap, adds corresponding seed data, and polishes various frontend copies. The review identified several critical issues: a compilation error in MeilisearchProductSearchIndex due to passing a Duration instead of milliseconds to the client factory, missing timeouts on the Amap RestClient, and a type-checking bug in LogisticsTrackingServiceImpl where the JSON string addressSnapshot is not parsed before being evaluated as a Map. Additionally, the reviewer recommended optimizing search indexing by avoiding synchronous HTTP calls inside database transactions, paginating product records during reindexing to prevent memory exhaustion, and removing redundant Jackson conversions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| requestFactory.setConnectTimeout(properties.getTimeout()); | ||
| requestFactory.setReadTimeout(properties.getTimeout()); |
There was a problem hiding this comment.
The SimpleClientHttpRequestFactory.setConnectTimeout(int) and setReadTimeout(int) methods accept an integer representing milliseconds, not a java.time.Duration. Passing properties.getTimeout() directly will cause a compilation failure.
| requestFactory.setConnectTimeout(properties.getTimeout()); | |
| requestFactory.setReadTimeout(properties.getTimeout()); | |
| requestFactory.setConnectTimeout((int) properties.getTimeout().toMillis()); | |
| requestFactory.setReadTimeout((int) properties.getTimeout().toMillis()); |
| this.amapRestClient = RestClient.builder() | ||
| .baseUrl("https://restapi.amap.com") | ||
| .build(); |
There was a problem hiding this comment.
The amapRestClient is built without any connect or read timeouts configured. If the external Amap API experiences latency or hangs, it can block the request thread indefinitely, potentially leading to thread pool exhaustion and application-wide denial of service. Please configure reasonable timeouts using a ClientHttpRequestFactory.
org.springframework.http.client.SimpleClientHttpRequestFactory requestFactory =
new org.springframework.http.client.SimpleClientHttpRequestFactory();
requestFactory.setConnectTimeout(5000);
requestFactory.setReadTimeout(5000);
this.amapRestClient = RestClient.builder()
.baseUrl("https://restapi.amap.com")
.requestFactory(requestFactory)
.build();| private String addressText(Map<String, Object> fulfillment) { | ||
| Object snapshot = fulfillment == null ? null : fulfillment.get("addressSnapshot"); | ||
| if (!(snapshot instanceof Map<?, ?> address)) { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
The addressSnapshot field is stored as a JSON string in the database. When retrieved via JDBC, it is typically returned as a java.lang.String rather than a pre-parsed Map. Consequently, snapshot instanceof Map will evaluate to false, causing this method to silently return an empty string and skip geocoding entirely. You should check if the snapshot is a String and parse it using the injected objectMapper.
@SuppressWarnings("unchecked")
private String addressText(Map<String, Object> fulfillment) {
Object snapshot = fulfillment == null ? null : fulfillment.get("addressSnapshot");
if (snapshot == null) {
return "";
}
Map<?, ?> address = null;
if (snapshot instanceof Map<?, ?> map) {
address = map;
} else if (snapshot instanceof String str) {
if (str.isBlank()) {
return "";
}
try {
address = objectMapper.readValue(str, Map.class);
} catch (Exception e) {
// Ignore parsing errors and fallback to empty
}
}
if (address == null) {
return "";
}| public List<Map<String, Object>> findPublicSearchIndexDocuments() { | ||
| return jdbcTemplate.queryForList(baseSql() | ||
| + " WHERE p.is_deleted = FALSE AND p.status = 'on_sale'" | ||
| + " AND (p.review_status = 'not_required' OR p.review_status = 'approved')" | ||
| + " ORDER BY p.id") | ||
| .stream() | ||
| .map(this::toSearchIndexMap) | ||
| .collect(Collectors.toList()); | ||
| } |
There was a problem hiding this comment.
This method queries and loads the entire products table into memory at once to build the search index. As the product catalog grows, this will lead to high memory consumption and potentially trigger an OutOfMemoryError (OOM). Consider implementing keyset pagination (e.g., batching by p.id) or using a streaming query to process and index documents in smaller, manageable chunks.
| private void syncSearchDocument(Long productId) { | ||
| productMapper.findPublicSearchIndexDocumentById(productId) | ||
| .ifPresentOrElse(searchService::syncProductSearchDocument, | ||
| () -> searchService.removeProductSearchDocument(productId)); | ||
| } |
There was a problem hiding this comment.
The syncSearchDocument method performs synchronous external HTTP requests to Meilisearch. Since this method is called from mutating operations (such as addFavorite, removeFavorite, publishProduct, updateProduct, etc.) which typically run within a database transaction, any latency or downtime in Meilisearch will hold the database connection open. This can quickly exhaust the database connection pool and degrade application performance. Consider executing these search index sync operations asynchronously (e.g., using Spring's @Async or registering a transaction synchronization to run after commit).
| return list.stream() | ||
| .filter(Map.class::isInstance) | ||
| .map(hit -> objectMapper.convertValue(hit, new TypeReference<Map<String, Object>>() { | ||
| })) | ||
| .toList(); |
There was a problem hiding this comment.
Instantiating new TypeReference<Map<String, Object>>() {} inside a stream map operation creates a new anonymous class instance for every single search hit, which is inefficient. Since Jackson deserializes JSON objects into Map (specifically LinkedHashMap) by default, the hits are already Map instances. You can simply cast them to avoid the overhead of Jackson conversion and class instantiation.
return list.stream()
.filter(Map.class::isInstance)
.map(hit -> (Map<String, Object>) hit)
.toList();| Object response = restClient.post() | ||
| .uri(path) | ||
| .body(payload) | ||
| .retrieve() | ||
| .body(Object.class); | ||
| return objectMapper.convertValue(response, new TypeReference<Map<String, Object>>() { | ||
| }); |
There was a problem hiding this comment.
Using objectMapper.convertValue with new TypeReference to convert the response is redundant and inefficient because restClient can directly deserialize the response into a Map using body(Map.class).
| Object response = restClient.post() | |
| .uri(path) | |
| .body(payload) | |
| .retrieve() | |
| .body(Object.class); | |
| return objectMapper.convertValue(response, new TypeReference<Map<String, Object>>() { | |
| }); | |
| return restClient.post() | |
| .uri(path) | |
| .body(payload) | |
| .retrieve() | |
| .body(Map.class); |
What changed
Type
Testing
cd backend && mvnw.cmd test— all passcd frontend && npm test— all passcd frontend && npm run build— no errorsChecklist
CHANGELOG.mdprepended (skip for docs-only)docs/09-api-spec/).httpsmoke file updated if endpoint added or changed (docs/06-http/)docs/08-tasks/)Notes