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

Make error rendering context-aware #21803

Closed
wants to merge 8 commits into from
Closed

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Nov 13, 2022

  • Currently the error handling in modules/context/context.go always use ctx.HTML(indirectly) to render the error, however it is possible that the context doesn't have any HTML render set(e.g. APIContext).
  • Let the context creator provide the error handling, currently web will still use the current functions and API/Package will roughly do the same but using ctx.JSON.
  • Implement a dummyRender for the API/Package Context to not see a vague NPE error.

For clarity purposes, this panic was found via:

context.RedirectToUser(ctx.Context, userName, redirectUserID)
->
ctx.ServerError("GetUserByID", err)
->
ctx.HTML(http.StatusInternalServerError, base.TplName("status/500"))
->
if err := ctx.Render.HTML(ctx.Resp, status, string(name), templates.BaseVars().Merge(ctx.Data)); err != nil {

ContextAPI doesn't set the Render field, which caused an NPE.

Gusted added 3 commits November 13, 2022 15:05
- Currently the error handling in `modules/context/context.go` always
use `ctx.HTML`(indirectly) to render the error, however it is possible that the
context doesn't have any HTML render set(e.g. `APIContext`).
- Let the context creator provide the error handling, currently web will
still use the current functions and API will roughly do the same but
using `ctx.JSON`.
@silverwind silverwind added this to the 1.19.0 milestone Nov 13, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 13, 2022
modules/context/api.go Outdated Show resolved Hide resolved
Gusted and others added 2 commits November 13, 2022 22:20
@lunny
Copy link
Member

lunny commented Nov 15, 2022

For a long-term target, APIContent should have a new struct to remove unnecessary fields rather than deriving from Context. The benefit of that is less memory usage per request especially for those big Gitea instances. #16455 should be the first step towards that target. I'm not against this PR but I think this still need to be changed in future.

@6543
Copy link
Member

6543 commented Nov 18, 2022

Without looking more into the code, did you make sure normal users dont see the stacktrace in prod mode?

@Gusted
Copy link
Contributor Author

Gusted commented Nov 18, 2022

Stacktraces were never exposed to begin with and conditions were copies (ctrl + f setting.IsProd)

@lunny
Copy link
Member

lunny commented Feb 6, 2023

For web routers, sometimes it will return JSON, sometimes return HTML and for RSS it's XML.
I couldn't find it becomes more convenient than explicitly invoking a render function in the router function.

@lunny lunny modified the milestones: 1.19.0, 1.20.0 Feb 6, 2023
@Gusted
Copy link
Contributor Author

Gusted commented Feb 9, 2023

For web routers, sometimes it will return JSON, sometimes return HTML and for RSS it's XML. I couldn't find it becomes more convenient than explicitly invoking a render function in the router function.

That would require a very broad refactor, which I won't contribute. If that's the consensus to fix this problem, feel free to close my PR.

@wxiaoguang
Copy link
Contributor

We need to decouple the Gitea's contexts first. Otherwise more things are mixed together.

-> Decouple WebContext/APIContext/PrivateContext #24786

@wxiaoguang wxiaoguang closed this May 18, 2023
@GiteaBot GiteaBot removed this from the 1.20.0 milestone May 18, 2023
lunny pushed a commit that referenced this pull request May 21, 2023
Replace #16455

Close #21803

Mixing different Gitea contexts together causes some problems:

1. Unable to respond proper content when error occurs, eg: Web should
respond HTML while API should respond JSON
2. Unclear dependency, eg: it's unclear when Context is used in
APIContext, which fields should be initialized, which methods are
necessary.


To make things clear, this PR introduces a Base context, it only
provides basic Req/Resp/Data features.

This PR mainly moves code. There are still many legacy problems and
TODOs in code, leave unrelated changes to future PRs.
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants