7988 synchronize places from database with google places api #8015
Conversation
…abase-with-google-places-api' into 7988-synchronize-places-from-database-with-google-places-api
|
Warning Rate limit exceeded@Warded120 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces enhanced place filtering capabilities in the GreenCity application by integrating Google Places API functionality. The changes span multiple components, including security configuration, controllers, services, and data transfer objects. A new endpoint for filtering places from an external API has been added, along with comprehensive error handling and testing infrastructure to support this feature. Additionally, new methods and exception handling mechanisms have been introduced to ensure robust interactions with the Google Places API. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
service/src/main/java/greencity/service/GoogleApiService.java (2)
72-95: Add parameter validation and consider response pagination.The implementation looks solid, but consider these improvements:
- Add null validation for the userVO parameter
- The Places API supports pagination through next_page_token, which might be useful for handling large result sets
Consider adding parameter validation:
public List<PlacesSearchResult> getResultFromPlacesApi(FilterPlacesApiDto filterDto, UserVO userVO) { + if (userVO == null) { + throw new IllegalArgumentException("UserVO cannot be null"); + } + if (filterDto == null) { + throw new IllegalArgumentException("FilterPlacesApiDto cannot be null"); + } List<PlacesSearchResult> placesResults = new ArrayList<>();
105-133: Well-structured filter application logic!The implementation is clean and maintainable. Each filter is properly checked before application.
Consider using the builder pattern to make the request construction more fluent:
private static NearbySearchRequest applyFiltersToRequest(FilterPlacesApiDto filterDto, NearbySearchRequest request) { return Optional.of(request) .map(req -> filterDto.getKeyword() != null ? req.keyword(filterDto.getKeyword()) : req) .map(req -> filterDto.getType() != null ? req.type(filterDto.getType()) : req) // ... other filters .get(); }service/src/test/java/greencity/service/GoogleApiServiceTest.java (2)
116-144: Consider enhancing assertion coverageWhile the test effectively verifies the size of combined results, consider adding assertions for:
- The content/order of the results
- Individual locale responses
- Specific address components
This would provide more comprehensive validation of the geocoding functionality.
Example assertion additions:
// Verify content of results assertEquals(ModelUtils.getGeocodingResultUk()[0].formattedAddress, actual.get(0).formattedAddress); assertEquals(ModelUtils.getGeocodingResultEn()[0].formattedAddress, actual.get(ModelUtils.getGeocodingResultUk().length).formattedAddress);
146-172: Enhance error handling verificationThe test confirms that exceptions don't propagate, but consider verifying:
- The returned result (should it be empty list?)
- Logging of the failed attempts
- Error messages or specific exception types
Example enhancement:
List<GeocodingResult> results = googleApiService.getResultFromGeoCode(searchRequest); assertTrue(results.isEmpty(), "Should return empty list when both requests fail"); // Consider adding verification for error logging if applicableservice-api/src/main/java/greencity/exception/exceptions/PlaceAlreadyExistsException.java (1)
1-7: Well-structured exception class!The exception class is appropriately designed as an unchecked exception and uses Lombok's @StandardException for clean code. Consider adding Javadoc to document the specific scenarios when this exception is thrown.
@StandardException +/** + * Exception thrown when attempting to add a place that already exists in the database, + * typically when the coordinates match an existing place within a certain precision. + */ public class PlaceAlreadyExistsException extends RuntimeException {service-api/src/main/java/greencity/dto/filter/FilterPlacesApiDto.java (1)
13-23: Consider adding field validationsThe DTO fields could benefit from validation annotations to ensure data integrity:
radiusshould have a minimum value (typically > 0)locationshould not be null when performing location-based searchesminPriceandmaxPriceshould maintain a valid range relationshippublic class FilterPlacesApiDto { + @NotNull(message = "Location is required for place search") private LatLng location; + @Min(value = 1, message = "Radius must be greater than 0") private int radius; private RankBy rankBy; private String keyword; private PriceLevel minPrice; private PriceLevel maxPrice; private String name; private boolean openNow; private PlaceType type;dao/src/main/java/greencity/repository/LocationRepo.java (1)
34-42: Well-implemented coordinate comparison with precision handling!The native SQL query effectively handles coordinate comparison with 4 decimal places precision. A few considerations:
- The ROUND + CAST combination might affect index usage
- 4 decimal places ≈ 11m precision at the equator, which seems reasonable for place deduplication
Consider adding an index on (lat, lng) if not already present, to optimize these existence checks:
CREATE INDEX idx_locations_lat_lng ON locations (lat, lng);service-api/src/main/java/greencity/service/LocationService.java (1)
61-70: Consider using primitive types for required parametersThe method is well-documented and aligns with the repository implementation. However, since coordinates are essential for location checking, consider using primitive
doubleinstead ofDoubleto indicate that null values are not acceptable.- boolean existsByLatAndLng(Double lat, Double lng); + boolean existsByLatAndLng(double lat, double lng);service/src/main/java/greencity/service/LocationServiceImpl.java (1)
122-125: Clean implementation, consider adding parameter validation.The method effectively delegates to the repository layer. However, consider adding null checks and coordinate range validation.
@Override public boolean existsByLatAndLng(Double lat, Double lng) { + if (lat == null || lng == null) { + throw new IllegalArgumentException("Coordinates cannot be null"); + } + if (lat < -90 || lat > 90 || lng < -180 || lng > 180) { + throw new IllegalArgumentException("Invalid coordinates range"); + } return locationRepo.existsByLatAndLng(lat, lng); }service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
130-130: Clear error message format, consider adding Javadoc.The error message format is well-structured with proper precision for coordinates.
Consider adding Javadoc to document the format parameters:
/** * Error message for duplicate place coordinates. * Format parameters: * 1. %f - latitude (4 decimal places) * 2. %f - longitude (4 decimal places) */ public static final String PLACE_ALREADY_EXISTS = "Place with lat: %.4f and lng: %.4f already exists";core/src/test/java/greencity/controller/PlaceControllerTest.java (1)
435-463: Good test coverage, consider adding error scenarios.The test effectively verifies the happy path. Consider adding tests for:
- Invalid coordinates
- Missing required parameters
- API error responses
Example test case for invalid coordinates:
@Test void getFilteredPlacesFromApiWithInvalidCoordinates() throws Exception { String json = """ { "location": { "lat": 91, // Invalid latitude "lng": 181 // Invalid longitude }, "radius": 10000 } """; this.mockMvc.perform(post(placeLink + "/filter/api") .content(json) .principal(principal) .contentType(MediaType.APPLICATION_JSON)) .andExpect(status().isBadRequest()); }core/src/test/java/greencity/ModelUtils.java (1)
580-586: Consider initializing LocationDto fieldsThe empty LocationDto constructor might not provide meaningful test data. Consider initializing it with test coordinates and address information for more realistic test scenarios.
- .location(new LocationDto()) + .location(LocationDto.builder() + .id(1L) + .lat(50.4501) + .lng(30.5234) + .address("Test Address") + .build())service/src/test/java/greencity/service/PlaceServiceImplTest.java (1)
657-674: Consider adding error case tests for API integration.The happy path test is well implemented. Consider adding tests for:
- API timeout scenarios
- Invalid response formats
- Network failures
service/src/test/java/greencity/ModelUtils.java (1)
3332-3342: Consider adding more test data variations.The FilterPlacesApiDto builder provides good basic test data. Consider adding factory methods for:
- Edge cases (maximum radius, empty keyword)
- Invalid combinations (incompatible rankBy and radius)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/src/main/java/greencity/config/SecurityConfig.java(1 hunks)core/src/main/java/greencity/controller/PlaceController.java(3 hunks)core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java(2 hunks)core/src/main/resources/messages.properties(1 hunks)core/src/test/java/greencity/ModelUtils.java(4 hunks)core/src/test/java/greencity/controller/PlaceControllerTest.java(3 hunks)dao/src/main/java/greencity/repository/LocationRepo.java(2 hunks)service-api/pom.xml(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service-api/src/main/java/greencity/dto/filter/FilterPlacesApiDto.java(1 hunks)service-api/src/main/java/greencity/exception/exceptions/PlaceAlreadyExistsException.java(1 hunks)service-api/src/main/java/greencity/service/LocationService.java(1 hunks)service-api/src/main/java/greencity/service/PlaceService.java(3 hunks)service/src/main/java/greencity/service/GoogleApiService.java(3 hunks)service/src/main/java/greencity/service/LocationServiceImpl.java(1 hunks)service/src/main/java/greencity/service/PlaceServiceImpl.java(5 hunks)service/src/test/java/greencity/ModelUtils.java(5 hunks)service/src/test/java/greencity/service/GoogleApiServiceTest.java(3 hunks)service/src/test/java/greencity/service/PlaceServiceImplTest.java(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/resources/messages.properties
🔇 Additional comments (15)
service/src/main/java/greencity/service/GoogleApiService.java (2)
5-18: Clean and well-organized import additions!The new imports are properly organized and specifically target the required Google Places API functionality.
Line range hint
35-35: Verify thread safety of GeoApiContext usage.The
contextfield is shared across multiple methods. Let's verify its thread safety for concurrent API calls.service/src/test/java/greencity/service/GoogleApiServiceTest.java (2)
6-13: Well-organized imports!The new imports for Places API integration are properly organized and include all necessary classes for the enhanced functionality.
174-220: Excellent test coverage with room for enhancementThe test thoroughly covers the Places API integration, but consider these improvements:
- Reduce mock setup duplication:
private NearbySearchRequest setupMockRequest(NearbySearchRequest request, FilterPlacesApiDto filterDto, String language) { return when(request.radius(filterDto.getRadius())).thenReturn(request) .language(language).thenReturn(request) // ... other common setups .name(filterDto.getName()).thenReturn(request); }
- Add edge case tests:
- Empty/null filter parameters
- Maximum/minimum values for radius
- Invalid location coordinates
- Empty results handling
- Verify response transformation:
// Add assertions for specific fields assertEquals(expected.get(0).name, actual.get(0).name); assertEquals(expected.get(0).geometry, actual.get(0).geometry);service-api/src/main/java/greencity/dto/filter/FilterPlacesApiDto.java (1)
24-39: Review default values in defaultJsonThe default values need attention:
- Latitude and longitude of (0,0) is in the Atlantic Ocean
- A radius of 0 is invalid for place searches
- Consider documenting the purpose of these default values or adjusting them to more meaningful defaults
Could you clarify if these default values are intended for documentation/example purposes or actual default behavior?
service-api/src/main/java/greencity/service/PlaceService.java (1)
192-202: Well-documented API integration method.The method signature and documentation are clear and consistent with the existing codebase patterns.
core/src/test/java/greencity/ModelUtils.java (1)
568-578: Well-structured test utility method!The method provides a comprehensive test DTO with appropriate test data ranges and follows the builder pattern effectively.
core/src/main/java/greencity/controller/PlaceController.java (2)
259-261: Documentation improvements enhance clarity!The updated documentation clearly distinguishes between places retrieved from the database versus the Google Places API, which helps API consumers understand the data source.
283-285: Clean API documentation with hidden implementation details!Good use of @parameter(hidden = true) to keep the OpenAPI documentation clean while maintaining the required authentication context.
service/src/test/java/greencity/service/PlaceServiceImplTest.java (1)
788-811: Well-structured test for duplicate location validation!The test effectively verifies the business logic for preventing duplicate locations. Good use of mocking and verification steps.
service-api/pom.xml (1)
111-116: Verify Google Maps Services dependency version for security.The dependency is correctly configured, but please verify:
- If version 2.2.0 is the latest stable release
- Known security vulnerabilities in this version
Run this script to check for newer versions and security advisories:
✅ Verification successful
Google Maps Services dependency is up-to-date and secure
The version 2.2.0 is the latest stable release, and no security vulnerabilities were found in the GitHub Security Advisory Database. The dependency is properly configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Maven Central for latest version curl -s "https://search.maven.org/solrsearch/select?q=g:com.google.maps+AND+a:google-maps-services&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.google.maps:google-maps-services") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 536
core/src/main/java/greencity/config/SecurityConfig.java (1)
331-331: LGTM! Security configuration is properly set for the new endpoint.The new endpoint
/place/filter/apiis correctly configured with appropriate role-based access control, requiring authenticated users with USER, ADMIN, MODERATOR, or UBS_EMPLOYEE roles.core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java (2)
31-32: LGTM! Import statements are properly organized.The imports for
WrongIdExceptionandPlaceAlreadyExistsExceptionare correctly added.
218-224: LGTM! Exception handler is well implemented.The handler for
PlaceAlreadyExistsExceptionfollows the established pattern:
- Proper logging of the exception message
- Consistent error response structure
- Appropriate HTTP status code (BAD_REQUEST)
service/src/main/java/greencity/service/PlaceServiceImpl.java (1)
581-585: LGTM! Proper duplicate check implementation.The implementation correctly:
- Extracts latitude and longitude from geocoding results
- Checks for existing places at the same coordinates
- Throws a descriptive exception with formatted message
| private com.google.maps.model.LatLng getLocationFromUserVO(UserVO userVO) { | ||
| return new com.google.maps.model.LatLng(userVO.getUserLocationDto().getLatitude(), | ||
| userVO.getUserLocationDto().getLongitude()); | ||
| } |
There was a problem hiding this comment.
Add null safety checks for location conversion.
The method needs protection against null UserLocationDto to prevent NullPointerException.
Add null validation:
private com.google.maps.model.LatLng getLocationFromUserVO(UserVO userVO) {
+ if (userVO.getUserLocationDto() == null) {
+ throw new IllegalArgumentException("User location data is not available");
+ }
return new com.google.maps.model.LatLng(userVO.getUserLocationDto().getLatitude(),
userVO.getUserLocationDto().getLongitude());
}📝 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.
| private com.google.maps.model.LatLng getLocationFromUserVO(UserVO userVO) { | |
| return new com.google.maps.model.LatLng(userVO.getUserLocationDto().getLatitude(), | |
| userVO.getUserLocationDto().getLongitude()); | |
| } | |
| private com.google.maps.model.LatLng getLocationFromUserVO(UserVO userVO) { | |
| if (userVO.getUserLocationDto() == null) { | |
| throw new IllegalArgumentException("User location data is not available"); | |
| } | |
| return new com.google.maps.model.LatLng(userVO.getUserLocationDto().getLatitude(), | |
| userVO.getUserLocationDto().getLongitude()); | |
| } |
| /** | ||
| * The method which return a list of {@link PlaceByBoundsDto} filtered by values | ||
| * contained in the incoming {@link FilterPlacesApiDto} object. | ||
| * {@link PlaceByBoundsDto} are retrieved from Google Places API | ||
| * | ||
| * @param filterDto contains all information about the filtering of the list. | ||
| * @return a list of {@code PlaceByBoundsDto} | ||
| */ | ||
| @Operation(summary = "Return a list places from Google Geocoding API filtered by values contained " | ||
| + "in the incoming FilterPlaceDto object") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = HttpStatuses.OK, | ||
| content = @Content(schema = @Schema(example = FilterPlacesApiDto.defaultJson))), | ||
| @ApiResponse(responseCode = "303", description = HttpStatuses.SEE_OTHER, | ||
| content = @Content(examples = @ExampleObject(HttpStatuses.SEE_OTHER))), | ||
| @ApiResponse(responseCode = "400", description = HttpStatuses.BAD_REQUEST, | ||
| content = @Content(examples = @ExampleObject(HttpStatuses.BAD_REQUEST))), | ||
| @ApiResponse(responseCode = "403", description = HttpStatuses.FORBIDDEN, | ||
| content = @Content(examples = @ExampleObject(HttpStatuses.FORBIDDEN))), | ||
| @ApiResponse(responseCode = "404", description = HttpStatuses.NOT_FOUND, | ||
| content = @Content(examples = @ExampleObject(HttpStatuses.NOT_FOUND))) | ||
| }) | ||
| @PostMapping("/filter/api") | ||
| public ResponseEntity<List<PlaceByBoundsDto>> getFilteredPlacesFromApi( | ||
| @Schema( | ||
| description = "Filters for places from API", | ||
| name = "FilterPlacesApiDto", | ||
| type = "object", | ||
| example = FilterPlacesApiDto.defaultJson) @Valid @RequestBody FilterPlacesApiDto filterDto, | ||
| @CurrentUser @Parameter(hidden = true) UserVO userVO) { | ||
| return ResponseEntity.ok().body(placeService.getPlacesByFilter(filterDto, userVO)); |
There was a problem hiding this comment.
💡 Codebase verification
Consider implementing robust rate limiting and error handling for Google Places API
The current implementation makes parallel requests for multiple locales without rate limiting protection. To prevent API quota exhaustion and ensure service reliability:
- Implement a rate limiter (e.g., token bucket algorithm) to control requests per second
- Add specific error handling for quota exceeded scenarios:
try {
// API call
} catch (ApiException e) {
if (e.getMessage().contains("OVER_QUERY_LIMIT")) {
// Handle rate limit exceeded
}
}- Consider caching frequently requested results to reduce API calls
🔗 Analysis chain
Verify rate limiting for Google Places API integration.
The endpoint is well-documented with appropriate response codes. However, consider:
- Implementing rate limiting to prevent API quota exhaustion
- Adding error handling for Google Places API failures
Run this script to check for rate limit handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for rate limit handling in the codebase
rg -A 5 "RateLimitExceededException|GoogleApiException"
Length of output: 57
Script:
#!/bin/bash
# Search for Google Places API related code and configuration
echo "=== Searching for Google Places API related files ==="
fd -t f -e java -e properties -e yml | xargs rg -l "google.*places|places.*api"
echo -e "\n=== Checking service implementation ==="
ast-grep --pattern 'class $_ implements $_ {
$$$
getPlacesByFilter($_, $_) {
$$$
}
$$$
}'
Length of output: 397
Script:
#!/bin/bash
# Search with broader patterns
echo "=== Searching for Google API related code ==="
rg -l "google.*api|googleapi|places.*service" --type java
echo -e "\n=== Checking for configuration properties ==="
rg -l "google\." --type properties --type yml
echo -e "\n=== Looking for PlaceService implementation ==="
rg -A 10 "class.*PlaceService.*implements" --type java
Length of output: 4298
Script:
#!/bin/bash
# Check the implementation and configuration
echo "=== Checking PlaceService implementation ==="
rg -A 20 "getPlacesByFilter.*FilterPlacesApiDto" service/src/main/java/greencity/service/PlaceServiceImpl.java
echo -e "\n=== Checking application properties ==="
cat core/src/main/resources/application.properties | grep -A 5 "google"
Length of output: 1718
Script:
#!/bin/bash
# Check Google API Service implementation
echo "=== Checking Google API Service implementation ==="
rg -A 10 "class.*GoogleApiService" --type java
echo "=== Checking for Google API related exception handling ==="
rg -A 5 "getResultFromPlacesApi|GoogleApiService" --type java
Length of output: 11814
| @Override | ||
| public List<PlaceByBoundsDto> getPlacesByFilter(FilterPlacesApiDto filterDto, UserVO userVO) { | ||
| List<PlacesSearchResult> fromPlacesApi = googleApiService.getResultFromPlacesApi(filterDto, userVO); | ||
| return fromPlacesApi.stream() | ||
| .map(el -> new PlaceByBoundsDto( | ||
| System.currentTimeMillis(), | ||
| el.name, | ||
| new LocationDto(System.currentTimeMillis(), el.geometry.location.lat, el.geometry.location.lng, | ||
| el.vicinity))) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a proper ID generation strategy.
Using System.currentTimeMillis() for IDs is not recommended as it:
- Can lead to duplicate IDs if multiple places are created in the same millisecond
- Makes the IDs predictable and potentially vulnerable to enumeration attacks
Consider using:
- UUID.randomUUID() for globally unique identifiers
- Database auto-generated sequence for sequential IDs
- A dedicated ID generation service for distributed systems
- System.currentTimeMillis(),
+ UUID.randomUUID().toString(),
- new LocationDto(System.currentTimeMillis(), el.geometry.location.lat, el.geometry.location.lng,
+ new LocationDto(UUID.randomUUID().toString(), el.geometry.location.lat, el.geometry.location.lng,Committable suggestion skipped: line range outside the PR's diff.
…abase-with-google-places-api' into 7988-synchronize-places-from-database-with-google-places-api
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dao/src/main/java/greencity/repository/LocationRepo.java (1)
36-44: Consider performance optimization for coordinate comparison.The implementation is correct, but there are a few suggestions to consider:
- The current query performs rounding for every row in the table. Consider creating computed/generated columns for rounded coordinates to improve query performance.
- The 4 decimal places precision gives approximately 11 meters accuracy at the equator. Consider documenting this business decision in the comments.
Here's an example of how to create computed columns:
ALTER TABLE locations ADD COLUMN lat_rounded NUMERIC GENERATED ALWAYS AS (ROUND(CAST(lat AS numeric), 4)) STORED, ADD COLUMN lng_rounded NUMERIC GENERATED ALWAYS AS (ROUND(CAST(lng AS numeric), 4)) STORED; CREATE INDEX idx_location_coords ON locations(lat_rounded, lng_rounded);Then the query could be simplified to:
- @Query(value = """ - SELECT EXISTS ( - SELECT 1 - FROM locations l - WHERE ROUND(CAST(l.lat AS numeric), 4) = ROUND(CAST(:lat AS numeric), 4) - AND ROUND(CAST(l.lng AS numeric), 4) = ROUND(CAST(:lng AS numeric), 4) - ) - """, nativeQuery = true) + @Query(value = """ + SELECT EXISTS ( + SELECT 1 + FROM locations l + WHERE l.lat_rounded = ROUND(CAST(:lat AS numeric), 4) + AND l.lng_rounded = ROUND(CAST(:lng AS numeric), 4) + ) + """, nativeQuery = true)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dao/src/main/java/greencity/repository/LocationRepo.java(2 hunks)service-api/src/main/java/greencity/service/LocationService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- service-api/src/main/java/greencity/service/LocationService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
dao/src/main/java/greencity/repository/LocationRepo.java (3)
6-7: LGTM! Clean import organization.The new imports are properly organized and specifically imported rather than using wildcard imports.
25-35: Well-documented method with clear purpose!The Javadoc is comprehensive and clearly explains the method's behavior, including the important detail about 4 decimal places precision.
36-44: Verify existing indexes on the locations table.Let's check if there are any existing indexes that could affect query performance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
service/src/test/java/greencity/service/PlaceServiceImplTest.java (2)
657-674: Good test coverage, consider adding edge cases.The test effectively verifies the happy path for Google Places API integration. Consider adding test cases for:
- Empty API results
- API error responses
- Rate limit scenarios
@Test void getPlacesByFilterFromApiTest() { FilterPlacesApiDto filterDto = getFilterPlacesApiDto(); when(googleApiService.getResultFromPlacesApi(filterDto, userVO)).thenReturn(getPlacesSearchResultEn()); var result = placeService.getPlacesByFilter(filterDto, userVO); List<PlaceByBoundsDto> updatedResult = result.stream().map(el -> { el.setId(1L); el.getLocation().setId(1L); return el; }).toList(); List<PlaceByBoundsDto> expectedResult = getPlaceByBoundsDtoFromApi(); Assertions.assertArrayEquals(updatedResult.toArray(), expectedResult.toArray()); verify(googleApiService).getResultFromPlacesApi(filterDto, userVO); + + // Test empty results + when(googleApiService.getResultFromPlacesApi(filterDto, userVO)).thenReturn(Collections.emptyList()); + assertTrue(placeService.getPlacesByFilter(filterDto, userVO).isEmpty()); + + // Test API error + when(googleApiService.getResultFromPlacesApi(filterDto, userVO)).thenThrow(new RuntimeException("API Error")); + assertThrows(RuntimeException.class, () -> placeService.getPlacesByFilter(filterDto, userVO)); }
787-809: Consider a more descriptive test name.The test effectively verifies duplicate location handling. However, the test name could be more descriptive to better indicate the scenario being tested.
-void addPlaceFromUiSaveAlreadyExistingLocation() { +void addPlaceFromUi_WhenLocationAlreadyExists_ThrowsException() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/service/PlaceServiceImplTest.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
service/src/test/java/greencity/service/PlaceServiceImplTest.java (1)
10-10: LGTM! Well-organized imports.The new imports are properly organized and directly support the new test functionality.
Also applies to: 41-41, 68-70
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
service/src/test/java/greencity/service/GoogleApiServiceTest.java (2)
117-173: Consider enhancing test method clarity and maintainability.The geocoding test methods are well-structured, but could be improved:
- Consider making test method names more descriptive:
-void getResultsFromGeocodeTest() +void getResultFromGeoCode_WhenBothLocalesSucceed_ReturnsCombinedResults() -void getResultsFromGeocodeThrowsTest() +void getResultFromGeoCode_WhenBothLocalesFail_ReturnsEmptyList()
- Extract repeated test data as constants:
private static final String TEST_SEARCH_REQUEST = "testSearchRequest"; private static final Locale LOCALE_UK = Locale.forLanguageTag("uk"); private static final Locale LOCALE_EN = Locale.forLanguageTag("en");
175-309: Consider adding tests for additional edge cases.While the current test coverage is good, consider adding tests for these scenarios:
- Empty results from both locales
- Mixed results (success from one locale, empty from another)
- Invalid filter parameters (negative radius, invalid price range)
- Rate limit exceeded scenarios
Example for empty results test:
@Test void getResultFromPlacesApi_WhenBothLocalesReturnEmpty_ReturnsEmptyList() { // Setup similar to existing tests but return empty results when(requestUk.await()).thenReturn(new PlacesSearchResult[0]); when(requestEn.await()).thenReturn(new PlacesSearchResult[0]); // Assert empty result }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/service/GoogleApiServiceTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
service/src/test/java/greencity/service/GoogleApiServiceTest.java (1)
6-7: Well-structured imports and class setup!The new imports are properly organized and align well with the added functionality for Places API integration.
Also applies to: 10-11, 13-13, 15-15, 17-17, 26-26
|
|
||
| List<PlacesSearchResult> actual = googleApiService.getResultFromPlacesApi(filterDto, userVO); | ||
|
|
||
| List<PlacesSearchResult> expected = List.of(ModelUtils.getPlacesSearchResultUk().getFirst(), | ||
| ModelUtils.getPlacesSearchResultEn().getFirst()); | ||
|
|
||
| assertEquals(actual.size(), expected.size()); | ||
|
|
||
| verify(requestUk, times(1)).await(); | ||
| verify(requestEn, times(1)).await(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void getResultFromPlacesApiNullLocationTest() throws IOException, InterruptedException, ApiException { | ||
| FilterPlacesApiDto filterDto = ModelUtils.getFilterPlacesApiDto(); | ||
| filterDto.setLocation(null); | ||
| UserVO userVO = ModelUtils.getUserVO(); | ||
| userVO.getUserLocationDto().setLatitude(null); | ||
|
|
||
| Locale localeUk = Locale.forLanguageTag("uk"); | ||
| Locale localeEn = Locale.forLanguageTag("en"); | ||
| try (MockedStatic<PlacesApi> placesApiMockedStatic = mockStatic(PlacesApi.class)) { | ||
| NearbySearchRequest requestUk = mock(NearbySearchRequest.class); | ||
| NearbySearchRequest requestEn = mock(NearbySearchRequest.class); | ||
|
|
||
| when(PlacesApi.nearbySearchQuery(context, filterDto.getLocation())).thenReturn(requestUk, requestEn); | ||
|
|
||
| when(requestUk.radius(filterDto.getRadius())).thenReturn(requestUk); | ||
| when(requestUk.language(localeUk.getLanguage())).thenReturn(requestUk); | ||
| when(requestUk.keyword(filterDto.getKeyword())).thenReturn(requestUk); | ||
| when(requestUk.type(filterDto.getType())).thenReturn(requestUk); | ||
| when(requestUk.rankby(filterDto.getRankBy())).thenReturn(requestUk); | ||
| when(requestUk.minPrice(filterDto.getMinPrice())).thenReturn(requestUk); | ||
| when(requestUk.maxPrice(filterDto.getMaxPrice())).thenReturn(requestUk); | ||
| when(requestUk.openNow(filterDto.isOpenNow())).thenReturn(requestUk); | ||
| when(requestUk.name(filterDto.getName())).thenReturn(requestUk); | ||
|
|
||
| when(requestEn.radius(filterDto.getRadius())).thenReturn(requestEn); | ||
| when(requestEn.language(localeEn.getLanguage())).thenReturn(requestEn); | ||
| when(requestEn.keyword(filterDto.getKeyword())).thenReturn(requestEn); | ||
| when(requestEn.type(filterDto.getType())).thenReturn(requestEn); | ||
| when(requestEn.rankby(filterDto.getRankBy())).thenReturn(requestEn); | ||
| when(requestEn.minPrice(filterDto.getMinPrice())).thenReturn(requestEn); | ||
| when(requestEn.maxPrice(filterDto.getMaxPrice())).thenReturn(requestEn); | ||
| when(requestEn.openNow(filterDto.isOpenNow())).thenReturn(requestEn); | ||
| when(requestEn.name(filterDto.getName())).thenReturn(requestEn); | ||
|
|
||
| when(requestUk.await()).thenReturn(ModelUtils.getPlacesSearchResponseUk()); | ||
| when(requestEn.await()).thenReturn(ModelUtils.getPlacesSearchResponseEn()); | ||
|
|
||
| assertThrows(NotFoundException.class, () -> googleApiService.getResultFromPlacesApi(filterDto, userVO)); | ||
|
|
||
| verify(requestUk, times(0)).await(); | ||
| verify(requestEn, times(0)).await(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void getResultFromPlacesApiThrowsApiExceptionTest() throws IOException, InterruptedException, ApiException { | ||
| FilterPlacesApiDto filterDto = ModelUtils.getFilterPlacesApiDto(); | ||
| UserVO userVO = ModelUtils.getUserVO(); | ||
|
|
||
| Locale localeUk = Locale.forLanguageTag("uk"); | ||
| Locale localeEn = Locale.forLanguageTag("en"); | ||
| try (MockedStatic<PlacesApi> placesApiMockedStatic = mockStatic(PlacesApi.class)) { | ||
| NearbySearchRequest requestUk = mock(NearbySearchRequest.class); | ||
| NearbySearchRequest requestEn = mock(NearbySearchRequest.class); | ||
|
|
||
| when(PlacesApi.nearbySearchQuery(context, filterDto.getLocation())).thenReturn(requestUk, requestEn); | ||
|
|
||
| when(requestUk.radius(filterDto.getRadius())).thenReturn(requestUk); | ||
| when(requestUk.language(localeUk.getLanguage())).thenReturn(requestUk); | ||
| when(requestUk.keyword(filterDto.getKeyword())).thenReturn(requestUk); | ||
| when(requestUk.type(filterDto.getType())).thenReturn(requestUk); | ||
| when(requestUk.rankby(filterDto.getRankBy())).thenReturn(requestUk); | ||
| when(requestUk.minPrice(filterDto.getMinPrice())).thenReturn(requestUk); | ||
| when(requestUk.maxPrice(filterDto.getMaxPrice())).thenReturn(requestUk); | ||
| when(requestUk.openNow(filterDto.isOpenNow())).thenReturn(requestUk); | ||
| when(requestUk.name(filterDto.getName())).thenReturn(requestUk); | ||
|
|
||
| when(requestEn.radius(filterDto.getRadius())).thenReturn(requestEn); | ||
| when(requestEn.language(localeEn.getLanguage())).thenReturn(requestEn); | ||
| when(requestEn.keyword(filterDto.getKeyword())).thenReturn(requestEn); | ||
| when(requestEn.type(filterDto.getType())).thenReturn(requestEn); | ||
| when(requestEn.rankby(filterDto.getRankBy())).thenReturn(requestEn); | ||
| when(requestEn.minPrice(filterDto.getMinPrice())).thenReturn(requestEn); | ||
| when(requestEn.maxPrice(filterDto.getMaxPrice())).thenReturn(requestEn); | ||
| when(requestEn.openNow(filterDto.isOpenNow())).thenReturn(requestEn); | ||
| when(requestEn.name(filterDto.getName())).thenReturn(requestEn); | ||
|
|
||
| when(requestUk.await()).thenReturn(ModelUtils.getPlacesSearchResponseUk()); | ||
| when(requestEn.await()).thenThrow(ApiException.class); | ||
|
|
||
| assertDoesNotThrow(() -> googleApiService.getResultFromPlacesApi(filterDto, userVO)); | ||
|
|
||
| verify(requestUk, times(1)).await(); | ||
| verify(requestEn, times(1)).await(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in Places API test methods.
The Places API test methods contain significant duplication in mock setup. Consider extracting common setup code:
private NearbySearchRequest setupMockRequest(NearbySearchRequest request, FilterPlacesApiDto filterDto, Locale locale) {
when(request.radius(filterDto.getRadius())).thenReturn(request);
when(request.language(locale.getLanguage())).thenReturn(request);
when(request.keyword(filterDto.getKeyword())).thenReturn(request);
when(request.type(filterDto.getType())).thenReturn(request);
when(request.rankby(filterDto.getRankBy())).thenReturn(request);
when(request.minPrice(filterDto.getMinPrice())).thenReturn(request);
when(request.maxPrice(filterDto.getMaxPrice())).thenReturn(request);
when(request.openNow(filterDto.isOpenNow())).thenReturn(request);
when(request.name(filterDto.getName())).thenReturn(request);
return request;
}This would simplify your test methods significantly:
@Test
void getResultFromPlacesApiTest() throws IOException, InterruptedException, ApiException {
FilterPlacesApiDto filterDto = ModelUtils.getFilterPlacesApiDto();
UserVO userVO = ModelUtils.getUserVO();
try (MockedStatic<PlacesApi> placesApiMockedStatic = mockStatic(PlacesApi.class)) {
NearbySearchRequest requestUk = setupMockRequest(
mock(NearbySearchRequest.class), filterDto, LOCALE_UK);
NearbySearchRequest requestEn = setupMockRequest(
mock(NearbySearchRequest.class), filterDto, LOCALE_EN);
// ... rest of the test
}
}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
service/src/test/java/greencity/service/GoogleApiServiceTest.java (1)
175-221: 🛠️ Refactor suggestionReduce code duplication in Places API test methods.
The Places API test methods contain significant duplication in mock setup for both Ukrainian and English locales. This makes the tests harder to maintain and more prone to errors.
Extract the common setup code into helper methods as suggested in the previous review:
private NearbySearchRequest setupMockRequest(NearbySearchRequest request, FilterPlacesApiDto filterDto, Locale locale) { when(request.radius(filterDto.getRadius())).thenReturn(request); when(request.language(locale.getLanguage())).thenReturn(request); when(request.keyword(filterDto.getKeyword())).thenReturn(request); when(request.type(filterDto.getType())).thenReturn(request); when(request.rankby(filterDto.getRankBy())).thenReturn(request); when(request.minPrice(filterDto.getMinPrice())).thenReturn(request); when(request.maxPrice(filterDto.getMaxPrice())).thenReturn(request); when(request.openNow(filterDto.isOpenNow())).thenReturn(request); when(request.name(filterDto.getName())).thenReturn(request); return request; }This would significantly simplify your test methods:
@Test void getResultFromPlacesApiTest() throws IOException, InterruptedException, ApiException { FilterPlacesApiDto filterDto = ModelUtils.getFilterPlacesApiDto(); UserVO userVO = ModelUtils.getUserVO(); try (MockedStatic<PlacesApi> placesApiMockedStatic = mockStatic(PlacesApi.class)) { NearbySearchRequest requestUk = setupMockRequest( mock(NearbySearchRequest.class), filterDto, Locale.forLanguageTag("uk")); NearbySearchRequest requestEn = setupMockRequest( mock(NearbySearchRequest.class), filterDto, Locale.forLanguageTag("en")); when(PlacesApi.nearbySearchQuery(context, filterDto.getLocation())) .thenReturn(requestUk, requestEn); when(requestUk.await()).thenReturn(ModelUtils.getPlacesSearchResponseUk()); when(requestEn.await()).thenReturn(ModelUtils.getPlacesSearchResponseEn()); List<PlacesSearchResult> actual = googleApiService.getResultFromPlacesApi(filterDto, userVO); assertEquals(2, actual.size()); verify(requestUk, times(1)).await(); verify(requestEn, times(1)).await(); } }Also applies to: 223-266, 268-309
🧹 Nitpick comments (1)
service/src/test/java/greencity/service/GoogleApiServiceTest.java (1)
117-145: Test method looks good but could benefit from descriptive test name.The test logic for
getResultsFromGeocodeTestis well-structured and covers both Ukrainian and English locales. However, the test name could be more descriptive of what it's actually testing.Consider renaming to
getResultsFromGeocode_WhenBothLocalesSucceed_ReturnsCombinedResultsto better describe the test scenario and expected outcome.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/service/GoogleApiServiceTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
service/src/test/java/greencity/service/GoogleApiServiceTest.java (3)
147-173: Test method effectively verifies error handling.The
getResultsFromGeocodeThrowsTestproperly verifies that the service gracefully handles API exceptions for both locales.
223-266: Good edge case testing for null location.The test
getResultFromPlacesApiNullLocationTestproperly verifies that aNotFoundExceptionis thrown when location is null. The verification of zero calls toawait()is a nice touch.
268-309: Good error handling test for API exception.The test
getResultFromPlacesApiThrowsApiExceptionTesteffectively verifies that the service gracefully handles API exceptions and continues processing.
service/src/test/java/greencity/service/GoogleApiServiceTest.java
Outdated
Show resolved
Hide resolved
service-api/src/main/java/greencity/dto/filter/FilterPlacesApiDto.java
Outdated
Show resolved
Hide resolved
…abase-with-google-places-api' into 7988-synchronize-places-from-database-with-google-places-api
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
service/src/test/java/greencity/service/GoogleApiServiceTest.java (1)
177-223: 🛠️ Refactor suggestionReduce code duplication in Places API test setup.
There's significant duplication in the mock setup for the Places API requests between Ukrainian and English locales.
Extract the common setup code into a helper method:
private NearbySearchRequest setupMockRequest(NearbySearchRequest request, FilterPlacesApiDto filterDto, Locale locale) { when(request.radius(filterDto.getRadius())).thenReturn(request); when(request.language(locale.getLanguage())).thenReturn(request); when(request.keyword(filterDto.getKeyword())).thenReturn(request); when(request.type(filterDto.getType())).thenReturn(request); when(request.rankby(filterDto.getRankBy())).thenReturn(request); when(request.minPrice(filterDto.getMinPrice())).thenReturn(request); when(request.maxPrice(filterDto.getMaxPrice())).thenReturn(request); when(request.openNow(filterDto.isOpenNow())).thenReturn(request); when(request.name(filterDto.getName())).thenReturn(request); return request; }This would simplify the test to:
@Test void getResultFromPlacesApiTest() throws IOException, InterruptedException, ApiException { FilterPlacesApiDto filterDto = ModelUtils.getFilterPlacesApiDto(); UserVO userVO = ModelUtils.getUserVO(); try (MockedStatic<PlacesApi> placesApiMockedStatic = mockStatic(PlacesApi.class)) { NearbySearchRequest requestUk = setupMockRequest( mock(NearbySearchRequest.class), filterDto, Locale.forLanguageTag("uk")); NearbySearchRequest requestEn = setupMockRequest( mock(NearbySearchRequest.class), filterDto, Locale.forLanguageTag("en")); when(PlacesApi.nearbySearchQuery(context, filterDto.getLocation())) .thenReturn(requestUk, requestEn); when(requestUk.await()).thenReturn(ModelUtils.getPlacesSearchResponseUk()); when(requestEn.await()).thenReturn(ModelUtils.getPlacesSearchResponseEn()); List<PlacesSearchResult> actual = googleApiService.getResultFromPlacesApi(filterDto, userVO); List<PlacesSearchResult> expected = List.of( ModelUtils.getPlacesSearchResultUk().getFirst(), ModelUtils.getPlacesSearchResultEn().getFirst() ); assertEquals(actual.size(), expected.size()); verify(requestUk, times(1)).await(); verify(requestEn, times(1)).await(); } }
🧹 Nitpick comments (6)
service/src/test/java/greencity/service/GoogleApiServiceTest.java (1)
215-216: Consider using a more descriptive assertion message.Add a descriptive message to the assertion to make test failures more informative.
- assertEquals(actual.size(), expected.size()); + assertEquals(actual.size(), expected.size(), + "Expected combined results from both locales");core/src/test/java/greencity/ModelUtils.java (2)
572-582: Add documentation for the test scenario.The method provides comprehensive test data, but adding a brief comment explaining the test scenario would improve maintainability.
+ /** + * Creates a FilterPlacesApiDto with default test values for Google Places API filtering. + * Includes location (0,0), 10km radius, test keyword, prominence ranking, + * and full price range (FREE to VERY_EXPENSIVE). + * + * @return FilterPlacesApiDto configured for testing + */ public static FilterPlacesApiDto getFilterPlacesApiDto() {
584-590: Enhance test data quality.The current implementation uses an empty LocationDto, which might not provide sufficient coverage for testing scenarios.
Consider enhancing the test data:
public static List<PlaceByBoundsDto> getPlaceByBoundsDto() { return List.of(PlaceByBoundsDto.builder() .id(1L) .name("testx") - .location(new LocationDto()) + .location(LocationDto.builder() + .lat(50.4501) + .lng(30.5234) + .address("1 Test Street") + .build()) .build()); }service/src/test/java/greencity/ModelUtils.java (3)
546-550: Consider using meaningful test coordinates.The hardcoded coordinates (1d, 1d) could be replaced with more realistic test values to better represent real-world scenarios and edge cases.
- .latitude(1d) - .longitude(1d) + .latitude(50.4501) // Kyiv coordinates + .longitude(30.5234)
3349-3363: Enhance test data realism in place bounds DTOs.The test location data could be more realistic and comprehensive.
public static List<PlaceByBoundsDto> getPlaceByBoundsDtoFromApi() { return List.of(PlaceByBoundsDto.builder() .id(1L) - .name("testName") + .name("Forum Lviv") .location(new LocationDto(1L, - 1d, - 1d, - "testVicinity" + 49.8419, + 24.0315, + "Forum Lviv, 7B Pid Dubom Street" )) .build()); }
3365-3406: Extract localized test data into constants.The hardcoded strings for names and addresses in both English and Ukrainian could be moved to constants for better maintainability.
+ private static final String TEST_PLACE_NAME_UK = "Форум Львів"; + private static final String TEST_PLACE_VICINITY_UK = "вул. Під Дубом, 7Б"; + private static final String TEST_PLACE_NAME_EN = "Forum Lviv"; + private static final String TEST_PLACE_VICINITY_EN = "7B Pid Dubom Street"; public static List<PlacesSearchResult> getPlacesSearchResultUk() { PlacesSearchResult placesSearchResult = new PlacesSearchResult(); - placesSearchResult.name = "тестІмя"; - placesSearchResult.vicinity = "тестАдреса"; + placesSearchResult.name = TEST_PLACE_NAME_UK; + placesSearchResult.vicinity = TEST_PLACE_VICINITY_UK;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/greencity/controller/PlaceController.java(3 hunks)core/src/test/java/greencity/ModelUtils.java(4 hunks)service-api/src/main/java/greencity/dto/filter/FilterPlacesApiDto.java(1 hunks)service/src/test/java/greencity/ModelUtils.java(4 hunks)service/src/test/java/greencity/service/GoogleApiServiceTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- service-api/src/main/java/greencity/dto/filter/FilterPlacesApiDto.java
- core/src/main/java/greencity/controller/PlaceController.java
🔇 Additional comments (6)
service/src/test/java/greencity/service/GoogleApiServiceTest.java (4)
6-17: LGTM! Well-organized imports.The new imports are properly organized and necessary for the Places API functionality.
119-147: LGTM! Well-structured geocoding test.The test effectively verifies the geocoding functionality for both Ukrainian and English locales, with proper mocking and assertions.
225-268: LGTM! Comprehensive null location handling test.The test properly verifies that a NotFoundException is thrown when location is null, and ensures no API calls are made.
270-311: LGTM! Robust error handling test.The test effectively verifies that the service gracefully handles API exceptions and continues processing.
Note: This test would also benefit from the helper method suggested earlier to reduce code duplication.
core/src/test/java/greencity/ModelUtils.java (1)
3-5: Well-structured imports!Good job using specific imports instead of wildcards, making the dependencies clear and maintainable.
Also applies to: 30-30, 39-39, 41-41
service/src/test/java/greencity/ModelUtils.java (1)
3337-3347: LGTM! Comprehensive test data for filter parameters.The test data covers all essential filter parameters including location, radius, keyword, ranking, price levels, and open status.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
service/src/test/java/greencity/ModelUtils.java (1)
Line range hint
1-3409: Fix formatting issues before merging.The CI pipeline indicates formatting issues. Please run the following command to fix the formatting:
mvn -f service net.revelc.code.formatter:formatter-maven-plugin:2.23.0:format🧰 Tools
🪛 GitHub Actions: CI/CD GreenCity
[error] File has not been previously formatted. Please format file using
mvn -f service net.revelc.code.formatter:formatter-maven-plugin:2.23.0:formatand commit before running validation!
♻️ Duplicate comments (2)
service/src/main/java/greencity/service/PlaceServiceImpl.java (2)
438-448:⚠️ Potential issueConsider using UUID for ID generation instead of timestamps.
Using
System.currentTimeMillis()for IDs can lead to:
- Duplicate IDs if multiple places are created simultaneously
- Predictable IDs that could be vulnerable to enumeration attacks
Apply this diff to use UUID instead:
- System.currentTimeMillis(), + UUID.randomUUID().toString(), - new LocationDto(System.currentTimeMillis(), el.geometry.location.lat, el.geometry.location.lng, + new LocationDto(UUID.randomUUID().toString(), el.geometry.location.lat, el.geometry.location.lng,
438-448:⚠️ Potential issueAdd error handling for Google Places API calls.
The external API call could fail due to network issues, rate limiting, or API errors. Consider adding proper exception handling.
Apply this diff to add error handling:
@Override public List<PlaceByBoundsDto> getPlacesByFilter(FilterPlacesApiDto filterDto, UserVO userVO) { + try { List<PlacesSearchResult> fromPlacesApi = googleApiService.getResultFromPlacesApi(filterDto, userVO); return fromPlacesApi.stream() .map(el -> new PlaceByBoundsDto( UUID.randomUUID().toString(), el.name, new LocationDto(UUID.randomUUID().toString(), el.geometry.location.lat, el.geometry.location.lng, el.vicinity))) .toList(); + } catch (Exception e) { + throw new ExternalServiceException("Failed to fetch places from Google API", e); + } }
🧹 Nitpick comments (1)
service/src/test/java/greencity/ModelUtils.java (1)
548-552: Consider using more realistic coordinates for test data.While hardcoded coordinates work for testing, using realistic coordinates (e.g., actual city coordinates) would make the test data more meaningful and help catch potential edge cases.
- .latitude(1d) - .longitude(1d) + .latitude(50.4501) // Kyiv coordinates + .longitude(30.5234)🧰 Tools
🪛 GitHub Actions: CI/CD GreenCity
[error] File has not been previously formatted. Please format file using
mvn -f service net.revelc.code.formatter:formatter-maven-plugin:2.23.0:formatand commit before running validation!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/src/main/java/greencity/config/SecurityConfig.java(1 hunks)core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java(3 hunks)core/src/main/resources/messages.properties(1 hunks)core/src/test/java/greencity/controller/PlaceControllerTest.java(3 hunks)service-api/pom.xml(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(3 hunks)service-api/src/main/java/greencity/service/PlaceService.java(4 hunks)service/src/main/java/greencity/service/LocationServiceImpl.java(1 hunks)service/src/main/java/greencity/service/PlaceServiceImpl.java(4 hunks)service/src/test/java/greencity/ModelUtils.java(4 hunks)service/src/test/java/greencity/service/PlaceServiceImplTest.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- service/src/main/java/greencity/service/LocationServiceImpl.java
- core/src/main/java/greencity/config/SecurityConfig.java
- service-api/pom.xml
- service-api/src/main/java/greencity/constant/ErrorMessage.java
- core/src/main/resources/messages.properties
- core/src/test/java/greencity/controller/PlaceControllerTest.java
- core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java
- service-api/src/main/java/greencity/service/PlaceService.java
🧰 Additional context used
🪛 GitHub Actions: CI/CD GreenCity
service/src/test/java/greencity/ModelUtils.java
[error] File has not been previously formatted. Please format file using mvn -f service net.revelc.code.formatter:formatter-maven-plugin:2.23.0:format and commit before running validation!
🔇 Additional comments (4)
service/src/test/java/greencity/ModelUtils.java (1)
3340-3408: Well-structured test data implementation for Google Places API!The implementation provides comprehensive test data with proper multilingual support and follows the Google Places API response format. The separation of English and Ukrainian test data facilitates thorough testing of localization features.
🧰 Tools
🪛 GitHub Actions: CI/CD GreenCity
[error] File has not been previously formatted. Please format file using
mvn -f service net.revelc.code.formatter:formatter-maven-plugin:2.23.0:formatand commit before running validation!service/src/main/java/greencity/service/PlaceServiceImpl.java (1)
559-568: Well-implemented duplicate place validation!The implementation correctly:
- Validates geocoding results
- Checks for existing locations using coordinates
- Uses appropriate exception handling
service/src/test/java/greencity/service/PlaceServiceImplTest.java (2)
674-691: Well-structured test for Google Places API integration!The test thoroughly verifies:
- Correct API call execution
- Result transformation
- Response mapping
804-826: Comprehensive test coverage for duplicate place validation!The test effectively:
- Verifies the duplicate check logic
- Ensures proper exception handling
- Confirms that save is not called for duplicates
|
Warded120
deleted the
7988-synchronize-places-from-database-with-google-places-api
branch
Issue Link: 7988
Changes:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Security