Plan: AEP Compliance Gap Analysis & Remediation#72
Closed
ebichman-1 wants to merge 1 commit into
Closed
Conversation
…ting gaps - Add Update (PATCH) and Delete (DELETE) endpoints for ServiceType. - Update (PATCH) for CatalogItemInstance to satisfy AEP-5 standard method requirements. - Expand Makefile check-aep target to lint servicetype specs, - Add .spectral.yaml overrides for type-only schemas, add custom AEP-148 standard fields rule, - Update Error schema RFC reference from 7807 to 9457. Implement corresponding store, service, and handler layers with tests. Regenerate all API code (types, spec, server, client). Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: ebichman-1 <ebichman@redhat.com> 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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
UpdateServiceType, the handler buildsUpdateServiceTypeRequestasMetadata: request.Body.MetadataandSpec: request.Body.Spec, butrequest.Body.Metadatais*v1alpha1.ServiceTypeMetadatawhile the request struct expects*struct{ Labels *map[string]string }, which will not compile; you likely need an explicit translation layer that copiesMetadata.Labelsinto the pointer field. - The
UpdateCatalogItemInstancehandler always setsDisplayNameandSpecinUpdateCatalogItemInstanceRequest(taking addresses of the body fields), so a PATCH with omitted fields will still overwrite existing values with zero values; if you want true merge-patch semantics, consider distinguishing between omitted and explicitly-null/empty fields (e.g., by using pointer fields in the generated type or by inspecting the raw JSON). - The
ServiceTypeStore.Deleteimplementation infersErrServiceTypeHasCatalogItemsby string-matching"foreign key"in the DB error, which is brittle across drivers and schema changes; if possible, rely on specific DB error codes or a well-defined constraint name to make this mapping more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UpdateServiceType`, the handler builds `UpdateServiceTypeRequest` as `Metadata: request.Body.Metadata` and `Spec: request.Body.Spec`, but `request.Body.Metadata` is `*v1alpha1.ServiceTypeMetadata` while the request struct expects `*struct{ Labels *map[string]string }`, which will not compile; you likely need an explicit translation layer that copies `Metadata.Labels` into the pointer field.
- The `UpdateCatalogItemInstance` handler always sets `DisplayName` and `Spec` in `UpdateCatalogItemInstanceRequest` (taking addresses of the body fields), so a PATCH with omitted fields will still overwrite existing values with zero values; if you want true merge-patch semantics, consider distinguishing between omitted and explicitly-null/empty fields (e.g., by using pointer fields in the generated type or by inspecting the raw JSON).
- The `ServiceTypeStore.Delete` implementation infers `ErrServiceTypeHasCatalogItems` by string-matching `"foreign key"` in the DB error, which is brittle across drivers and schema changes; if possible, rely on specific DB error codes or a well-defined constraint name to make this mapping more robust.
## Individual Comments
### Comment 1
<location path="internal/handlers/v1alpha1/catalog_item_instance.go" line_range="120-122" />
<code_context>
+func (h *Handler) UpdateCatalogItemInstance(ctx context.Context, request server.UpdateCatalogItemInstanceRequestObject) (server.UpdateCatalogItemInstanceResponseObject, error) {
+ h.logger.InfoContext(ctx, "Updating catalog item instance", "id", request.CatalogItemInstanceId)
+
+ updateReq := &service.UpdateCatalogItemInstanceRequest{
+ DisplayName: &request.Body.DisplayName,
+ Spec: &request.Body.Spec,
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** UpdateCatalogItemInstance treats an omitted display_name in the merge-patch body as an explicit empty string, which breaks JSON Merge Patch semantics.
Because `request.Body.DisplayName` is a plain string, it always deserializes to `""` when omitted, and the handler unconditionally passes `&request.Body.DisplayName` into `UpdateCatalogItemInstanceRequest`. As a result, `mergeCatalogItemInstance` will clear the existing display name even when the client did not include `display_name` in the patch, breaking JSON Merge Patch semantics (omitted fields should be ignored).
To fix this, you’ll need a way to distinguish omission from an explicit update, e.g. by:
- Using `*string` for `display_name` in the generated PATCH body type, or
- Adding a layer that inspects the raw body to see if `display_name` was present before setting it on the service request, or
- Introducing a dedicated patch DTO with pointer fields for patchable attributes.
Without this, PATCH calls can unintentionally blank out display names.
</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
+120
to
+122
| updateReq := &service.UpdateCatalogItemInstanceRequest{ | ||
| DisplayName: &request.Body.DisplayName, | ||
| Spec: &request.Body.Spec, |
Contributor
There was a problem hiding this comment.
issue (bug_risk): UpdateCatalogItemInstance treats an omitted display_name in the merge-patch body as an explicit empty string, which breaks JSON Merge Patch semantics.
Because request.Body.DisplayName is a plain string, it always deserializes to "" when omitted, and the handler unconditionally passes &request.Body.DisplayName into UpdateCatalogItemInstanceRequest. As a result, mergeCatalogItemInstance will clear the existing display name even when the client did not include display_name in the patch, breaking JSON Merge Patch semantics (omitted fields should be ignored).
To fix this, you’ll need a way to distinguish omission from an explicit update, e.g. by:
- Using
*stringfordisplay_namein the generated PATCH body type, or - Adding a layer that inspects the raw body to see if
display_namewas present before setting it on the service request, or - Introducing a dedicated patch DTO with pointer fields for patchable attributes.
Without this, PATCH calls can unintentionally blank out display names.
Contributor
|
This PR requires an analysis first. |
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
2 participants
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.
Plan: AEP Compliance Gap Analysis & Remediation
Context
Requests an analysis of the
Makefile and .spectral.yaml against AEP compliance standards
(https://aep.dev/5/), identifying missing linting rules, misconfigured
enforcement, and gaps in automation. The main openapi.yaml currently passes
all 55 AEP linter rules, but there are gaps in tooling coverage, custom rule
enforcement, and standard method completeness per AEP-5.
Gap Analysis Summary
Phase 1: Tooling Fixes (.spectral.yaml + Makefile)
Step 1.1 — Expand Makefile check-aep target
File: Makefile:81-82
Add servicetype specs to the lint command so developers can catch violations
locally:
check-aep: spectral lint --fail-severity=warn ./api/v1alpha1/openapi.yaml ./api/v1alpha1/servicetypes/*/spec.yamlStep 1.2 — Add overrides for servicetype specs
File: .spectral.yaml
Servicetype specs are type-only schemas (no paths, no servers). The general
OpenAPI rules oas3-api-servers, info-contact, and oas3-unused-component are
inapplicable and should be suppressed:
overrides: - files: - "api/v1alpha1/servicetypes/*/spec.yaml" rules: oas3-api-servers: "off" info-contact: "off" oas3-unused-component: "off"Step 1.3 — Add custom AEP-148 standard fields rule
File: .spectral.yaml
Add a rule enforcing that resources with x-aep-resource include the standard
fields path, create_time, update_time:
Phase 2: OpenAPI Spec — Add Missing Standard Methods (AEP-5)
AEP-5 states: "Ideally all five standard methods should be exposed for every
resource."
Step 2.1 — Add ServiceType Update (PATCH) endpoint
File: api/v1alpha1/openapi.yaml — add under /service-types/{serviceTypeId}
path
Following the existing updateCatalogItem pattern (lines 308-350):
Step 2.2 — Add ServiceType Delete (DELETE) endpoint
File: api/v1alpha1/openapi.yaml — add under /service-types/{serviceTypeId}
path
Following the existing deleteCatalogItem pattern (lines 352-377):
Step 2.3 — Add CatalogItemInstance Update (PATCH) endpoint
File: api/v1alpha1/openapi.yaml — add under
/catalog-item-instances/{catalogItemInstanceId} path
Following the existing updateCatalogItem pattern:
Step 2.4 — Fix Error schema RFC reference
File: api/v1alpha1/openapi.yaml:1060
Change RFC 7807 to RFC 9457 in the Error schema description.
Phase 3: Regenerate Code
Step 3.1 — Regenerate all API code
Run make generate-api to regenerate:
Phase 4: Implement Missing Handlers
Step 4.1 — ServiceType Store — Add Update and Delete methods
File: internal/store/service_type.go
Following the catalogItemStore patterns in internal/store/catalog_item.go:
deleting
Add corresponding sentinel errors: ErrServiceTypeHasCatalogItems
Step 4.2 — ServiceType Service — Add Update and Delete methods
File: internal/service/service_type.go
Following the catalogItemService patterns in internal/service/catalog_item.go:
Step 4.3 — ServiceType Handler — Add UpdateServiceType and DeleteServiceType
File: internal/handlers/v1alpha1/service_type.go
Following the handler patterns in internal/handlers/v1alpha1/catalog_item.go:
Add error mapping functions in the service_type errors file.
Step 4.4 — CatalogItemInstance Handler/Service — Add UpdateCatalogItemInstance
Files:
Note: The store layer already has Update() and UpdateResourceID() methods, so
only handler and service implementations are needed.
Phase 5: Tests
Step 5.1 — Add unit tests for new handlers
Follow existing test patterns in internal/handlers/v1alpha1/*_test.go
Step 5.2 — Add unit tests for new service methods
Follow existing test patterns in internal/service/*_test.go
Step 5.3 — Add unit tests for new store methods
Follow existing test patterns in internal/store/*_test.go
Verification
Files to Modify