Skip to content

Comments

Phase 1: Extract and Strengthen API Contracts for FTGO Monolith Decoupling#41

Open
devin-ai-integration[bot] wants to merge 2 commits intomasterfrom
devin/1771506616-phase1-api-contracts
Open

Phase 1: Extract and Strengthen API Contracts for FTGO Monolith Decoupling#41
devin-ai-integration[bot] wants to merge 2 commits intomasterfrom
devin/1771506616-phase1-api-contracts

Conversation

@devin-ai-integration
Copy link

@devin-ai-integration devin-ai-integration bot commented Feb 19, 2026

Phase 1: Extract and Strengthen API Contracts for FTGO Monolith Decoupling

Summary

Strengthens API contracts across all four FTGO services in preparation for future microservice extraction. The changes fall into four categories:

  1. DTO extraction to API modules — Moved response DTOs (GetOrderResponse, GetConsumerResponse, GetRestaurantResponse, CreateRestaurantResponse) out of implementation modules into their corresponding -api modules so downstream consumers can depend on the API module alone.

  2. Swagger/OpenAPI annotations — Added @Api, @ApiOperation, @ApiResponses, @ApiParam to all four controllers and @ApiModel/@ApiModelProperty to all DTOs. Added springfox-swagger2:2.8.0 dependency to all four -api module build.gradle files.

  3. URL path versioning — All controller base paths changed from /{resource} to /api/v1/{resource} (e.g. /orders/api/v1/orders). This is a breaking change for any existing clients.

  4. Cross-service dependency documentation — New CROSS-SERVICE-DEPENDENCIES.md mapping direct repository/service calls that must become HTTP calls in Phase 2.

New DTOs created: GetOrderResponse (with nested CourierActionDTO), GetConsumerResponse, ValidateOrderForConsumerRequest, GetCourierResponse, GetRestaurantResponse (in both order-api and restaurant-api), CreateRestaurantResponse (in restaurant-api).

Updates since last revision

  • Fixed NullPointerException in CourierController.get(): The Courier entity's available field is Boolean (wrapper type) and is never initialized in the constructor. The previous code called courier.isAvailable() which auto-unboxes to primitive boolean, throwing NPE when available is null. Fixed by using Boolean.TRUE.equals(courier.getAvailable()).
  • Added getters to Courier entity: Added getName(), getAddress(), and getAvailable() methods to support safe field access in the controller.
  • Populated courier name/address: GetCourierResponse now includes the courier's name and address instead of passing null.

Local Verification

The application was started locally via Docker Compose and verified:

  • All four /api/v1/ endpoint groups route correctly (orders, consumers, restaurants, couriers)
  • Swagger UI loads at /swagger-ui.html with all four service tags and annotated endpoints visible
  • API docs at /v2/api-docs return correct OpenAPI JSON with all @ApiModel/@ApiModelProperty metadata
  • Unit tests pass (./gradlew test for modified modules)

Swagger UI — all services documented with /api/v1/ paths:

Swagger UI overview

Swagger UI expanded endpoints

Review & Testing Checklist for Human

  • Verify URL path changes don't break existing clients — All endpoints now use /api/v1/ prefix. Only OrderControllerTest was updated to use the new paths; other controller tests (if any exist beyond this repo) may still reference old paths. Check for any integration tests, E2E tests, or external clients that depend on the old paths.
  • Verify DTO field type changes are safeGetOrderResponse.orderTotal changed from Money object to String (via .asString()). GetRestaurantResponse.id changed from Long (boxed) to long (primitive). Confirm these don't cause NPEs or serialization issues in production.
  • Check duplicate GetRestaurantResponse classes — There's now a GetRestaurantResponse in both ftgo-order-service-api (net.chrisrichardson.ftgo.orderservice.api.web) and ftgo-restaurant-service-api (net.chrisrichardson.ftgo.restaurantservice.events). Verify this is intentional and doesn't cause confusion or import conflicts.
  • Verify Courier entity changes don't affect other code — Added getName(), getAddress(), getAvailable() getters to the Courier JPA entity. While the entity uses @Access(AccessType.FIELD) for Hibernate, Jackson may now serialize these fields if the entity is ever returned directly elsewhere in the codebase. Verify no other code paths return Courier entities directly.
  • Test all four services' endpoints — Local testing showed all endpoints route correctly but return 500 errors due to pre-existing DB schema issues (SQL grammar exceptions). Verify endpoints work in a properly configured environment with correct schema.
  • Review Snyk security failures — CI check security/snyk failed with 4 tests. These appear to be pre-existing dependency vulnerabilities (not introduced by this PR), but should be reviewed at Snyk dashboard.

Test Plan

  1. Start the FTGO application with proper MySQL schema
  2. Test each service's endpoints using the new /api/v1/ paths:
    • POST /api/v1/consumers → create consumer
    • GET /api/v1/consumers/{consumerId} → retrieve consumer
    • POST /api/v1/restaurants → create restaurant
    • GET /api/v1/restaurants/{restaurantId} → retrieve restaurant
    • POST /api/v1/couriers → create courier
    • GET /api/v1/couriers/{courierId} → retrieve courier (verify name, address, and availability are populated)
    • POST /api/v1/orders → create order
    • GET /api/v1/orders/{orderId} → retrieve order
  3. Verify Swagger UI at /swagger-ui.html shows all endpoints with proper documentation
  4. Run full test suite: ./gradlew test -x :ftgo-end-to-end-tests-common:test -x :ftgo-end-to-end-tests:test

Notes

  • The CROSS-SERVICE-DEPENDENCIES.md doc references some DTOs (CourierAvailability, RestaurantMenuDTO, MenuItemDTO) that don't exist yet — these are placeholders for Phase 2.
  • The build succeeded for all modified modules, but the full build fails due to a pre-existing dependency issue in ftgo-end-to-end-tests-common (missing eventuate-util-test:0.1.0.RELEASE).
  • Adding springfox-swagger2 to API modules increases their dependency footprint, but Spring dependencies are excluded to keep them lightweight.
  • Local testing revealed pre-existing DB schema issues (SQL grammar exceptions) that prevent successful data operations. These are not introduced by this PR but should be addressed separately.

Link to Devin run: https://app.devin.ai/sessions/2d5c7c1499c547859343dbe7a7bbf8bc
Requested by: @cogjack

…pling

- Create missing request/response DTOs in API modules:
  - Order API: GetOrderResponse, GetRestaurantResponse (moved from impl)
  - Consumer API: GetConsumerResponse, ValidateOrderForConsumerRequest
  - Restaurant API: CreateRestaurantResponse, GetRestaurantResponse (moved from impl)
  - Courier API: GetCourierResponse

- Add OpenAPI/Swagger annotations (@Api, @apioperation, @apiresponse,
  @apimodel, @ApiModelProperty) to all controllers and DTOs

- Implement API versioning with /api/v1 URL path prefix across all services

- Document cross-service dependencies in CROSS-SERVICE-DEPENDENCIES.md

- Remove old impl-level DTOs, ensuring API modules contain only DTOs

Co-Authored-By: Jack  Meigel <jack.meigel@cognition.ai>
@devin-ai-integration
Copy link
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Author

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment on lines 59 to 64
GetCourierResponse response = new GetCourierResponse(
courier.getId(),
null,
null,
courier.isAvailable()
);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 NullPointerException when getting courier with unset available field due to Boolean-to-boolean unboxing

The CourierController.get() method calls courier.isAvailable() which auto-unboxes the Boolean available field to a primitive boolean. However, the Courier entity's available field is declared as Boolean (wrapper type) at ftgo-domain/src/main/java/net/chrisrichardson/ftgo/domain/Courier.java:28 and is never initialized in the constructor (Courier(PersonName, Address) at line 33-36). For any newly created courier that hasn't had noteAvailable() or noteUnavailable() called, available will be null, and the auto-unboxing in isAvailable() (ftgo-domain/src/main/java/net/chrisrichardson/ftgo/domain/Courier.java:51-52) will throw a NullPointerException.

Root Cause

The previous code at this endpoint returned the Courier entity directly as JSON (return new ResponseEntity<>(courier, HttpStatus.OK)), which serialized available as JSON null without issue. The new code explicitly calls courier.isAvailable() to pass the value to GetCourierResponse, triggering Java's auto-unboxing of Booleanboolean.

The Courier constructor at ftgo-domain/src/main/java/net/chrisrichardson/ftgo/domain/Courier.java:33:

public Courier(PersonName name, Address address) {
    this.name = name;
    this.address = address;
    // available is never set, remains null
}

isAvailable() at line 51:

public boolean isAvailable() {
    return available; // NullPointerException when available is null
}

Impact: Any GET request for a courier that was created but never had its availability explicitly set will result in a 500 Internal Server Error.

Suggested change
GetCourierResponse response = new GetCourierResponse(
courier.getId(),
null,
null,
courier.isAvailable()
);
GetCourierResponse response = new GetCourierResponse(
courier.getId(),
null,
null,
courier.isAvailable() != null && courier.isAvailable()
);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed in 988b251. The Courier entity's available field is Boolean (wrapper) and never initialized in the constructor, so isAvailable() would NPE on auto-unboxing.

Fix:

  • Added getName(), getAddress(), and getAvailable() getters to the Courier entity
  • Used Boolean.TRUE.equals(courier.getAvailable()) for null-safe availability check (defaults to false when null)
  • Now also populates name and address in the GetCourierResponse instead of passing null

- Add getName(), getAddress(), getAvailable() getters to Courier entity
- Use Boolean.TRUE.equals() for null-safe availability check
- Populate name and address fields in GetCourierResponse

Co-Authored-By: Jack  Meigel <jack.meigel@cognition.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant