Return 400 Bad Request for missing body in RegisterArtifact#169
Return 400 Bad Request for missing body in RegisterArtifact#169
Conversation
|
@copilot internal/app/service.go:89 - Should be a 404 error from the api contract that if the group doesn't exist, return a 404 openapi.yaml:440-444 - should likely be an array of accepted repositories |
Fixed both issues in commit aa7173d:
|
There was a problem hiding this comment.
Pull request overview
Adds support for registering artifacts under a group via a new POST /groups/{groupID}/artifacts endpoint, including OpenAPI/spec updates and supporting service/handler logic.
Changes:
- Added OpenAPI definition for artifact registration plus
ArtifactRegistrationschema; updatedgitRepositoryto an array of URLs. - Implemented
Service.RegisterArtifactandHandler.RegisterArtifactwiring, plus updated mocks/tests. - Regenerated
api/api.gen.goto include the new endpoint/types; updated.gitignorefor Go build outputs.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| openapi.yaml | Defines the new POST endpoint and request schema; updates artifact git repository field to an array. |
| internal/httpapi/handler.go | Adds strict handler implementation for RegisterArtifact. |
| internal/httpapi/handler_test.go | Extends handler test scaffolding and adds a RegisterArtifact test. |
| internal/domain/group.go | Introduces a domain Artifact model used by the service/handler. |
| internal/app/service.go | Adds transactional RegisterArtifact implementation and new sentinel errors. |
| internal/app/service_test.go | Adds unit tests for RegisterArtifact service behavior. |
| api/api.gen.go | Regenerates OpenAPI bindings for the new endpoint and updated artifact schema. |
| .gitignore | Adds ignores for Go binaries (with a duplicated bin/ entry). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Artifact' |
There was a problem hiding this comment.
The POST /groups/{groupID}/artifacts operation only defines a 201 response, but the PR/API contract says group-not-found should be a 404 (and duplicates are prevented, which typically maps to 409). Add the missing error responses to the OpenAPI spec so the generated server/client types can return the correct HTTP status codes instead of falling back to generic error handling.
| $ref: '#/components/schemas/Artifact' | |
| $ref: '#/components/schemas/Artifact' | |
| '404': | |
| description: Group not found | |
| '409': | |
| description: Artifact already exists in the group |
| website: | ||
| type: string | ||
| format: URL | ||
| example: "https://spongepowered.org/" | ||
| description: The website URL (optional) | ||
| issues: | ||
| type: string | ||
| format: URL | ||
| example: "https://github.com/SpongePowered/Sponge/issues" | ||
| description: The issues/tracker URL (optional) |
There was a problem hiding this comment.
The API description mentions website and issues are nullable, but the schema only allows strings (no nullable: true). If you intend to accept explicit JSON nulls (and document that behavior), mark these properties as nullable in OpenAPI 3.0; otherwise update the contract wording to “optional” instead of “nullable”.
| // Check if artifact already exists | ||
| _, err = tx.GetArtifactByGroupAndId(ctx, db.GetArtifactByGroupAndIdParams{ | ||
| GroupID: artifact.GroupID, | ||
| ArtifactID: artifact.ArtifactID, | ||
| }) | ||
| if err == nil { | ||
| // Artifact exists | ||
| return ErrArtifactAlreadyExists | ||
| } | ||
| if !errors.Is(err, pgx.ErrNoRows) { | ||
| return fmt.Errorf("failed to check if artifact exists: %w", err) | ||
| } |
There was a problem hiding this comment.
Duplicate prevention isn’t actually guaranteed here: CreateArtifact is defined as an upsert (ON CONFLICT ... DO UPDATE, see internal/db/query.sql.go:12-20). Under concurrency, two requests can both pass the pre-check and the second will silently update the existing row, returning 201 instead of ErrArtifactAlreadyExists. Fix by making the insert fail on conflict (or DO NOTHING + detect no row returned) and map the unique-violation/conflict to ErrArtifactAlreadyExists.
| // Create git repositories JSON array | ||
| gitRepos := artifact.GitRepositories | ||
| gitReposJSON, err := json.Marshal(gitRepos) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal git repositories: %w", err) | ||
| } |
There was a problem hiding this comment.
json.Marshal on a nil []string returns the JSON literal null, not []. Since git_repositories is modeled as a JSONB array (db/schema.sql:13), storing null is likely incorrect and can break consumers that assume an array. Ensure artifact.GitRepositories is non-nil (and ideally non-empty) before marshaling, or normalize nil to an empty slice.
| // Create the artifact | ||
| _, err = tx.CreateArtifact(ctx, db.CreateArtifactParams{ | ||
| GroupID: artifact.GroupID, | ||
| ArtifactID: artifact.ArtifactID, | ||
| Name: artifact.DisplayName, | ||
| Website: artifact.Website, | ||
| GitRepositories: gitReposJSON, | ||
| }) |
There was a problem hiding this comment.
domain.Artifact includes Issues, and the handler returns issues in the 201 response, but the service/database write path never persists it (artifacts table has no issues column per db/schema.sql). This will make POST appear to succeed while the data can’t be retrieved later. Either persist issues in storage (schema + queries + CreateArtifactParams) or remove it from the registration flow/response until it’s supported.
| txMock.EXPECT().CreateArtifact(mock.Anything, mock.MatchedBy(func(params db.CreateArtifactParams) bool { | ||
| return params.GroupID == "org.example" && | ||
| params.ArtifactID == "myartifact" && | ||
| params.Name == "My Artifact" && | ||
| params.Website != nil && *params.Website == "https://example.org" | ||
| })).Return(db.Artifact{}, nil) |
There was a problem hiding this comment.
The CreateArtifact expectations don’t currently assert that GitRepositories is marshaled/passed correctly (e.g., JSON array bytes, and multi-repo ordering). Since this is a key behavior change, strengthen the matcher to validate params.GitRepositories decodes to the expected []string so regressions (including nil->null) are caught.
gabizou
force-pushed
the
copilot/create-artifact-registration-endpoint
branch
from
5d9458a to
bf1d298
Compare
- Add 404 and 409 responses to OpenAPI spec for POST /groups/{groupID}/artifacts
- Create custom HTTP error types that implement both error interface and response interface
- GroupNotFoundError returns 404 when group doesn't exist
- ArtifactAlreadyExistsError returns 409 when artifact already exists in group
- Update handler to return appropriate error types based on service errors
- Add comprehensive tests for error handling and HTTP status codes
- Regenerated API code with new response types
Co-authored-by: gabizou <1203877+gabizou@users.noreply.github.com>
- Add 400 response to OpenAPI spec for POST /groups/{groupID}/artifacts
- Create BadRequestError custom type that implements both error interface and response interface
- Update handler to return BadRequestError instead of generic error for missing body
- Add comprehensive tests for BadRequestError and missing body scenario
- Regenerated API code with new 400 response type
This prevents missing request body from returning HTTP 500 and properly returns 400 Bad Request instead.
Co-authored-by: gabizou <1203877+gabizou@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var body RegisterArtifactJSONRequestBody | ||
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil { | ||
| sh.options.RequestErrorHandlerFunc(w, r, fmt.Errorf("can't decode JSON body: %w", err)) | ||
| return | ||
| } | ||
| request.Body = &body |
There was a problem hiding this comment.
The generated strict handler always attempts to JSON-decode the request body and returns early on decode errors (including an empty body → EOF). That means RegisterArtifact in internal/httpapi/handler.go will never see request.Body == nil for real HTTP requests, so the new BadRequestError("request body is required") path won’t be used to produce the 400 response described in the PR. If you want a dedicated 400 response object/message for missing/empty bodies, handle EOF/http.NoBody in the request binding layer (or via NewStrictHandlerWithOptions + a custom RequestErrorHandlerFunc) and translate it into the desired 400 response.
| type: array | ||
| items: | ||
| type: string | ||
| format: URL |
There was a problem hiding this comment.
Artifact.gitRepository is changed from a single string URL to an array of URLs. This is a breaking API change for all endpoints returning Artifact (and changes the generated Go type). Please confirm this is intended and ensure any existing clients/handlers are updated; if only registration needs an array, consider keeping the response field stable or introducing a new field instead.
| type: array | |
| items: | |
| type: string | |
| format: URL | |
| type: string | |
| format: URL |
RegisterArtifact treated missing request body as a generic error, resulting in HTTP 500. Now returns proper 400 Bad Request via dedicated response object.
Changes
errorandRegisterArtifactResponseObjectinterfaces, following pattern of existing 404/409 errorsNewBadRequestError("request body is required")instead offmt.Errorf()Example
Before:
After:
Error types satisfy both Go error semantics and HTTP contract requirements, ensuring proper status codes without bubbling generic errors.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.