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 every not exist error unwrappable to a fs.ErrNotExist #20891

Merged
merged 16 commits into from
Oct 18, 2022

Conversation

zeripath
Copy link
Contributor

A lot of our code is repeatedly testing if individual errors are
specific types of Not Exist errors. This is repetitative and unnecesary.
Unwrap() error provides a common way of labelling an error as a
NotExist error and we can/should use this.

This PR has chosen to use the common io/fs errors e.g.
fs.ErrNotExist for our errors. This is in some ways not completely
correct as these are not filesystem errors but it seems like a
reasonable thing to do and would allow us to simplify a lot of our code
to errors.Is(err, fs.ErrNotExist) instead of
package.IsErr...NotExist(err)

I am open to suggestions to use a different base error - perhaps
models/db.ErrNotExist if that would be felt to be better.

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath zeripath added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Aug 21, 2022
@zeripath zeripath added this to the 1.18.0 milestone Aug 21, 2022
@zeripath
Copy link
Contributor Author

The next step following this would be to change ServerError code to autodetect if the `Err it is passed is an fs.ErrNotExist and simply return NotFound and so on.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 21, 2022
@zeripath zeripath force-pushed the make-every-not-exist-err-a-not-exist-err branch from 96d8eaa to 7b0550a Compare August 21, 2022 15:25
A lot of our code is repeatedly testing if individual errors are
specific types of Not Exist errors. This is repetitative and unnecesary.
`Unwrap() error` provides a common way of labelling an error as a
NotExist error and we can/should use this.

This PR has chosen to use the common `io/fs` errors e.g.
`fs.ErrNotExist` for our errors. This is in some ways not completely
correct as these are not filesystem errors but it seems like a
reasonable thing to do and would allow us to simplify a lot of our code
to `errors.Is(err, fs.ErrNotExist)` instead of
`package.IsErr...NotExist(err)`

I am open to suggestions to use a different base error - perhaps
`models/db.ErrNotExist` if that would be felt to be better.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

Conflicts fixed.

Signed-off-by: Andrew Thornton <art27@cantab.net>
modules/util/error.go Outdated Show resolved Hide resolved
modules/util/error.go Show resolved Hide resolved
@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 4, 2022
Co-authored-by: delvh <dev.lh@web.de>
@lafriks
Copy link
Member

lafriks commented Oct 13, 2022

If the aim is to use this in ServerError why not to create interface to implement for errors that would return status code to be used for this error in response and safe error message to return in response?

something like: https://github.com/azugo/azugo/blob/main/errors.go#L16-L26

@wolfogre
Copy link
Member

It's tiring to implement the interface by writing lots of func (err X) Unwrap() error, maybe we can provide predefined types to composite, like:

type WrapErrNotExist struct{} // TODO: need a better name

func (err WrapErrNotExist) Unwrap() error {
	return util.ErrExist
}

// ---

type ErrFooNotExist {
	WrapErrNotExist
 	// ...
}

type ErrBarNotExist {
	WrapErrNotExist
 	// ...
}

@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 Oct 13, 2022
@wxiaoguang
Copy link
Contributor

I agree with "this is in some ways not completely correct as these are not filesystem errors"

But :

but it seems like a
reasonable thing to do and would allow us to simplify a lot of our code
to errors.Is(err, fs.ErrNotExist) instead of
package.IsErr...NotExist(err)

Why not use our own error system, instead of using fs.ErrNotExist ?

@wxiaoguang
Copy link
Contributor

@zeripath Do you think if it's better to use our own errors instead of fs errors? I agree with your thought that "This is in some ways not completely correct as these are not filesystem errors". At the moment, I can not imagine any benefit of using the fs errors, IMO using fs errors might make code a little confusing.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

@zeripath Do you think if it's better to use our own errors instead of fs errors? I agree with your thought that "This is in some ways not completely correct as these are not filesystem errors". At the moment, I can not imagine any benefit of using the fs errors, IMO using fs errors might make code a little confusing.

I've changed it - the main benefit of using the fs errors was that we might have been able to rely on upstream handling of these errors, but it's fine.

@zeripath
Copy link
Contributor Author

It's tiring to implement the interface by writing lots of func (err X) Unwrap() error, maybe we can provide predefined types to composite, like:

type WrapErrNotExist struct{} // TODO: need a better name

func (err WrapErrNotExist) Unwrap() error {
	return util.ErrExist
}

// ---

type ErrFooNotExist {
	WrapErrNotExist
 	// ...
}

type ErrBarNotExist {
	WrapErrNotExist
 	// ...
}

Whilst I can see your point I'm not sure it's much better than just writing the three-line function.

What I'd like to do is get rid of most of these BlahNotExistErrs. We can create a common detailed NotExistErr if necesary:

type DetailedNotExist struct {
  Resource interface{}
}

func (d DetailedNotExist) Error() string {
   // This would use reflection... to reveal the type of the Resource and which non-empty fields are set
}

func (d DetailedNotExist) Unwrap() error {
   return NotExistErr
}

@6543
Copy link
Member

6543 commented Oct 18, 2022

🚀

@6543 6543 merged commit 716fcfc into go-gitea:main Oct 18, 2022
@zeripath zeripath deleted the make-every-not-exist-err-a-not-exist-err branch October 18, 2022 18:29
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 19, 2022
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Remove unnecessary misspell ignore pattern (go-gitea#21475)
  Fix read system configuration bug when installing (go-gitea#21489)
  Fix viewing user subscriptions (go-gitea#21482)
  Make every not exist error unwrappable to a fs.ErrNotExist (go-gitea#20891)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants