Maintenance: NotificationController - Add comprehensive integration tests

[claude]

Summary

Added comprehensive integration test coverage for NotificationController to verify REST API endpoint behavior, validation, and error handling.

Area Inspected

• NotificationController (src/main/java/spring/memewikibe/api/controller/notification/NotificationController.java)
• Related components: NotificationTokenRegister, NotificationCreateTokenRequest, NotificationToken

Issues Found

Missing Test Coverage: The NotificationController had no integration tests despite being a public REST API endpoint. While the service layer (NotificationTokenRegister) had comprehensive tests, there was no verification of:

• HTTP request/response handling
• Validation integration with controller layer
• Error response formatting through ControllerAdvice
• Edge cases at the API boundary

Changes Made

Created NotificationControllerTest with 9 test cases covering:

Happy Path

• ✅ Successful token registration (201 CREATED)
• ✅ Duplicate token handling (idempotent behavior)
• ✅ Long tokens (200+ characters for FCM tokens)
• ✅ Special characters in tokens

Validation & Error Handling

• ✅ Blank token validation (400 BAD REQUEST)
• ✅ Whitespace-only token validation
• ✅ Null token validation
• ✅ Malformed JSON handling
• ✅ Missing request body handling

All tests use @IntegrationTest with @AutoConfigureMockMvc to verify the complete request/response cycle including Spring's validation framework and custom error handling.

Why This Improves the Code

1. Complete Coverage: Controllers are the API contract - they need dedicated tests to verify HTTP-level behavior
2. Regression Prevention: Ensures validation rules and error responses remain consistent
3. Documentation: Tests serve as living documentation of expected API behavior
4. Confidence: Verifies the full integration stack works correctly together

Test Results

All tests pass:

./gradlew test --tests "NotificationControllerTest"
BUILD SUCCESSFUL

🤖 Generated with Claude Code

[claude]

PR Review: NotificationController Integration Tests

Overview

This PR adds comprehensive integration test coverage for the NotificationController. The tests are well-structured and cover important scenarios. However, there are several issues and potential improvements to address.

---

Critical Issues

1. Test Isolation & Race Condition Risk

Location: NotificationControllerTest.java:54-69

The test 동일한_토큰을_여러번_등록해도_성공한다() has a race condition vulnerability:

// verify - should only have one token
then(tokenRepository.findAll()).hasSize(1);

Problem: The verification happens outside the transaction boundary. Between the second POST request completing and the findAll() call, another test could potentially start and clean up the database, or the update timestamp could change.

Impact: This could lead to flaky tests in CI/CD environments.

Recommendation: Move the verification inside a transactional context or use @Sql annotations to ensure proper test isolation.

---

2. Potential Database Column Length Constraint Missing

Location: NotificationControllerTest.java:139-153

The test verifies that 200-character tokens work:

String longToken = "a".repeat(200);

Problem: The NotificationToken entity doesn't explicitly define a column length constraint:

@Id
private String token;  // No @Column(length = ...) specified

Security/Data Integrity Risk:

• Without explicit length constraints, behavior is database-dependent
• Could allow extremely long tokens (potential DoS vector)
• May cause silent truncation in some databases

Recommendation: Add explicit column constraints:

@Id
@Column(length = 255) // or appropriate length based on FCM token max length
private String token;

---

3. Missing Token Format Validation

Location: Controller validation only checks @NotBlank

Problem: The current validation accepts any non-blank string, including:

• Tokens with only whitespace after trimming
• Potentially malicious strings (SQL injection attempts via token)
• Invalid FCM token formats

Security Risk: While the token is used as a primary key (reducing SQL injection risk), there's no validation that the token is actually a valid FCM token format.

Recommendation: Add format validation:

public record NotificationCreateTokenRequest(
    @NotBlank(message = "Token is required")
    @Pattern(regexp = "^[A-Za-z0-9:_.-]+$", message = "Invalid token format")
    String token
) {}

---

Code Quality Issues

4. Inconsistent Test Naming Between Layers

Location: Service tests vs Controller tests

The service layer test (NotificationTokenRegisterTest.java:78) has a comment:

// Firebase Cloud Messaging tokens are typically 152+ characters

But the controller test (NotificationControllerTest.java:141) says:

// Firebase Cloud Messaging tokens can be 152+ characters

Issue: Inconsistent documentation ("typically" vs "can be") and hardcoded 200-character test value without justification.

Recommendation:

• Research actual FCM token max length (appears to be ~152-163 characters)
• Use realistic test data (e.g., 163 characters instead of arbitrary 200)
• Document the source of this information

---

5. Duplicate Test Coverage

Location: NotificationControllerTest.java duplicates service layer tests

Several tests duplicate service layer coverage:

• 매우_긴_토큰도_성공적으로_등록된다() (also in service test)
• 특수문자가_포함된_토큰도_성공적으로_등록된다() (also in service test)
• 동일한_토큰을_여러번_등록해도_성공한다() (also in service test)

Issue: These tests don't add value at the controller layer since they're testing business logic already covered by service tests.

Recommendation: Controller tests should focus on:

• HTTP-specific concerns (status codes, headers, content negotiation)
• Request/response serialization
• Validation integration
• Error response formatting

Remove tests that purely duplicate service layer behavior.

---

6. Missing Edge Cases

6.1 Content-Type Validation Missing

Location: All POST requests assume application/json

Missing test: What happens if someone sends text/plain or omits Content-Type?

Recommendation: Add test:

@Test
void 잘못된_Content_Type으로_요청_시_415_에러를_반환한다() throws Exception {
    mockMvc.perform(post("/api/notifications/register")
            .contentType(MediaType.TEXT_PLAIN)
            .content("plain text"))
        .andExpect(status().isUnsupportedMediaType());
}

6.2 Token Trimming Behavior Unclear

Location: NotificationCreateTokenRequest

Question: Should " token " (with surrounding whitespace) be treated as "token"?

Currently @NotBlank allows this, but it's unclear if trimming is desired. The domain model doesn't trim:

public static NotificationToken create(String token) {
    return NotificationToken.builder()
        .token(token)  // No trimming
        .build();
}

Recommendation: Either:

1. Add explicit trim in the request record, or
2. Add a test documenting that whitespace is preserved (and verify this is intended)

---

Performance Considerations

7. Potential N+1 Query Issue (Future Risk)

Location: NotificationTokenRegister.java:18

if (!notificationTokenRepository.existsById(token)) {
    notificationTokenRepository.save(NotificationToken.create(token));
}

Issue: This performs two database operations for new tokens:

1. existsById() - SELECT query
2. save() - INSERT query

Better approach: Use database-level upsert or handle the duplicate key exception:

@Transactional
public void registerToken(@NonNull final String token) {
    try {
        notificationTokenRepository.save(NotificationToken.create(token));
    } catch (DataIntegrityViolationException e) {
        // Token already exists - this is fine
        log.debug("Token already registered: {}", token);
    }
}

This reduces database round-trips by 50% for new tokens.

---

8. Missing Test Cleanup Verification

Location: @AfterEach cleanup

@AfterEach
void tearDown() {
    tokenRepository.deleteAllInBatch();
}

Issue: No verification that cleanup actually worked. If cleanup fails, subsequent tests could be affected.

Recommendation: Add assertion or use @DirtiesContext for critical tests.

---

Security Concerns

9. No Rate Limiting Tests

Location: Missing entirely

Security Risk: The endpoint has no apparent rate limiting. An attacker could:

• Spam the database with millions of fake tokens
• Cause database bloat
• Execute a DoS attack

Recommendation:

1. Add rate limiting (e.g., using Bucket4j or Spring's rate limiting)
2. Add tests to verify rate limiting works:

@Test
void 동일_IP에서_과도한_요청_시_429_에러를_반환한다() throws Exception {
    // Test rate limiting
}

---

10. Missing Authorization/Authentication

Location: NotificationController has no security annotations

Security Risk: The endpoint is completely public. Anyone can:

• Register tokens for any user
• Potentially pollute the notification token database
• Register tokens they don't own

Recommendation: Consider if this endpoint should:

• Require authentication
• Associate tokens with user IDs
• Validate token ownership

If intentionally public, add a comment explaining why.

---

Testing Best Practices

11. Magic Numbers in Tests

Location: Multiple tests use hardcoded strings

String token = "valid-fcm-token-123";
String token = "duplicate-token";

Issue: Test data lacks consistency and doesn't represent realistic FCM tokens.

Recommendation: Create test data constants or fixtures:

private static final String VALID_FCM_TOKEN = 
    "eGhb...YyMzQ1Njc4OTA"; // Realistic FCM token format

private static final String REALISTIC_LONG_TOKEN = 
    "a".repeat(163); // Actual FCM max length

---

12. Assertion Clarity

Location: NotificationControllerTest.java:88

.andExpect(jsonPath("$.error.data.token").value("Token is required"));

Issue: This hardcodes the error message from the annotation. If the annotation message changes, the test breaks.

Recommendation: Either:

1. Extract error messages to constants, or
2. Just verify that the field token has some error message:

.andExpect(jsonPath("$.error.data.token").exists())
.andExpect(jsonPath("$.error.data.token").isNotEmpty());

---

Summary

Strengths

• Good test coverage of validation scenarios
• Proper use of integration tests with real Spring context
• Well-structured given/when/then pattern
• Korean test names follow project conventions

Must Fix

1. Add explicit column length constraint on NotificationToken.token
2. Fix potential race condition in duplicate token test
3. Add token format validation
4. Consider authentication/authorization requirements

Should Fix

1. Remove duplicate test coverage (covered by service layer)
2. Add Content-Type validation test
3. Clarify token trimming behavior
4. Optimize database queries (use upsert pattern)
5. Add rate limiting consideration

Nice to Have

1. Use realistic FCM token test data
2. Extract magic strings to constants
3. Add rate limiting tests
4. Document why endpoint is public (if intentional)

---

Test Coverage Score: 7/10

The tests cover the happy path and basic validation well, but miss important edge cases, security concerns, and performance optimizations.

Overall Recommendation: Request changes before merging. The critical issues (column constraints, format validation, security) should be addressed.