Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" #23687

wxiaoguang · 2023-03-24T14:18:27Z

Why this PR comes

At first, I'd like to help users like #23636 (there are a lot)

The unclear "Internal Server Error" is quite anonying, scare users, frustrate contributors, nobody knows what happens.

So, it's always good to provide meaningful messages to end users (of course, do not leak sensitive information).

When I started working on the "response message to end users", I found that the related code has a lot of technical debt. A lot of copy&paste code, unclear fields and usages.

So I think it's good to make everything clear.

Tech Backgrounds

Gitea has many sub-commands, some are used by admins, some are used by SSH servers or Git Hooks. Many sub-commands use "internal API" to communicate with Gitea web server.

Before, Gitea server always use StatusCode + Json "err" field to return messages.

The CLI sub-commands: they expect to show all error related messages to site admin
The Serv/Hook sub-commands (for git clients): they could only show safe messages to end users, the error log could only be recorded by "SSHLog" to Gitea web server.

In the old design, it assumes that:

If the StatusCode is 500 (in some functions), then the "err" field is error log, shouldn't be exposed to git client.
If the StatusCode is 40x, then the "err" field could be exposed. And some functions always read the "err" no matter what the StatusCode is.

The old code is not strict, and it's difficult to distinguish the messages clearly and then output them correctly.

This PR

To help to remove duplicate code and make everything clear, this PR introduces ResponseExtra and requestJSONResp.

ResponseExtra is a struct which contains "extra" information of a internal API response, including StatusCode, UserMsg, Error
requestJSONResp is a generic function which can be used for all cases to help to simplify the calls.
Remove all map["err"], always use private.Response{Err} to construct error messages.
User messages and error messages are separated clearly, the fail and handleCliResponseExtra will output correct messages.
Replace all Internal Server Error messages with meaningful (still safe) messages.

This PR saves more than 300 lines, while makes the git client messages more clear.

Many gitea-serv/git-hook related essential functions are covered by tests.

wxiaoguang · 2023-03-24T18:38:32Z

This PR is ready for review

lunny · 2023-03-25T13:26:07Z

I think something maybe wrong when push via ssh protocol but clone is OK.

ServCommand failed: internal API error response, status=403
fatal: protocol error: bad line length character:
Git

wxiaoguang · 2023-03-25T13:34:11Z

I think something maybe wrong when push via ssh protocol but clone is OK.
ServCommand failed: internal API error response, status=403
fatal: protocol error: bad line length character:
Git

The "push" should have been covered by tests.

How do you reproduce it? According to "internal API error response, status=403", I guess it's related to some permission or protection problem during your test?

lunny · 2023-03-25T13:39:55Z

I think something maybe wrong when push via ssh protocol but clone is OK.
ServCommand failed: internal API error response, status=403
fatal: protocol error: bad line length character:
Git
The "push" should have been covered by tests.

How do you reproduce it? According to "internal API error response, status=403", I guess it's related to some permission or protection problem during your test?

Just clone and push. I'm the admin of this instance.
OK. My fault. It's a mirror repository. But I think the user message is not friendly like before.

wxiaoguang · 2023-03-25T14:14:25Z

OK. My fault. It's a mirror repository. But I think the user message is not friendly like before.

My plan is to fine tune more messages in the future, and we have enough time to eat our own dogfood.

At least, it's much easier than before to provide more friendly messages to end users.

Made a fix for the log output, could you try if the new commit could output the friendly messages?

cmd/keys.go

cmd/mailer.go

cmd/serv.go

modules/httplib/httplib.go

routers/private/serv.go

modules/private/internal.go

modules/private/request.go

Co-authored-by: delvh <[email protected]>

# Conflicts: # contrib/pr/checkout.go

lunny · 2023-03-26T03:40:47Z

$ git push origin main
Gitea: Mirror Repository lunny2/tea is read-only
ServCommand failed: internal API error response, status=403
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Maybe the line ServCommand failed: internal API error response, status=403 should not be displayed?

wxiaoguang · 2023-03-26T03:42:41Z

$ git push origin main
Gitea: Mirror Repository lunny2/tea is read-only
ServCommand failed: internal API error response, status=403
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Maybe the line ServCommand failed: internal API error response, status=403 should not be displayed?

because you are using dev mode. In prod mode, there won't be the internal log

…ists, omit empty response field

jolheiser · 2023-03-28T18:23:25Z

@delvh did you have any further reservations?

delvh

@jolheiser nope.

Ah, nice. With this approval, I've popped the top two priorities off my mental review queue.
Now I can focus on the lower priority PRs again…

delvh · 2023-03-28T18:53:53Z

modules/private/internal.go

-		res.Err = err.Error()
-	}
-	return &res
+	Err     string `json:"err,omitempty"`      // server-side error log message, it won't be exposed to end users


Regarding my previous comment here:
I've previously misunderstood something here, I thought this response would be returned to the user, not that it is the result of a Gitea internal request.
The user message is simply sent through fail.
That makes a lot more sense then.

* upstream/main: Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" (go-gitea#23687) Add CSS rules for basic colored labels (go-gitea#23774) Add meilisearch support (go-gitea#23136) Add missing translation for `actions.runners.reset_registration_token_success` (go-gitea#23732) [skip ci] Updated translations via Crowdin Implement Issue Config (go-gitea#20956) Set repository link based on the url in package.json for npm packages (go-gitea#20379) Add API to manage issue dependencies (go-gitea#17935) Add creation time in tag list page (go-gitea#23693) Make minio package support legacy MD5 checksum (go-gitea#23768) Yarden Shoham has a new email address (go-gitea#23767) fix br display for packages curls (go-gitea#23737)

fix

057be7b

wxiaoguang marked this pull request as draft March 24, 2023 14:19

yardenshoham added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 24, 2023

wxiaoguang force-pushed the refactor-cmd branch 8 times, most recently from 528885c to 56914d4 Compare March 24, 2023 18:03

This comment was marked as off-topic.

Sign in to view

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 24, 2023

wxiaoguang force-pushed the refactor-cmd branch from 56914d4 to 3c42039 Compare March 24, 2023 18:38

wxiaoguang marked this pull request as ready for review March 24, 2023 18:38

wxiaoguang force-pushed the refactor-cmd branch 2 times, most recently from 760926f to 5c83c6b Compare March 24, 2023 19:16

wxiaoguang changed the title ~~WIP: Refactor internal API for git commands~~ Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" Mar 24, 2023

wxiaoguang force-pushed the refactor-cmd branch 2 times, most recently from a35f8ce to 949cb5d Compare March 25, 2023 05:34

refactor

9dfa002

wxiaoguang force-pushed the refactor-cmd branch from 949cb5d to 9dfa002 Compare March 25, 2023 05:48

wxiaoguang added 2 commits March 25, 2023 22:23

fix message output

eba9561

use git.InitSimple(ctx)

0183de7

delvh reviewed Mar 25, 2023

View reviewed changes

wxiaoguang and others added 3 commits March 26, 2023 08:47

Update cmd/mailer.go

0919f95

Update cmd/serv.go

e0cb52e

Co-authored-by: delvh <[email protected]>

Merge branch 'main' into refactor-cmd

66a0671

# Conflicts: # contrib/pr/checkout.go

lunny approved these changes Mar 26, 2023

View reviewed changes

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 Mar 26, 2023

wxiaoguang force-pushed the refactor-cmd branch 2 times, most recently from bcf00a2 to fb87fad Compare March 26, 2023 06:42

revert git.InitSimple(ctx), make internal request use "body" if it ex…

c9fd1bb

…ists, omit empty response field

wxiaoguang force-pushed the refactor-cmd branch from fb87fad to c9fd1bb Compare March 26, 2023 06:50

wxiaoguang requested a review from delvh March 26, 2023 19:03

wolfogre approved these changes Mar 28, 2023

View reviewed changes

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 Mar 28, 2023

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 28, 2023

lunny added this to the 1.20.0 milestone Mar 28, 2023

wxiaoguang added 2 commits March 28, 2023 23:30

Merge branch 'main' into refactor-cmd

9c3dc13

add "Gitea: " prefix for dev mode error log message

9d3d7fb

delvh approved these changes Mar 28, 2023

View reviewed changes

Merge branch 'main' into refactor-cmd

52a9d95

lunny merged commit f453879 into go-gitea:main Mar 29, 2023

lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 29, 2023

wxiaoguang deleted the refactor-cmd branch March 29, 2023 08:06

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" #23687

Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" #23687

wxiaoguang commented Mar 24, 2023 •

edited

Loading

This comment was marked as off-topic.

wxiaoguang commented Mar 24, 2023

lunny commented Mar 25, 2023 •

edited

Loading

wxiaoguang commented Mar 25, 2023

lunny commented Mar 25, 2023

wxiaoguang commented Mar 25, 2023 •

edited

Loading

lunny commented Mar 26, 2023

wxiaoguang commented Mar 26, 2023

jolheiser commented Mar 28, 2023

delvh left a comment •

edited

Loading

delvh Mar 28, 2023

Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" #23687

Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" #23687

Conversation

wxiaoguang commented Mar 24, 2023 • edited Loading

Why this PR comes

Tech Backgrounds

This PR

This comment was marked as off-topic.

wxiaoguang commented Mar 24, 2023

lunny commented Mar 25, 2023 • edited Loading

wxiaoguang commented Mar 25, 2023

lunny commented Mar 25, 2023

wxiaoguang commented Mar 25, 2023 • edited Loading

lunny commented Mar 26, 2023

wxiaoguang commented Mar 26, 2023

jolheiser commented Mar 28, 2023

delvh left a comment • edited Loading

Choose a reason for hiding this comment

delvh Mar 28, 2023

Choose a reason for hiding this comment

wxiaoguang commented Mar 24, 2023 •

edited

Loading

lunny commented Mar 25, 2023 •

edited

Loading

wxiaoguang commented Mar 25, 2023 •

edited

Loading

delvh left a comment •

edited

Loading