api-contract-review

API Contract Review Skill

Audit REST API design for correctness, consistency, and compatibility.

When to Use

• User asks "review this API" / "check REST endpoints"
• Before releasing API changes
• Reviewing PR with controller changes
• Checking backward compatibility

Quick Reference: Common Issues

| Issue | Symptom | Impact | |-------|---------|--------| | Wrong HTTP verb | POST for idempotent operation | Confusion, caching issues | | Missing versioning | /users instead of /v1/users | Breaking changes affect all clients | | Entity leak | JPA entity in response | Exposes internals, N+1 risk | | 200 with error | {"status": 200, "error": "..."} | Breaks error handling | | Inconsistent naming | /getUsers vs /users | Hard to learn API |

HTTP Verb Semantics

Verb Selection Guide

| Verb | Use For | Idempotent | Safe | Request Body | |------|---------|------------|------|--------------| | GET | Retrieve resource | Yes | Yes | No | | POST | Create new resource | No | No | Yes | | PUT | Replace entire resource | Yes | No | Yes | | PATCH | Partial update | No* | No | Yes | | DELETE | Remove resource | Yes | No | Optional |

*PATCH can be idempotent depending on implementation

Common Mistakes

// ❌ POST for retrieval
@PostMapping("/users/search")
public List<User> searchUsers(@RequestBody SearchCriteria criteria) { }

// ✅ GET with query params (or POST only if criteria is very complex)
@GetMapping("/users")
public List<User> searchUsers(
    @RequestParam String name,
    @RequestParam(required = false) String email) { }

// ❌ GET for state change
@GetMapping("/users/{id}/activate")
public void activateUser(@PathVariable Long id) { }

// ✅ POST or PATCH for state change
@PostMapping("/users/{id}/activate")
public ResponseEntity<Void> activateUser(@PathVariable Long id) { }

// ❌ POST for idempotent update
@PostMapping("/users/{id}")
public User updateUser(@PathVariable Long id, @RequestBody UserDto dto) { }

// ✅ PUT for full replacement, PATCH for partial
@PutMapping("/users/{id}")
public User replaceUser(@PathVariable Long id, @RequestBody UserDto dto) { }

@PatchMapping("/users/{id}")
public User updateUser(@PathVariable Long id, @RequestBody UserPatchDto dto) { }

API Versioning

Strategies

| Strategy | Example | Pros | Cons | |----------|---------|------|------| | URL path | /v1/users | Clear, easy routing | URL changes | | Header | Accept: application/vnd.api.v1+json | Clean URLs | Hidden, harder to test | | Query param | /users?version=1 | Easy to add | Easy to forget |

Recommended: URL Path

// ✅ Versioned endpoints
@RestController
@RequestMapping("/api/v1/users")
public class UserControllerV1 { }

@RestController
@RequestMapping("/api/v2/users")
public class UserControllerV2 { }

// ❌ No versioning
@RestController
@RequestMapping("/api/users")  // Breaking changes affect everyone
public class UserController { }

Version Checklist

• [ ] All public APIs have version in path
• [ ] Internal APIs documented as internal (or versioned too)
• [ ] Deprecation strategy defined for old versions

Request/Response Design

DTO vs Entity

// ❌ Entity in response (leaks internals)
@GetMapping("/{id}")
public User getUser(@PathVariable Long id) {
    return userRepository.findById(id).orElseThrow();
    // Exposes: password hash, internal IDs, lazy collections
}

// ✅ DTO response
@GetMapping("/{id}")
public UserResponse getUser(@PathVariable Long id) {
    User user = userService.findById(id);
    return UserResponse.from(user);  // Only public fields
}

Response Consistency

// ❌ Inconsistent responses
@GetMapping("/users")
public List<User> getUsers() { }  // Returns array

@GetMapping("/users/{id}")
public User getUser(@PathVariable Long id) { }  // Returns object

@GetMapping("/users/count")
public int countUsers() { }  // Returns primitive

// ✅ Consistent wrapper (optional but recommended for large APIs)
@GetMapping("/users")
public ApiResponse<List<UserResponse>> getUsers() {
    return ApiResponse.success(userService.findAll());
}

// Or at minimum, consistent structure:
// - Collections: always wrapped or always raw (pick one)
// - Single items: always object
// - Counts/stats: always object { "count": 42 }

Pagination

// ❌ No pagination on collections
@GetMapping("/users")
public List<User> getAllUsers() {
    return userRepository.findAll();  // Could be millions
}

// ✅ Paginated
@GetMapping("/users")
public Page<UserResponse> getUsers(
    @RequestParam(defaultValue = "0") int page,
    @RequestParam(defaultValue = "20") int size) {
    return userService.findAll(PageRequest.of(page, size));
}

HTTP Status Codes

Success Codes

| Code | When to Use | Response Body | |------|-------------|---------------| | 200 OK | Successful GET, PUT, PATCH | Resource or result | | 201 Created | Successful POST (created) | Created resource + Location header | | 204 No Content | Successful DELETE, or PUT with no body | Empty |

Error Codes

| Code | When to Use | Common Mistake | |------|-------------|----------------| | 400 Bad Request | Invalid input, validation failed | Using for "not found" | | 401 Unauthorized | Not authenticated | Confusing with 403 | | 403 Forbidden | Authenticated but not allowed | Using 401 instead | | 404 Not Found | Resource doesn't exist | Using 400 | | 409 Conflict | Duplicate, concurrent modification | Using 400 | | 422 Unprocessable | Semantic error (valid syntax, invalid meaning) | Using 400 | | 500 Internal Error | Unexpected server error | Exposing stack traces |

Anti-Pattern: 200 with Error Body

// ❌ NEVER DO THIS
@GetMapping("/{id}")
public ResponseEntity<Map<String, Object>> getUser(@PathVariable Long id) {
    try {
        User user = userService.findById(id);
        return ResponseEntity.ok(Map.of("status", "success", "data", user));
    } catch (NotFoundException e) {
        return ResponseEntity.ok(Map.of(  // Still 200!
            "status", "error",
            "message", "User not found"
        ));
    }
}

// ✅ Use proper status codes
@GetMapping("/{id}")
public ResponseEntity<UserResponse> getUser(@PathVariable Long id) {
    return userService.findById(id)
        .map(ResponseEntity::ok)
        .orElse(ResponseEntity.notFound().build());
}

Error Response Format

Consistent Error Structure

// ✅ Standard error response
public class ErrorResponse {
    private String code;        // Machine-readable: "USER_NOT_FOUND"
    private String message;     // Human-readable: "User with ID 123 not found"
    private Instant timestamp;
    private String path;
    private List<FieldError> errors;  // For validation errors
}

// In GlobalExceptionHandler
@ExceptionHandler(ResourceNotFoundException.class)
public ResponseEntity<ErrorResponse> handleNotFound(
        ResourceNotFoundException ex, HttpServletRequest request) {
    return ResponseEntity.status(HttpStatus.NOT_FOUND)
        .body(ErrorResponse.builder()
            .code("RESOURCE_NOT_FOUND")
            .message(ex.getMessage())
            .timestamp(Instant.now())
            .path(request.getRequestURI())
            .build());
}

Security: Don't Expose Internals

// ❌ Exposes stack trace
@ExceptionHandler(Exception.class)
public ResponseEntity<String> handleAll(Exception ex) {
    return ResponseEntity.status(500)
        .body(ex.getStackTrace().toString());  // Security risk!
}

// ✅ Generic message, log details server-side
@ExceptionHandler(Exception.class)
public ResponseEntity<ErrorResponse> handleAll(Exception ex) {
    log.error("Unexpected error", ex);  // Full details in logs
    return ResponseEntity.status(500)
        .body(ErrorResponse.of("INTERNAL_ERROR", "An unexpected error occurred"));
}

Backward Compatibility

Breaking Changes (Avoid in Same Version)

| Change | Breaking? | Migration | |--------|-----------