Add ReuseResponses feature to Reflector for OpenAPI 3.0 and 3.1

[luke-hagar-sp]

This update introduces the ReuseResponses boolean flag in the Reflector struct, allowing common response definitions to be moved to components/responses.

The new methods generateResponseName and collectResponse facilitate the generation of consistent response names and the storage of responses in components when ReuseResponses is enabled. The setupResponse method has been updated to utilize these new functionalities, improving response management.

This allows operations response sections with repeated responses to be collapsed down to something like this:

responses:
  "400":
    $ref: '#/components/responses/BadRequest'
  "401":
    $ref: '#/components/responses/Unauthorized'
  "403":
    $ref: '#/components/responses/Forbidden'
  "404":
    $ref: '#/components/responses/NotFound'
  "500":
    $ref: '#/components/responses/InternalServerError'
  "503":
    $ref: '#/components/responses/ServiceUnavailable'

[llamapreview]

AI Code Review by LlamaPReview

🎯 TL;DR & Recommendation

Recommendation: Request Changes

This PR introduces a response reuse feature but has critical issues including potential naming collisions that violate OpenAPI specs, lack of response equivalence checks leading to incorrect documentation, and broken customization callbacks.

📄 Documentation Diagram

This diagram documents the new response reuse workflow in the Reflector.

sequenceDiagram
    participant O as Operation
    participant R as Reflector
    participant C as Components

    O->>R: AddOperation
    R->>R: setupResponse
    alt ReuseResponses enabled
        R->>R: generateResponseName
        R->>R: collectResponse
        R->>C: Store response
        R-->>O: Use reference
    else
        R-->>O: Use inline response
    end
    note over R: PR #35;156 added ReuseResponses flag and response collection

Loading

🌟 Strengths

• Adds valuable response reuse capability for OpenAPI specs.
• Includes comprehensive tests for basic functionality.

Priority	File	Category	Impact Summary	Anchors
P1	openapi3/reflect.go	Architecture	Naming collisions violate OpenAPI uniqueness	method:generateResponseName
P1	openapi3/reflect.go	Architecture	Missing equivalence check may reuse wrong responses	method:collectResponse
P1	openapi3/reflect.go	Architecture	Customization callback broken for reused responses	method:setupResponse
P2	openapi3/reflect.go	Maintainability	Global flag lacks per-operation control
P2	response_reuse_test.go	Testing	Tests incomplete, missing edge cases
P2	openapi3/reflect.go	Maintainability	Poor anonymous struct handling causes collisions	method:generateResponseName

🔍 Notable Themes

• The response naming strategy needs refinement to ensure uniqueness across operations and status codes.
• Customization functionality is compromised when responses are reused, requiring a different approach.

📈 Risk Diagram

This diagram illustrates the risks associated with response naming collisions and broken customization.

sequenceDiagram
    participant R as Reflector
    participant C as Components

    R->>R: generateResponseName
    note over R: R1(P1): Naming collision risk
    R->>R: collectResponse
    note over R: R2(P1): No equivalence check
    R->>C: Store or reuse response
    note over R: R3(P1): Customization broken for references

Loading

⚠️ **Unanchored Suggestions (Manual Review Recommended)**

The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.

---

📁 File: openapi3/reflect.go

The name generation doesn't handle anonymous structs or generic types well. When cu.Structure is an anonymous struct, structType.Name() returns empty, falling back to the status-based name. This could lead to generic names like "Response200" for different anonymous structs, causing collisions. The implementation should handle these edge cases more gracefully.

Suggestion:

// For anonymous structs, generate a name based on content fingerprint
if structType.Name() == "" {
    // Use a hash of the structure type or other identifying information
    return "AnonymousResponse" + strconv.Itoa(httpStatus) + generateFingerprint(cu)
}

Related Code:

// generateResponseName creates a consistent name for a response based on its status and schema.
func (r *Reflector) generateResponseName(httpStatus int, cu openapi.ContentUnit) string {
	// ... implementation ...
}

---

---

💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.

[llamapreview]

P1 | Confidence: High

The response name generation logic creates potential naming collisions. When different operations use the same response structure type, they'll generate identical names. However, when different status codes use different structures that happen to have the same type name, they'll also collide. This violates OpenAPI's requirement that component names be unique. The related_context shows multiple test files creating Reflector instances, and in real usage, different packages could have structs with identical names.

Code Suggestion:

// Include HTTP status in the name to ensure uniqueness across different status codes
if structType.Name() != "" {
    return structType.Name() + strconv.Itoa(httpStatus)
}

Evidence: method:generateResponseName

[llamapreview]

P1 | Confidence: High

The current implementation doesn't verify that existing responses with the same name are actually equivalent to the new response. If two different responses generate the same name (due to the naming collision issue), the second response will incorrectly reference the first response's definition. This could lead to incorrect API documentation where different response types are documented as being the same.

Code Suggestion:

// Add response equivalence check before reusing
if existing, exists := r.Spec.Components.Responses.MapOfResponseOrRefValues[name]; exists {
    // Compare key response properties before reusing
    if responsesAreEquivalent(existing, resp) {
        return name
    }
    // Generate unique name for different responses with same base name
    name = generateUniqueName(name, r.Spec.Components.Responses)
}

Evidence: method:collectResponse

[llamapreview]

P1 | Confidence: High

When ReuseResponses is enabled and a response is stored in components, the cu.Customize callback receives an empty ResponseOrRef with only a reference set. This breaks the customization capability because the callback can no longer modify the actual response content. The related_context shows extensive usage of operation customization in tests, indicating this is a critical feature.

Code Suggestion:

if refName != "" {
    ror = ResponseOrRef{}
    ror.SetReference("#/components/responses/" + refName)
    // Apply customization to the component response if it exists
    if compResp, exists := r.Spec.Components.Responses.MapOfResponseOrRefValues[refName]; exists {
        if cu.Customize != nil {
            cu.Customize(&compResp)
        }
    }
} else {
    ror = ResponseOrRef{Response: resp}
    if cu.Customize != nil {
        cu.Customize(&ror)
    }
}

Evidence: method:setupResponse

[llamapreview]

P2 | Confidence: Medium

Speculative: The feature introduces a global flag that affects all operations, but there's no way to control response reuse on a per-operation basis. In complex APIs, some operations might benefit from response reuse while others should keep inline responses for clarity. The related_context shows diverse operation patterns that might have different response reuse requirements.

Code Suggestion:

// Consider operation-level override
type OperationContext struct {
    // ... existing fields ...
    ReuseResponses *bool // nil means use reflector setting
}

[llamapreview]

P2 | Confidence: Medium

Speculative: The tests verify that components are populated but don't validate the actual content or reference correctness. They don't test edge cases like naming collisions, different structures with same names, or the interaction with the Customize callback. More comprehensive validation would ensure the feature works correctly in complex scenarios.

Code Suggestion:

// Add test for naming collision scenario
func TestResponseReuse_NamingCollision(t *testing.T) {
    // Test that different structures with same type name don't collide
}

// Add test for customize callback with reused responses  
func TestResponseReuse_WithCustomize(t *testing.T) {
    // Verify customization works correctly with component references
}