--- name: decebals/api-contract-review source: https://app.decimal.ai/s/decebals-api-contract-review@1/SKILL.md source_sha256: 374cecbf5c02 --- # 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 ```java // ❌ POST for retrieval @PostMapping("/users/search") public List searchUsers(@RequestBody SearchCriteria criteria) { } // ✅ GET with query params (or POST only if criteria is very complex) @GetMapping("/users") public List 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 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 ```java // ✅ 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 ```java // ❌ 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 ```java // ❌ Inconsistent responses @GetMapping("/users") public List 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> 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 ```java // ❌ No pagination on collections @GetMapping("/users") public List getAllUsers() { return userRepository.findAll(); // Could be millions } // ✅ Paginated @GetMapping("/users") public Page 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 ```java // ❌ NEVER DO THIS @GetMapping("/{id}") public ResponseEntity> 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 getUser(@PathVariable Long id) { return userService.findById(id) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); } ``` --- ## Error Response Format ### Consistent Error Structure ```java // ✅ 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 errors; // For validation errors } // In GlobalExceptionHandler @ExceptionHandler(ResourceNotFoundException.class) public ResponseEntity 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 ```java // ❌ Exposes stack trace @ExceptionHandler(Exception.class) public ResponseEntity handleAll(Exception ex) { return ResponseEntity.status(500) .body(ex.getStackTrace().toString()); // Security risk! } // ✅ Generic message, log details server-side @ExceptionHandler(Exception.class) public ResponseEntity 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 | |--------|-----------|-----------| | Remove endpoint | Yes | Deprecate first, remove in next version | | Remove field from response | Yes | Keep field, return null/default | | Add required field to request | Yes | Make optional with default | | Change field type | Yes | Add new field, deprecate old | | Rename field | Yes | Support both temporarily | | Change URL path | Yes | Redirect old to new | ### Non-Breaking Changes (Safe) - Add optional field to request - Add field to response - Add new endpoint - Add new optional query parameter ### Deprecation Pattern ```java @RestController @RequestMapping("/api/v1/users") public class UserControllerV1 { @Deprecated @GetMapping("/by-email") // Old endpoint public UserResponse getByEmailOld(@RequestParam String email) { return getByEmail(email); // Delegate to new } @GetMapping(params = "email") // New pattern public UserResponse getByEmail(@RequestParam String email) { return userService.findByEmail(email); } } ``` --- ## API Review Checklist ### 1. HTTP Semantics - [ ] GET for retrieval only (no side effects) - [ ] POST for creation (returns 201 + Location) - [ ] PUT for full replacement (idempotent) - [ ] PATCH for partial updates - [ ] DELETE for removal (idempotent) ### 2. URL Design - [ ] Versioned (`/v1/`, `/v2/`) - [ ] Nouns, not verbs (`/users`, not `/getUsers`) - [ ] Plural for collections (`/users`, not `/user`) - [ ] Hierarchical for relationships (`/users/{id}/orders`) - [ ] Consistent naming (kebab-case or camelCase, pick one) ### 3. Request Handling - [ ] Validation with `@Valid` - [ ] Clear error messages for validation failures - [ ] Request DTOs (not entities) - [ ] Reasonable size limits ### 4. Response Design - [ ] Response DTOs (not entities) - [ ] Consistent structure across endpoints - [ ] Pagination for collections - [ ] Proper status codes (not 200 for errors) ### 5. Error Handling - [ ] Consistent error format - [ ] Machine-readable error codes - [ ] Human-readable messages - [ ] No stack traces exposed - [ ] Proper 4xx vs 5xx distinction ### 6. Compatibility - [ ] No breaking changes in current version - [ ] Deprecated endpoints documented - [ ] Migration path for breaking changes --- ## Token Optimization For large APIs: 1. List all controllers: `find . -name "*Controller.java"` 2. Sample 2-3 controllers for pattern analysis 3. Check `@ExceptionHandler` configuration once 4. Grep for specific anti-patterns: ```bash # Find potential entity leaks grep -r "public.*Entity.*@GetMapping" --include="*.java" # Find 200 with error patterns grep -r "ResponseEntity.ok.*error" --include="*.java" # Find unversioned APIs grep -r "@RequestMapping.*api" --include="*.java" | grep -v "/v[0-9]" ```