Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle invalid target when creating releases using API #31841

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

kemzeb
Copy link
Contributor

@kemzeb kemzeb commented Aug 15, 2024

A 500 status code was thrown when passing a non-existent target to the create release API. This snapshot handles this error and instead throws a 404 status code.

Discovered while working on #31840.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 15, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 15, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Aug 15, 2024
@kemzeb
Copy link
Contributor Author

kemzeb commented Aug 16, 2024

Before:
Screenshot 2024-08-15 170507

After:
Screenshot 2024-08-15 170939

@delvh
Copy link
Member

delvh commented Aug 16, 2024

409 Conflict?
Wouldn't 400 Bad Request or 404 be better?

@@ -65,7 +65,7 @@ func createTag(ctx context.Context, gitRepo *git.Repository, rel *repo_model.Rel

commit, err := gitRepo.GetCommit(rel.Target)
if err != nil {
return false, fmt.Errorf("createTag::GetCommit[%v]: %w", rel.Target, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the more verbose error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it as createTag and GetCommit seemed too implementation-specific to release_service.CreateRelease() for it to be included in the API route. I have added an alternative message before printing out the error; let me know if it can be improved!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions in services will be used by both web side and API.
So if you want to change it for API, it is better to also check the web side.

ps: the alternative message you added is in this PR? L256 in routers/api/v1/repo/release.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if you want to change it for API, it is better to also check the web side.

From reading the web route code and observing the web UI, it doesn't seem possible for normal users to pass an invalid target (unless they make create the HTTP request by hand and send it, then they'll get a server error code).

ps: the alternative message you added is in this PR? L256 in routers/api/v1/repo/release.go?

Yes that is correct. I'm open for better ways of describing the error.

@kemzeb
Copy link
Contributor Author

kemzeb commented Aug 16, 2024

409 Conflict? Wouldn't 400 Bad Request or 404 be better?

Did not read the Mozilla docs for this closely (they explain that is used for "implementation specific purposes"). I think either a 404 or 422 makes sense here; I opted for 404.

Tag names cannot have spaces.
@techknowlogick techknowlogick added type/bug backport/v1.22 This PR should be backported to Gitea 1.22 labels Sep 12, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 12, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 12, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 12, 2024
@lunny lunny enabled auto-merge (squash) September 12, 2024 07:19
@lunny lunny merged commit f05d9c9 into go-gitea:main Sep 12, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.24.0 milestone Sep 12, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 12, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 13, 2024
* giteaofficial/main:
  Handle invalid target when creating releases using API (go-gitea#31841)
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Sep 14, 2024
A 500 status code was thrown when passing a non-existent target to the
create release API. This snapshot handles this error and instead throws
a 404 status code.

Discovered while working on go-gitea#31840.
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Sep 14, 2024
yp05327 pushed a commit that referenced this pull request Sep 17, 2024
Backport #31841 by @kemzeb

A 500 status code was thrown when passing a non-existent target to the
create release API. This snapshot handles this error and instead throws
a 404 status code.

Discovered while working on #31840.

Co-authored-by: Kemal Zebari <60799661+kemzeb@users.noreply.github.com>
@yp05327 yp05327 modified the milestones: 1.24.0, 1.23.0 Sep 19, 2024
@kemzeb kemzeb deleted the api-release-catch-no-commit-err branch September 23, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done All backports for this PR have been created backport/v1.22 This PR should be backported to Gitea 1.22 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants