Implement location service with API and Docker setup#1
Merged
Conversation
…ice, and repository layers
Reviewer's GuideImplements a new Spring Boot-based location-service module (with City and Airport domain models, REST controllers, services, repositories, mappers, DTOs, and MySQL configuration), wires it into the multi-module build, and adds Docker/MySQL setup plus documentation about MapStruct/Lombok usage and development tooling. Sequence diagram for creating a city via CityControllersequenceDiagram
actor Client
participant CityController
participant CityServiceImpl
participant CityRepository
participant CityMapper
participant Database
Client->>CityController: POST /api/v1/cities (CityRequest)
CityController->>CityServiceImpl: createCity(cityRequest)
CityServiceImpl->>CityRepository: existsByCityCodeIgnoreCase(cityRequest.cityCode)
CityRepository-->>CityServiceImpl: boolean exists
alt cityCode already exists
CityServiceImpl-->>CityController: throw IllegalArgumentException
CityController-->>Client: 400 error
else cityCode unique
CityServiceImpl->>CityMapper: toEntity(cityRequest)
CityMapper-->>CityServiceImpl: City
CityServiceImpl->>CityRepository: save(city)
CityRepository->>Database: insert City
Database-->>CityRepository: persisted City
CityRepository-->>CityServiceImpl: City
CityServiceImpl->>CityMapper: toDto(city)
CityMapper-->>CityServiceImpl: CityResponse
CityServiceImpl-->>CityController: CityResponse
CityController-->>Client: 201 ApiResponse\<CityResponse\>
end
Sequence diagram for creating an airport via AirportControllersequenceDiagram
actor Client
participant AirportController
participant AirportServiceimpl
participant CityRepository
participant AirportRepository
participant AirportMapper
participant Database
Client->>AirportController: POST /api/v1/airports (AirportRequest)
AirportController->>AirportServiceimpl: createAirport(request)
AirportServiceimpl->>CityRepository: findById(request.cityId)
CityRepository-->>AirportServiceimpl: Optional<City>
alt city not found
AirportServiceimpl-->>AirportController: throw IllegalArgumentException
AirportController-->>Client: 400 error
else city found
AirportServiceimpl->>AirportMapper: toEntity(request)
AirportMapper-->>AirportServiceimpl: Airport
AirportServiceimpl->>AirportServiceimpl: setCity(city)
AirportServiceimpl->>AirportRepository: save(airport)
AirportRepository->>Database: insert Airport
Database-->>AirportRepository: persisted Airport
AirportRepository-->>AirportServiceimpl: Airport
AirportServiceimpl->>AirportMapper: toDto(airport)
AirportMapper-->>AirportServiceimpl: AirportResponse
AirportServiceimpl-->>AirportController: AirportResponse
AirportController-->>Client: 201 ApiResponse<AirportResponse>
end
ER diagram for City and Airport tables in location-serviceerDiagram
CITY {
bigint id PK
varchar name
varchar country_code
varchar country
varchar city_code
varchar region_code
varchar time_zone_id
}
AIRPORT {
bigint id PK
char iata
varchar name
varchar time_zone_id
varchar street
varchar postal_code
double latitude
double longitude
bigint city_id FK
}
CITY ||--o{ AIRPORT : has
Class diagram for location-service domain, DTOs, services, and mappersclassDiagram
%% ===== Common-lib DTOs and value objects =====
class CityRequest {
+String name
+String countryCode
+String country
+String cityCode
+String regionCode
+String timeZoneOffset
}
class AirportRequest {
+String iata
+String name
+String timeZoneId
+Address address
+GeoCode geoCode
+Long cityId
}
class CityResponse {
+Long id
+String name
+String countryCode
+String country
+String cityCode
+String regionCode
+String timeZoneOffest
}
class AirportResponse {
+Long id
+String iata
+String name
+String timeZoneId
+Address address
+GeoCode geoCode
+CityResponse city
}
class ApiResponse~T~ {
+boolean success
+String message
+T data
+LocalDateTime timestamp
+success(data) ApiResponse~T~
+success(message, data) ApiResponse~T~
}
class Address {
+String street
+String postalCode
}
class GeoCode {
+Double latitude
+Double longitude
}
class CommonLibApplication
%% ===== JPA entities =====
class City {
+Long id
+String name
+String countryCode
+String country
+String cityCode
+String regionCode
+String timeZoneId
+List~Airport~ airports
}
class Airport {
+Long id
+String iata
+String name
+String timeZoneId
+Address address
+GeoCode geoCode
+City city
}
%% ===== Repositories =====
class CityRepository {
+Page~City~ findByCountryCodeIgnoreCase(countryCode, pageable)
+boolean existsByCityCodeIgnoreCase(cityCode)
+Page~City~ searchByKeyword(keyword, pageable)
}
class AirportRepository {
+boolean existsByIataIgnoreCase(iata)
+boolean existsByIataIgnoreCaseAndIdNot(iata, id)
+List~Airport~ findByCityId(cityId)
+boolean existsByCityId(cityId)
}
%% ===== Services =====
class CityService {
+CityResponse createCity(cityRequest)
+CityResponse updateCity(id, cityRequest)
+CityResponse getCityById(id)
+void deleteCity(id)
+Page~CityResponse~ getAllCities(pageable)
+Page~CityResponse~ searchCities(keyword, pageable)
+Page~CityResponse~ getCitiesByCountryCode(countryCode, pageable)
+boolean cityExists(cityCode)
}
class CityServiceImpl {
-CityRepository cityRepository
+CityResponse createCity(cityRequest)
+CityResponse updateCity(id, cityRequest)
+CityResponse getCityById(id)
+void deleteCity(id)
+Page~CityResponse~ getAllCities(pageable)
+Page~CityResponse~ searchCities(keyword, pageable)
+Page~CityResponse~ getCitiesByCountryCode(countryCode, pageable)
+boolean cityExists(cityCode)
}
class AirportService {
+AirportResponse createAirport(request)
+AirportResponse updateAirport(id, request)
+List~AirportResponse~ getAllAirports()
+AirportResponse getAirportById(id)
+void deleteAirport(id)
+List~AirportResponse~ getAirportsByCityId(cityId)
}
class AirportServiceimpl {
-AirportRepository airportRepository
-CityRepository cityRepository
-CityService cityService
+AirportResponse createAirport(request)
+AirportResponse updateAirport(id, request)
+List~AirportResponse~ getAllAirports()
+AirportResponse getAirportById(id)
+void deleteAirport(id)
+List~AirportResponse~ getAirportsByCityId(cityId)
}
%% ===== Mappers =====
class CityMapper {
+City toEntity(request)
+CityResponse toDto(city)
+void updateEntityFromRequest(request, city)
}
class AirportMapper {
-CityMapper cityMapper
+Airport toEntity(request)
+AirportResponse toDto(airport)
+void updateEntityFromRequest(request, airport)
}
%% ===== Controllers and application =====
class CityController {
-CityService cityService
+ResponseEntity createCity(cityRequest)
+ResponseEntity updateCity(id, cityRequest)
+ResponseEntity getCityById(id)
+ResponseEntity deleteCity(id)
+ResponseEntity getAllCities(pageable)
+ResponseEntity searchCities(keyword, pageable)
+ResponseEntity getCitiesByCountryCode(countryCode, pageable)
+ResponseEntity cityExists(cityCode)
}
class AirportController {
-AirportService airportService
+ResponseEntity createAirport(request)
+ResponseEntity updateAirport(id, request)
+ResponseEntity getAirportById(id)
+ResponseEntity getAllAirports()
+ResponseEntity deleteAirport(id)
+ResponseEntity getAirportsByCityId(cityId)
}
class HomeController {
+String HomeController()
}
class LocationServiceApplication {
+main(args)
}
%% ===== Relationships =====
City "1" --> "*" Airport : airports
Airport --> City : city
Airport --> Address : embedded
Airport --> GeoCode : embedded
AirportRequest --> Address : uses
AirportRequest --> GeoCode : uses
AirportResponse --> Address : uses
AirportResponse --> GeoCode : uses
AirportResponse --> CityResponse : uses
CityController --> CityService
AirportController --> AirportService
HomeController --> ApiResponse
CityServiceImpl ..|> CityService
AirportServiceimpl ..|> AirportService
CityServiceImpl --> CityRepository
AirportServiceimpl --> AirportRepository
AirportServiceimpl --> CityRepository
AirportServiceimpl --> CityService
CityRepository --> City
AirportRepository --> Airport
CityMapper --> City
CityMapper --> CityRequest
CityMapper --> CityResponse
AirportMapper --> Airport
AirportMapper --> AirportRequest
AirportMapper --> AirportResponse
AirportMapper --> CityMapper
LocationServiceApplication --> CityController
LocationServiceApplication --> AirportController
LocationServiceApplication --> HomeController
ApiResponse --> "T" CityResponse
ApiResponse --> "T" AirportResponse
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 17 issues, and left some high level feedback:
- AirportMapper is annotated as a Spring @component but only exposes static methods and never uses the injected CityMapper, which is both redundant and misleading; either remove @component and the field/constructor or refactor the methods to be instance-based and leverage the injected mapper.
- HomeController currently constructs ApiResponse via
new ApiResponse()even though ApiResponse is a Lombok @builder with final fields and no explicit constructor, which is unlikely to compile and also bypasses the provided factory methods; consider returningApiResponse.success("Welcome to Location Service API")(or similar) directly from the endpoint. - Service and repository naming is inconsistent with common conventions (e.g.,
AirportServiceimplinstead ofAirportServiceImpl, packageRepositorycapitalized, and service methods throwing bare IllegalArgumentException for not-found cases); aligning class/package names and using domain-specific exceptions mapped to proper HTTP status codes (e.g., 404) would make the API clearer and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- AirportMapper is annotated as a Spring @Component but only exposes static methods and never uses the injected CityMapper, which is both redundant and misleading; either remove @Component and the field/constructor or refactor the methods to be instance-based and leverage the injected mapper.
- HomeController currently constructs ApiResponse via `new ApiResponse()` even though ApiResponse is a Lombok @Builder with final fields and no explicit constructor, which is unlikely to compile and also bypasses the provided factory methods; consider returning `ApiResponse.success("Welcome to Location Service API")` (or similar) directly from the endpoint.
- Service and repository naming is inconsistent with common conventions (e.g., `AirportServiceimpl` instead of `AirportServiceImpl`, package `Repository` capitalized, and service methods throwing bare IllegalArgumentException for not-found cases); aligning class/package names and using domain-specific exceptions mapped to proper HTTP status codes (e.g., 404) would make the API clearer and easier to maintain.
## Individual Comments
### Comment 1
<location path="services/location-service/pom.xml" line_range="18-27" />
<code_context>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-starter-data-jpa</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-starter-webmvc</artifactId>
+ </dependency>
+
+ <dependency>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-devtools</artifactId>
+ <scope>runtime</scope>
+ <optional>true</optional>
+ </dependency>
+ <dependency>
+ <groupId>com.mysql</groupId>
+ <artifactId>mysql-connector-j</artifactId>
+ <scope>runtime</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.projectlombok</groupId>
+ <artifactId>lombok</artifactId>
+ <optional>true</optional>
+ </dependency>
+ <dependency>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-starter-data-jpa-test</artifactId>
+ <scope>test</scope>
+ </dependency>
</code_context>
<issue_to_address>
**issue (bug_risk):** The `spring-boot-starter-data-jpa-test` artifact does not exist and will cause build failures.
Spring Boot only provides `spring-boot-starter-test` for test support (including JPA-related tests), not `spring-boot-starter-data-jpa-test`. You should replace this dependency with `spring-boot-starter-test` (and add any extra test-scope dependencies you need), otherwise Maven will fail to resolve it.
</issue_to_address>
### Comment 2
<location path="services/location-service/src/main/java/com/flynest/location_service/service/impl/AirportServiceimpl.java" line_range="23" />
<code_context>
+@Service
+@RequiredArgsConstructor
+@Slf4j
+@Transactional(readOnly = true)
+public class AirportServiceimpl implements AirportService {
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Class-level `@Transactional(readOnly = true)` conflicts with write operations in this service.
Because of the class-level `@Transactional(readOnly = true)`, write methods like `createAirport`, `updateAirport`, and `deleteAirport` will execute in a read-only transaction, which may prevent changes from being persisted or cause vendor-specific issues. Consider removing the class-level `readOnly = true` and annotating only read methods, or explicitly overriding write methods with `@Transactional(readOnly = false)`.
</issue_to_address>
### Comment 3
<location path="services/location-service/src/main/java/com/flynest/location_service/service/impl/AirportServiceimpl.java" line_range="24" />
<code_context>
+@RequiredArgsConstructor
+@Slf4j
+@Transactional(readOnly = true)
+public class AirportServiceimpl implements AirportService {
+
+
</code_context>
<issue_to_address>
**nitpick (typo):** Class name `AirportServiceimpl` is likely a typo and inconsistent with Java naming conventions.
This mismatch may make it harder to find the implementation when searching the codebase. Please rename the class and file to `AirportServiceImpl` for consistency.
Suggested implementation:
```java
public class AirportServiceImpl implements AirportService {
```
1. Rename the file from:
`services/location-service/src/main/java/com/flynest/location_service/service/impl/AirportServiceimpl.java`
to:
`services/location-service/src/main/java/com/flynest/location_service/service/impl/AirportServiceImpl.java`
(e.g., using `git mv`).
2. Search the codebase for `AirportServiceimpl` (including configuration files, tests, and Spring component scans) and replace with `AirportServiceImpl` wherever it refers to this class/file.
</issue_to_address>
### Comment 4
<location path="services/location-service/src/main/java/com/flynest/location_service/controller/HomeController.java" line_range="14" />
<code_context>
+
+ @GetMapping("")
+ public String HomeController(){
+ ApiResponse response = new ApiResponse();
+ response.setMessage("Welcome to Location Service API");
+ return response.getMessage();
</code_context>
<issue_to_address>
**issue (bug_risk):** `ApiResponse` cannot be instantiated with `new` and the controller currently returns a plain String instead of the response wrapper.
Because `ApiResponse` uses `@Builder` and `final` fields, it has no no-args constructor, so `new ApiResponse()` will not compile. In addition, the method returns `String` while constructing an `ApiResponse`. Update the method to return something like `ResponseEntity<ApiResponse<String>>` and build the response via `ApiResponse.success("Welcome to Location Service API")`.
</issue_to_address>
### Comment 5
<location path="services/location-service/src/main/java/com/flynest/location_service/mapper/AirportMapper.java" line_range="9-12" />
<code_context>
+import lombok.RequiredArgsConstructor;
+import org.springframework.stereotype.Component;
+
+@Component
+public class AirportMapper {
+
+ private final CityMapper cityMapper;
+
+ public AirportMapper(CityMapper cityMapper) {
</code_context>
<issue_to_address>
**suggestion:** `AirportMapper` is a Spring component but exposes only static methods and the injected `CityMapper` is unused.
The class is declared as a Spring `@Component` with a `CityMapper` dependency, but all exposed methods are static and never use the injected instance. This inconsistency can confuse maintainers. Please either remove the `@Component` and the unused dependency, or convert the methods to instance methods that use the injected `CityMapper` where relevant.
Suggested implementation:
```java
import com.flynest.payload.response.AirportResponse;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
@Component
@RequiredArgsConstructor
public class AirportMapper {
private final CityMapper cityMapper;
// AirportRequest ──► Airport entity
public Airport toEntity(AirportRequest request) {
```
There are likely other static methods in `AirportMapper` (for mapping `Airport` to `AirportResponse`, etc.). To fully align with the suggestion:
1. Convert all remaining `public static` methods in this class to instance methods (`public ...`).
2. Where a `City` or city-related field is mapped, delegate to `cityMapper` (for example, `cityMapper.toResponse(airport.getCity())`) instead of manual mapping.
3. Update all usages of `AirportMapper` to obtain it via Spring injection (`@Autowired`/constructor injection) rather than calling static methods.
</issue_to_address>
### Comment 6
<location path="services/location-service/src/main/java/com/flynest/location_service/service/impl/CityServiceImpl.java" line_range="35-38" />
<code_context>
+ public CityResponse updateCity(Long id, CityRequest cityRequest) {
+ City city = cityRepository.findById(id)
+ .orElseThrow(() -> new IllegalArgumentException("City not found with id: " + id));
+// if (cityRequest.getCityCode() != null &&
+// !cityRequest.getCityCode().equalsIgnoreCase(city.getCityCode()) &&
+// cityRepository.existsByCityCodeIgnoreCase(cityRequest.getCityCode())) {
+// throw new IllegalArgumentException("City code already exists: " + cityRequest.getCityCode());
+// }
+ CityMapper.updateEntityFromRequest(cityRequest, city);
</code_context>
<issue_to_address>
**question (bug_risk):** Commented-out uniqueness check in `updateCity` may allow duplicate city codes.
Because `createCity` enforces `existsByCityCodeIgnoreCase` and `cityCode` is unique at the entity/DB level, updating a city without this check can lead to duplicate codes or DB constraint violations when the code changes. If uniqueness on update is still required, please restore this guard (or equivalent logic) rather than relying solely on the DB constraint.
</issue_to_address>
### Comment 7
<location path="services/location-service/src/main/java/com/flynest/location_service/mapper/CityMapper.java" line_range="32" />
<code_context>
+ .country(city.getCountry())
+ .cityCode(city.getCityCode())
+ .regionCode(city.getRegionCode())
+ .timeZoneOffest(city.getTimeZoneId())
+ .build();
+ }
</code_context>
<issue_to_address>
**suggestion (typo):** Typo in `timeZoneOffest` field name will propagate into the API contract.
`CityResponse` and the mapper both use the misspelled `timeZoneOffest`. If the API isn’t yet public, please rename this consistently to `timeZoneOffset` in all DTOs and mappers. If it’s already public, consider adding a correctly named field while still accepting the old one for backward compatibility.
Suggested implementation:
```java
.regionCode(city.getRegionCode())
.timeZoneOffset(city.getTimeZoneId())
```
To fully implement the suggestion you made in the review, you will also need to:
1. Update `CityResponse` in `services/location-service/src/main/java/com/flynest/location_service/dto/CityResponse.java` (or the equivalent package) to rename the field, getters/setters, and builder method from `timeZoneOffest` to `timeZoneOffset`.
2. If the API is already public and you need backward compatibility:
- Keep the old `timeZoneOffest` field annotated with `@JsonProperty("timeZoneOffest")`, mark it as `@Deprecated`, and/or ignore it for output.
- Add the new `timeZoneOffset` field with `@JsonProperty("timeZoneOffset")`.
- In the mapper, populate only `timeZoneOffset` for outgoing responses.
- For incoming requests, optionally accept both `timeZoneOffset` and `timeZoneOffest` if this DTO is also used as a request body model.
3. Update any tests, builders, or code constructing `CityResponse` directly to use `timeZoneOffset` instead of `timeZoneOffest`.
</issue_to_address>
### Comment 8
<location path="services/location-service/src/test/java/com/flynest/location_service/LocationServiceApplicationTests.java" line_range="6-7" />
<code_context>
+import org.junit.jupiter.api.Test;
+import org.springframework.boot.test.context.SpringBootTest;
+
+@SpringBootTest
+class LocationServiceApplicationTests {
+
+ @Test
</code_context>
<issue_to_address>
**issue (testing):** Add unit tests for CityServiceImpl and AirportServiceimpl to cover core domain logic and edge cases
Currently only a context-load smoke test exists, so none of the new service logic is exercised. Please add unit tests for `CityServiceImpl` and `AirportServiceimpl` with mocked repositories to validate both success and failure paths, for example:
- `CityServiceImpl.createCity`: unique city code succeeds; duplicate code (via `existsByCityCodeIgnoreCase`) throws.
- `CityServiceImpl.updateCity`: throws when `findById` is empty; correctly applies `CityMapper.updateEntityFromRequest` before saving.
- `AirportServiceimpl.createAirport`: throws when `cityRepository.findById` is empty; sets `city` on `Airport` before saving.
- `AirportServiceimpl.updateAirport`: throws when `airportRepository.findById` is empty; updates only non-null fields, including `city` when `cityId` is provided.
- `getAirportsByCityId` / `deleteAirport`: throw when `existsById` / `existsByCityId` preconditions fail.
This will ensure the service layer behavior is verified and guard against regressions if repository or mapper behavior changes.
</issue_to_address>
### Comment 9
<location path="services/location-service/src/test/java/com/flynest/location_service/LocationServiceApplicationTests.java" line_range="7-11" />
<code_context>
+import org.springframework.boot.test.context.SpringBootTest;
+
+@SpringBootTest
+class LocationServiceApplicationTests {
+
+ @Test
+ void contextLoads() {
+ }
+
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding simple mapper tests for CityMapper and AirportMapper to guard against mapping regressions
Because a lot of logic depends on `CityMapper` and `AirportMapper`, it would be good to add focused unit tests for them, for example:
- `CityMapper.toEntity` / `toDto`: verify all fields, including `timeZoneOffset` ↔ `timeZoneId`.
- `CityMapper.updateEntityFromRequest`: confirm only non-null request fields overwrite the entity.
- `AirportMapper.toEntity`: check `city` remains unset and other fields map correctly.
- `AirportMapper.toDto`: ensure nested `City` is mapped to `CityResponse` via `CityMapper.toDto`.
- `AirportMapper.updateEntityFromRequest`: validate only non-null fields update the entity and `city` is unchanged.
This will help catch subtle mapping regressions as DTOs/entities change.
Suggested implementation:
```java
package com.flynest.location_service;
import com.flynest.location_service.city.City;
import com.flynest.location_service.city.CityMapper;
import com.flynest.location_service.city.dto.CityRequest;
import com.flynest.location_service.city.dto.CityResponse;
import com.flynest.location_service.airport.Airport;
import com.flynest.location_service.airport.AirportMapper;
import com.flynest.location_service.airport.dto.AirportRequest;
import com.flynest.location_service.airport.dto.AirportResponse;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import static org.assertj.core.api.Assertions.assertThat;
@SpringBootTest
class LocationServiceApplicationTests {
@Autowired
private CityMapper cityMapper;
@Autowired
private AirportMapper airportMapper;
@Test
void contextLoads() {
}
@Test
void cityMapper_toEntity_and_toDto_shouldMapAllFields() {
// given
CityRequest request = new CityRequest();
request.setName("Berlin");
request.setCountryCode("DE");
// Adjust these field names/types to match your DTO
// e.g. request.setTimeZoneId("Europe/Berlin");
// request.setIataCode("BER");
// when
City entity = cityMapper.toEntity(request);
CityResponse response = cityMapper.toDto(entity);
// then
assertThat(response.getName()).isEqualTo(request.getName());
assertThat(response.getCountryCode()).isEqualTo(request.getCountryCode());
// For time zone mapping, assert the expected conversion between timeZoneId and timeZoneOffset here.
// Example (adjust to your actual model):
// assertThat(entity.getTimeZoneOffset()).isEqualTo(120);
// assertThat(response.getTimeZoneId()).isEqualTo("Europe/Berlin");
}
@Test
void cityMapper_updateEntityFromRequest_shouldUpdateOnlyNonNullFields() {
// given
City existing = new City();
existing.setName("Old Name");
existing.setCountryCode("DE");
// existing.setTimeZoneOffset(60);
CityRequest update = new CityRequest();
update.setName("New Name");
// leave countryCode and time zone null so they should not be overwritten
// when
cityMapper.updateEntityFromRequest(update, existing);
// then
assertThat(existing.getName()).isEqualTo("New Name");
assertThat(existing.getCountryCode()).isEqualTo("DE");
// assertThat(existing.getTimeZoneOffset()).isEqualTo(60);
}
@Test
void airportMapper_toEntity_shouldMapAllFields_andLeaveCityUnset() {
// given
AirportRequest request = new AirportRequest();
request.setCode("TXL");
request.setName("Tegel Airport");
request.setCountryCode("DE");
// request.setCityId(1L); // if such a field exists, entity.city should still remain null
// when
Airport entity = airportMapper.toEntity(request);
// then
assertThat(entity.getCode()).isEqualTo(request.getCode());
assertThat(entity.getName()).isEqualTo(request.getName());
assertThat(entity.getCountryCode()).isEqualTo(request.getCountryCode());
assertThat(entity.getCity()).isNull();
}
@Test
void airportMapper_toDto_shouldMapNestedCityUsingCityMapper() {
// given
City city = new City();
city.setName("Berlin");
city.setCountryCode("DE");
// city.setTimeZoneOffset(120);
Airport airport = new Airport();
airport.setCode("BER");
airport.setName("Berlin Brandenburg");
airport.setCountryCode("DE");
airport.setCity(city);
// when
AirportResponse response = airportMapper.toDto(airport);
// then
assertThat(response.getCode()).isEqualTo(airport.getCode());
assertThat(response.getName()).isEqualTo(airport.getName());
assertThat(response.getCountryCode()).isEqualTo(airport.getCountryCode());
assertThat(response.getCity()).isNotNull();
assertThat(response.getCity().getName()).isEqualTo(city.getName());
assertThat(response.getCity().getCountryCode()).isEqualTo(city.getCountryCode());
// Assert time zone mapping between City and CityResponse if applicable
}
@Test
void airportMapper_updateEntityFromRequest_shouldUpdateOnlyNonNullFields_andKeepCityUnchanged() {
// given
City city = new City();
city.setName("Berlin");
Airport existing = new Airport();
existing.setCode("OLD");
existing.setName("Old Airport");
existing.setCountryCode("DE");
existing.setCity(city);
AirportRequest update = new AirportRequest();
update.setCode("NEW");
// leave name and countryCode null so they should not be overwritten
// when
airportMapper.updateEntityFromRequest(update, existing);
// then
assertThat(existing.getCode()).isEqualTo("NEW");
assertThat(existing.getName()).isEqualTo("Old Airport");
assertThat(existing.getCountryCode()).isEqualTo("DE");
assertThat(existing.getCity()).isSameAs(city);
}
}
```
The above implementation assumes:
- Package names for `City`, `CityMapper`, `CityRequest`, `CityResponse`, `Airport`, `AirportMapper`, `AirportRequest`, and `AirportResponse`.
- Field names like `name`, `countryCode`, `code`, and time zone–related accessors.
You may need to:
1. Adjust the import statements to match the actual package structure of your entities, DTOs, and mappers.
2. Update field names and method calls in the tests (e.g. `getCode`, `getCountryCode`, time zone accessors) to match your real model.
3. Replace the commented time zone assertions with concrete expectations that align with your `timeZoneOffset` ↔ `timeZoneId` mapping logic.
</issue_to_address>
### Comment 10
<location path="services/location-service/readme.md" line_range="8" />
<code_context>
+- Airport
+ - IATA with country code and infos
+
+### Entities type
+1. Cities
+- id: string
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing "Entities type" to "Entity types" for correct grammar.
```suggestion
### Entity types
```
</issue_to_address>
### Comment 11
<location path="services/location-service/readme.md" line_range="26" />
<code_context>
+- geoCode --Embedded object
+ - latitude
+ - longitude
+- city [refreence of cities]
+ - one city have many airports @manytoone
+ - bidirection or unidirectional [each city airport hold reference of city but city hold reference of airport] ----Bidirectional
</code_context>
<issue_to_address>
**issue (typo):** Fix the spelling of "refreence" to "reference".
Change the bracket text to "[reference of cities]".
```suggestion
- city [reference of cities]
```
</issue_to_address>
### Comment 12
<location path="services/location-service/readme.md" line_range="31-33" />
<code_context>
+ - bidirection or unidirectional [each city airport hold reference of city but city hold reference of airport] ----Bidirectional
+
+
+### Redis Caching
+Cities and Airport data rarely change and its read very frequenctly
+- Cache annotion
+ - @Cacheable: to cache the result of a method call (airportByiata)
</code_context>
<issue_to_address>
**issue (typo):** Fix "its" and "frequenctly" in the Redis caching description.
Suggest: "Cities and airport data rarely change and it's read very frequently."
```suggestion
### Redis Caching
Cities and airport data rarely change and it's read very frequently
- Cache annotion
```
</issue_to_address>
### Comment 13
<location path="services/location-service/readme.md" line_range="33" />
<code_context>
+
+### Redis Caching
+Cities and Airport data rarely change and its read very frequenctly
+- Cache annotion
+ - @Cacheable: to cache the result of a method call (airportByiata)
+ - @CachePut: to update the cache without interfering with the method execution
</code_context>
<issue_to_address>
**issue (typo):** Correct the spelling of "annotion" to "annotation".
Change this bullet to “Cache annotation”.
```suggestion
- Cache annotation
```
</issue_to_address>
### Comment 14
<location path="services/location-service/readme.md" line_range="49" />
<code_context>
+ - POST: api/cities/bulk
+- Validation failure
+
+### Dto & Mappeer Pattern
+
+### Api EndPoints
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo "Mappeer" to "Mapper".
Also consider renaming this heading to “DTO & Mapper Pattern” to correct the spelling and capitalization.
```suggestion
### DTO & Mapper Pattern
```
</issue_to_address>
### Comment 15
<location path="services/location-service/readme.md" line_range="56" />
<code_context>
+base : api/cities
+1. Post : create
+2. Post : bulk create /bulk
+3. Get : get all citiest
+4. get : get by id {id}
+5. Put : update cities by id {id}
</code_context>
<issue_to_address>
**issue (typo):** Correct the typo "citiest" to "cities".
Change this phrase to “get all cities” to correct the spelling.
```suggestion
3. Get : get all cities
```
</issue_to_address>
### Comment 16
<location path="services/location-service/readme.md" line_range="61" />
<code_context>
+5. Put : update cities by id {id}
+6. delete : hard delete city by id {id}
+
+7. get : search by cititest /search
+#### Airports
+base :api/airport
</code_context>
<issue_to_address>
**question (typo):** Fix the typo "cititest" in this endpoint description.
Use "city" or "cities" instead of "cititest", e.g., "search by city/cities".
```suggestion
7. get : search by city /search
```
</issue_to_address>
### Comment 17
<location path="services/location-service/readme.md" line_range="64" />
<code_context>
+7. get : search by cititest /search
+#### Airports
+base :api/airport
+CURD operation [1 -6 ]
+7. get : get by iata code /iata/{iata}
+8. get : search by city id-code /city/{city}
</code_context>
<issue_to_address>
**suggestion (typo):** Correct "CURD operation" to the usual term "CRUD operation".
Use the standard CRUD acronym (Create, Read, Update, Delete) instead of CURD.
```suggestion
CRUD (Create, Read, Update, Delete) operations [1-6]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+18
to
+27
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-data-jpa</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-webmvc</artifactId> | ||
| </dependency> | ||
|
|
||
| <dependency> |
There was a problem hiding this comment.
issue (bug_risk): The spring-boot-starter-data-jpa-test artifact does not exist and will cause build failures.
Spring Boot only provides spring-boot-starter-test for test support (including JPA-related tests), not spring-boot-starter-data-jpa-test. You should replace this dependency with spring-boot-starter-test (and add any extra test-scope dependencies you need), otherwise Maven will fail to resolve it.
| @Service | ||
| @RequiredArgsConstructor | ||
| @Slf4j | ||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
issue (bug_risk): Class-level @Transactional(readOnly = true) conflicts with write operations in this service.
Because of the class-level @Transactional(readOnly = true), write methods like createAirport, updateAirport, and deleteAirport will execute in a read-only transaction, which may prevent changes from being persisted or cause vendor-specific issues. Consider removing the class-level readOnly = true and annotating only read methods, or explicitly overriding write methods with @Transactional(readOnly = false).
| @RequiredArgsConstructor | ||
| @Slf4j | ||
| @Transactional(readOnly = true) | ||
| public class AirportServiceimpl implements AirportService { |
There was a problem hiding this comment.
nitpick (typo): Class name AirportServiceimpl is likely a typo and inconsistent with Java naming conventions.
This mismatch may make it harder to find the implementation when searching the codebase. Please rename the class and file to AirportServiceImpl for consistency.
Suggested implementation:
public class AirportServiceImpl implements AirportService {-
Rename the file from:
services/location-service/src/main/java/com/flynest/location_service/service/impl/AirportServiceimpl.java
to:
services/location-service/src/main/java/com/flynest/location_service/service/impl/AirportServiceImpl.java
(e.g., usinggit mv). -
Search the codebase for
AirportServiceimpl(including configuration files, tests, and Spring component scans) and replace withAirportServiceImplwherever it refers to this class/file.
|
|
||
| @GetMapping("") | ||
| public String HomeController(){ | ||
| ApiResponse response = new ApiResponse(); |
There was a problem hiding this comment.
issue (bug_risk): ApiResponse cannot be instantiated with new and the controller currently returns a plain String instead of the response wrapper.
Because ApiResponse uses @Builder and final fields, it has no no-args constructor, so new ApiResponse() will not compile. In addition, the method returns String while constructing an ApiResponse. Update the method to return something like ResponseEntity<ApiResponse<String>> and build the response via ApiResponse.success("Welcome to Location Service API").
Comment on lines
+9
to
+12
| @Component | ||
| public class AirportMapper { | ||
|
|
||
| private final CityMapper cityMapper; |
There was a problem hiding this comment.
suggestion: AirportMapper is a Spring component but exposes only static methods and the injected CityMapper is unused.
The class is declared as a Spring @Component with a CityMapper dependency, but all exposed methods are static and never use the injected instance. This inconsistency can confuse maintainers. Please either remove the @Component and the unused dependency, or convert the methods to instance methods that use the injected CityMapper where relevant.
Suggested implementation:
import com.flynest.payload.response.AirportResponse;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
@Component
@RequiredArgsConstructor
public class AirportMapper {
private final CityMapper cityMapper;
// AirportRequest ──► Airport entity
public Airport toEntity(AirportRequest request) {There are likely other static methods in AirportMapper (for mapping Airport to AirportResponse, etc.). To fully align with the suggestion:
- Convert all remaining
public staticmethods in this class to instance methods (public ...). - Where a
Cityor city-related field is mapped, delegate tocityMapper(for example,cityMapper.toResponse(airport.getCity())) instead of manual mapping. - Update all usages of
AirportMapperto obtain it via Spring injection (@Autowired/constructor injection) rather than calling static methods.
|
|
||
| ### Redis Caching | ||
| Cities and Airport data rarely change and its read very frequenctly | ||
| - Cache annotion |
There was a problem hiding this comment.
issue (typo): Correct the spelling of "annotion" to "annotation".
Change this bullet to “Cache annotation”.
Suggested change
| - Cache annotion | |
| - Cache annotation |
| - POST: api/cities/bulk | ||
| - Validation failure | ||
|
|
||
| ### Dto & Mappeer Pattern |
There was a problem hiding this comment.
issue (typo): Fix the typo "Mappeer" to "Mapper".
Also consider renaming this heading to “DTO & Mapper Pattern” to correct the spelling and capitalization.
Suggested change
| ### Dto & Mappeer Pattern | |
| ### DTO & Mapper Pattern |
| base : api/cities | ||
| 1. Post : create | ||
| 2. Post : bulk create /bulk | ||
| 3. Get : get all citiest |
There was a problem hiding this comment.
issue (typo): Correct the typo "citiest" to "cities".
Change this phrase to “get all cities” to correct the spelling.
Suggested change
| 3. Get : get all citiest | |
| 3. Get : get all cities |
| 5. Put : update cities by id {id} | ||
| 6. delete : hard delete city by id {id} | ||
|
|
||
| 7. get : search by cititest /search |
There was a problem hiding this comment.
question (typo): Fix the typo "cititest" in this endpoint description.
Use "city" or "cities" instead of "cititest", e.g., "search by city/cities".
Suggested change
| 7. get : search by cititest /search | |
| 7. get : search by city /search |
| 7. get : search by cititest /search | ||
| #### Airports | ||
| base :api/airport | ||
| CURD operation [1 -6 ] |
There was a problem hiding this comment.
suggestion (typo): Correct "CURD operation" to the usual term "CRUD operation".
Use the standard CRUD acronym (Create, Read, Update, Delete) instead of CURD.
Suggested change
| CURD operation [1 -6 ] | |
| CRUD (Create, Read, Update, Delete) operations [1-6] |
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.
[Copilot is generating a summary..
This pull request primarily introduces and configures the City and Airport domain models, along with their associated controller, service, and repository layers for the location service. It also adds supporting request/response DTOs, embeddable value objects, and project configuration files for IDE and build tooling. Additionally, it updates the project documentation with lessons learned about using MapStruct and Lombok together.
Domain Model and DTO Additions:
AirportRequest,CityRequest, andAirportResponseDTOs to thecommon-libmodule for handling API requests and responses related to airports and cities. These classes include validation annotations and use Lombok for boilerplate reduction. [1] [2] [3]AddressandGeoCodein thecommon-libmodule to encapsulate address and geolocation data, with appropriate validation and Lombok annotations. [1] [2]IDE and Build Configuration:
.ideaconfiguration files to set up project encoding, annotation processor profiles (notably for Lombok and MapStruct), Maven modules, and workspace settings. This helps ensure consistent development environment setup for all contributors. [1] [2] [3] [4] [5] [6].idea/.gitignorefile, which previously excluded default IDE-generated files from version control.Project Structure and Documentation:
CommonLibApplicationentry point class in thecommon-libmodule.README.mdto document lessons learned about using MapStruct with Lombok, including the importance of annotation processor ordering and a sample mapper interface for cities.cloud/pom.xmlforservice-registry,config-server, andapi-gateway, possibly to temporarily disable their build..]Summary by Sourcery
Introduce a new Spring Boot-based location-service for managing cities and airports, wired into the multi-module build and backed by MySQL via Docker Compose.
New Features:
Enhancements:
Build:
Deployment:
Documentation:
Tests:
Chores: