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.

Sign up for GitHub

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

Jump to bottom

Handle invalid target when creating releases using API #31841

Merged
merged 4 commits into from
Sep 12, 2024
Merged

Handle invalid target when creating releases using API #31841

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a830df
Handle invalid target when creating releases using API
kemzeb Aug 15, 2024
1bb41eb
Make changes given PR feedback
kemzeb Aug 16, 2024
3c65175
Create a correctly-named tag
kemzeb Aug 30, 2024
e288903
Merge branch 'main' into api-release-catch-no-commit-err
GiteaBot Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions routers/api/v1/repo/release.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"code.gitea.io/gitea/models/perm"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit"
"code.gitea.io/gitea/modules/git"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/routers/api/v1/utils"
Expand Down Expand Up @@ -251,6 +252,8 @@ func CreateRelease(ctx *context.APIContext) {
ctx.Error(http.StatusConflict, "ReleaseAlreadyExist", err)
} else if models.IsErrProtectedTagName(err) {
ctx.Error(http.StatusUnprocessableEntity, "ProtectedTagName", err)
} else if git.IsErrNotExist(err) {
ctx.Error(http.StatusConflict, "ErrNotExist", err)
} else {
ctx.Error(http.StatusInternalServerError, "CreateRelease", err)
}
Expand Down
2 changes: 1 addition & 1 deletion services/release/release.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

return false, err
}

if len(msg) > 0 {
Expand Down
18 changes: 18 additions & 0 deletions tests/integration/api_releases_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,24 @@ func TestAPICreateReleaseToDefaultBranchOnExistingTag(t *testing.T) {
createNewReleaseUsingAPI(t, token, owner, repo, "v0.0.1", "", "v0.0.1", "test")
}

func TestAPICreateReleaseGivenInvalidTarget(t *testing.T) {
defer tests.PrepareTestEnv(t)()

repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
session := loginUser(t, owner.LowerName)
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)

urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/releases", owner.Name, repo.Name)
req := NewRequestWithJSON(t, "POST", urlStr, &api.CreateReleaseOption{
TagName: "I point to an invalid target",
Title: "Invalid Target",
Target: "invalid-target",
}).AddTokenAuth(token)

MakeRequest(t, req, http.StatusConflict)
}

func TestAPIGetLatestRelease(t *testing.T) {
defer tests.PrepareTestEnv(t)()

Expand Down