AEP Compliance Gap Analysis & Implementation Plan#42
Open
ebichman-1 wants to merge 1 commit into
Open
Conversation
…r, and error content-type
Close four AEP compliance gaps that the upstream linter does not enforce:
- AEP-132: Rename PolicyList response field from to
- AEP-133: Populate Location header in CreatePolicy 201 response
- AEP-193: Add middleware to return for error responses
- Add custom Spectral rules to prevent regressions on the above
BREAKING CHANGE: ListPolicies response JSON field renamed from to .
Assisted-by: Claude Opus 4.6 (Anthropic)
Signed-off-by: ebichman-1 <ebichman@redhat.com>
ebichman-1
requested review from
gabriel-farache,
gciavarrini,
jenniferubah,
machacekondra and
ygalblum
as code owners
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The ProblemJSON middleware wraps http.ResponseWriter but doesn’t preserve optional interfaces like http.Flusher, http.Hijacker, or http.Pusher; consider forwarding those when supported so middleware doesn’t break handlers that rely on them (e.g., streaming, websockets, or HTTP/2 server push).
- The aep-132 Spectral rule keying only on presence of next_page_token may flag any schema that happens to paginate rather than strictly list responses; if that’s too broad in practice, consider tightening the JSONPath (e.g., to specific operation responses) to avoid false positives.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The ProblemJSON middleware wraps http.ResponseWriter but doesn’t preserve optional interfaces like http.Flusher, http.Hijacker, or http.Pusher; consider forwarding those when supported so middleware doesn’t break handlers that rely on them (e.g., streaming, websockets, or HTTP/2 server push).
- The aep-132 Spectral rule keying only on presence of next_page_token may flag any schema that happens to paginate rather than strictly list responses; if that’s too broad in practice, consider tightening the JSONPath (e.g., to specific operation responses) to avoid false positives.
## Individual Comments
### Comment 1
<location path="internal/handlers/v1alpha1/policy.go" line_range="68-69" />
<code_context>
- // Convert back to server.Policy
return server.CreatePolicy201JSONResponse{
Body: policyV1Alpha1ToServer(*created),
+ Headers: server.CreatePolicy201ResponseHeaders{
+ Location: fmt.Sprintf("/api/v1alpha1/policies/%s", *created.Id),
+ },
}, nil
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider URL-escaping the policy ID before embedding it into the Location header path.
If `created.Id` can include characters that aren’t path-safe (spaces, `#`, `/`, etc.), the resulting Location header may be an invalid or ambiguous URI. Please use `url.PathEscape(*created.Id)` (or equivalent) when constructing the path, and consider defining the path template as a shared constant to keep it consistent across handlers.
</issue_to_address>
### Comment 2
<location path="internal/apiserver/server.go" line_range="47" />
<code_context>
router.Use(logging.RequestLogger)
router.Use(middleware.Logger)
router.Use(middleware.Recoverer)
+ router.Use(custommiddleware.ProblemJSON)
swagger, err := v1alpha1.GetSwagger()
</code_context>
<issue_to_address>
**issue (bug_risk):** The ProblemJSON middleware order likely prevents it from rewriting responses generated by Recoverer (e.g., panics).
Because of Chi’s middleware wrapping order, adding `ProblemJSON` after `middleware.Recoverer` means panics are handled by `Recoverer` using the original `ResponseWriter`, so `ProblemJSON` never sees or rewrites those 500 responses. That leaves panic/500 responses without the `application/problem+json` Content-Type, which may violate AEP-193. To cover all errors, including those from `Recoverer`, register `ProblemJSON` before `Recoverer` so it wraps the full stack.
</issue_to_address>
### Comment 3
<location path="internal/engineserver/server.go" line_range="47" />
<code_context>
router.Use(logging.RequestLogger)
router.Use(middleware.Logger)
router.Use(middleware.Recoverer)
+ router.Use(custommiddleware.ProblemJSON)
swagger, err := v1alpha1.GetSwagger()
</code_context>
<issue_to_address>
**issue (bug_risk):** Same middleware ordering concern here: ProblemJSON may not affect error responses generated by Recoverer.
To match the API server and ensure `middleware.Recoverer` errors are returned as `application/problem+json`, register `ProblemJSON` before `middleware.Recoverer` in this stack.
</issue_to_address>
### Comment 4
<location path="internal/middleware/problemjson.go" line_range="17-26" />
<code_context>
+ })
+}
+
+type problemJSONWriter struct {
+ http.ResponseWriter
+ wroteHeader bool
+}
+
+func (w *problemJSONWriter) WriteHeader(code int) {
+ if !w.wroteHeader && code >= 400 {
+ ct := w.Header().Get("Content-Type")
+ if strings.HasPrefix(ct, "application/json") {
+ w.Header().Set("Content-Type", strings.Replace(ct, "application/json", "application/problem+json", 1))
+ }
+ }
+ w.wroteHeader = true
+ w.ResponseWriter.WriteHeader(code)
+}
+
+func (w *problemJSONWriter) Write(b []byte) (int, error) {
+ if !w.wroteHeader {
+ w.WriteHeader(http.StatusOK)
+ }
+ return w.ResponseWriter.Write(b)
+}
+
+func (w *problemJSONWriter) Unwrap() http.ResponseWriter {
+ return w.ResponseWriter
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** The ResponseWriter wrapper does not forward optional interfaces (Flusher, Hijacker, Pusher, etc.), which may break callers that rely on them.
Since this wrapper only embeds `http.ResponseWriter` and doesn’t forward optional interfaces (`Flusher`, `Hijacker`, `Pusher`, `CloseNotifier`), any middleware/handler doing type assertions for streaming, websockets, etc. may break. Please add conditional delegation for these interfaces in `problemJSONWriter` or otherwise preserve the original concrete type so existing behavior remains unchanged.
</issue_to_address>
### Comment 5
<location path="internal/middleware/problemjson_test.go" line_range="26-29" />
<code_context>
+ }))
+ }
+
+ It("should rewrite Content-Type for 400 responses", func() {
+ rec := httptest.NewRecorder()
+ makeHandler(400, "application/json").ServeHTTP(rec, httptest.NewRequest("GET", "/", nil))
+ Expect(rec.Header().Get("Content-Type")).To(Equal("application/problem+json"))
+ })
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add an E2E test to verify error responses use application/problem+json
Since the implementation plan also called out an end-to-end check, please add an E2E test (e.g., triggering a known 4xx/5xx route) to confirm the middleware is wired into both API servers and that the `Content-Type` header is preserved as `application/problem+json` in real requests, not only in unit-level middleware tests.
Suggested implementation:
```golang
It("should rewrite Content-Type for 400 responses", func() {
rec := httptest.NewRecorder()
makeHandler(400, "application/json").ServeHTTP(rec, httptest.NewRequest("GET", "/", nil))
Expect(rec.Header().Get("Content-Type")).To(Equal("application/problem+json"))
})
It("should preserve application/problem+json Content-Type in an end-to-end HTTP request", func() {
// Arrange: wrap a real HTTP handler with the ProblemJSON middleware and serve via httptest.Server
handler := ProblemJSON(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Simulate a handler that returns a problem+json 400 response
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusBadRequest)
_, _ = w.Write([]byte(`{"type":"INTERNAL","status":400,"title":"bad request"}`))
}))
server := httptest.NewServer(handler)
defer server.Close()
// Act: perform a real HTTP request against the server
resp, err := http.Get(server.URL)
Expect(err).NotTo(HaveOccurred())
defer resp.Body.Close()
// Assert: middleware has rewritten the Content-Type header on the actual HTTP response
Expect(resp.StatusCode).To(Equal(http.StatusBadRequest))
Expect(resp.Header.Get("Content-Type")).To(Equal("application/problem+json"))
})
```
If your actual API servers are constructed via helper functions (e.g., `NewPublicServer`, `NewAdminServer`) that already wire in the `ProblemJSON` middleware, you may want to further adapt the E2E test to:
1. Start each real server instance (public/admin) using those constructors instead of directly wrapping `ProblemJSON` around a bare handler.
2. Hit a known 4xx/5xx route on each server and assert the `Content-Type` is `application/problem+json` as done above.
3. Place such tests either here or in a higher-level integration test file, depending on your existing test structure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+68
to
+69
| Headers: server.CreatePolicy201ResponseHeaders{ | ||
| Location: fmt.Sprintf("/api/v1alpha1/policies/%s", *created.Id), |
There was a problem hiding this comment.
issue (bug_risk): Consider URL-escaping the policy ID before embedding it into the Location header path.
If created.Id can include characters that aren’t path-safe (spaces, #, /, etc.), the resulting Location header may be an invalid or ambiguous URI. Please use url.PathEscape(*created.Id) (or equivalent) when constructing the path, and consider defining the path template as a shared constant to keep it consistent across handlers.
| router.Use(logging.RequestLogger) | ||
| router.Use(middleware.Logger) | ||
| router.Use(middleware.Recoverer) | ||
| router.Use(custommiddleware.ProblemJSON) |
There was a problem hiding this comment.
issue (bug_risk): The ProblemJSON middleware order likely prevents it from rewriting responses generated by Recoverer (e.g., panics).
Because of Chi’s middleware wrapping order, adding ProblemJSON after middleware.Recoverer means panics are handled by Recoverer using the original ResponseWriter, so ProblemJSON never sees or rewrites those 500 responses. That leaves panic/500 responses without the application/problem+json Content-Type, which may violate AEP-193. To cover all errors, including those from Recoverer, register ProblemJSON before Recoverer so it wraps the full stack.
| router.Use(logging.RequestLogger) | ||
| router.Use(middleware.Logger) | ||
| router.Use(middleware.Recoverer) | ||
| router.Use(custommiddleware.ProblemJSON) |
There was a problem hiding this comment.
issue (bug_risk): Same middleware ordering concern here: ProblemJSON may not affect error responses generated by Recoverer.
To match the API server and ensure middleware.Recoverer errors are returned as application/problem+json, register ProblemJSON before middleware.Recoverer in this stack.
Comment on lines
+17
to
+26
| type problemJSONWriter struct { | ||
| http.ResponseWriter | ||
| wroteHeader bool | ||
| } | ||
|
|
||
| func (w *problemJSONWriter) WriteHeader(code int) { | ||
| if !w.wroteHeader && code >= 400 { | ||
| ct := w.Header().Get("Content-Type") | ||
| if strings.HasPrefix(ct, "application/json") { | ||
| w.Header().Set("Content-Type", strings.Replace(ct, "application/json", "application/problem+json", 1)) |
There was a problem hiding this comment.
issue (bug_risk): The ResponseWriter wrapper does not forward optional interfaces (Flusher, Hijacker, Pusher, etc.), which may break callers that rely on them.
Since this wrapper only embeds http.ResponseWriter and doesn’t forward optional interfaces (Flusher, Hijacker, Pusher, CloseNotifier), any middleware/handler doing type assertions for streaming, websockets, etc. may break. Please add conditional delegation for these interfaces in problemJSONWriter or otherwise preserve the original concrete type so existing behavior remains unchanged.
Comment on lines
+26
to
+29
| It("should rewrite Content-Type for 400 responses", func() { | ||
| rec := httptest.NewRecorder() | ||
| makeHandler(400, "application/json").ServeHTTP(rec, httptest.NewRequest("GET", "/", nil)) | ||
| Expect(rec.Header().Get("Content-Type")).To(Equal("application/problem+json")) |
There was a problem hiding this comment.
suggestion (testing): Add an E2E test to verify error responses use application/problem+json
Since the implementation plan also called out an end-to-end check, please add an E2E test (e.g., triggering a known 4xx/5xx route) to confirm the middleware is wired into both API servers and that the Content-Type header is preserved as application/problem+json in real requests, not only in unit-level middleware tests.
Suggested implementation:
It("should rewrite Content-Type for 400 responses", func() {
rec := httptest.NewRecorder()
makeHandler(400, "application/json").ServeHTTP(rec, httptest.NewRequest("GET", "/", nil))
Expect(rec.Header().Get("Content-Type")).To(Equal("application/problem+json"))
})
It("should preserve application/problem+json Content-Type in an end-to-end HTTP request", func() {
// Arrange: wrap a real HTTP handler with the ProblemJSON middleware and serve via httptest.Server
handler := ProblemJSON(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Simulate a handler that returns a problem+json 400 response
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusBadRequest)
_, _ = w.Write([]byte(`{"type":"INTERNAL","status":400,"title":"bad request"}`))
}))
server := httptest.NewServer(handler)
defer server.Close()
// Act: perform a real HTTP request against the server
resp, err := http.Get(server.URL)
Expect(err).NotTo(HaveOccurred())
defer resp.Body.Close()
// Assert: middleware has rewritten the Content-Type header on the actual HTTP response
Expect(resp.StatusCode).To(Equal(http.StatusBadRequest))
Expect(resp.Header.Get("Content-Type")).To(Equal("application/problem+json"))
})If your actual API servers are constructed via helper functions (e.g., NewPublicServer, NewAdminServer) that already wire in the ProblemJSON middleware, you may want to further adapt the E2E test to:
- Start each real server instance (public/admin) using those constructors instead of directly wrapping
ProblemJSONaround a bare handler. - Hit a known 4xx/5xx route on each server and assert the
Content-Typeisapplication/problem+jsonas done above. - Place such tests either here or in a higher-level integration test file, depending on your existing test structure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AEP Compliance Gap Analysis & Implementation Plan
Context
The existing .spectral.yaml extends the upstream aep-dev/aep-openapi-linter (covering AEPs
0004, 0122, 0131-0137, 0140, 0142-0144, 0151, 0158, 0193) and adds one custom
rule. The Makefile has a working check-aep target. Both specs currently pass
all upstream linter rules with zero warnings.
Despite passing the linter, four compliance gaps exist - requirements the AEP
standard mandates but the upstream linter does not enforce.
Gap Summary
What's Already Compliant (no changes needed)
Implementation Plan
Phase 1: Location Header Fix (GAP 2) — Non-Breaking
Files to modify:
Change: Populate the Location header in the CreatePolicy201JSONResponse. The
generated server code already supports the Headers field — the handler simply
never sets it.
Tests: Update CreatePolicy unit test to assert Location header. Add E2E
assertion for resp.HTTPResponse.Header.Get("Location").
Verification: make test + make test-e2e-full
Phase 2: List Response Field Rename (GAP 1) — Breaking API Change
Rationale: AEP-132 states the list response resource array MUST be named
results. Current name is policies. Since this is a v1alpha1 API, breaking
changes are expected.
Step 1 — OpenAPI spec change:
schema (lines 489-495)
Step 2 — Regenerate code:
Step 3 — Update hand-written Go code:
All references to .Policies on v1alpha1.PolicyList / server.PolicyList types
become .Results:
conversion)
resp.JSON200.Results)
Important: The store-layer store.PolicyListResult.Policies is NOT affected —
it's an internal struct, not part of the API contract.
Verification: make generate-api + make build + make test + make test-e2e-full
Phase 3: Error Content-Type via Middleware (GAP 3)
Approach: Add a Chi middleware that rewrites Content-Type from
application/json to application/problem+json for responses with status >= 400.
This avoids OpenAPI spec changes or generated type renames.
New file: internal/middleware/problem_json.go
application/problem+json
Files to modify:
Tests:
no-op for 2xx)
application/problem+json
Phase 4: Custom Spectral Rules (GAP 4)
Add rules to .spectral.yaml to catch compliance issues the upstream linter
misses:
Verification: make check-aep must pass after all prior phases are complete.