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

Refactoring packages error processing #27979

Closed
wants to merge 8 commits into from
Closed

Refactoring packages error processing #27979

wants to merge 8 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 9, 2023

Unifyed approach to form response codes from gitea errors in packages module.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 9, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 9, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Nov 9, 2023
Comment on lines 29 to 31
ErrQuotaTypeSize = util.NewInvalidArgumentErrorf("maximum allowed package type size exceeded")
ErrQuotaTotalSize = util.NewInvalidArgumentErrorf("maximum allowed package storage quota exceeded")
ErrQuotaTotalCount = util.NewInvalidArgumentErrorf("maximum allowed package count exceeded")
Copy link
Member

Choose a reason for hiding this comment

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

Is "you have reached a limit" an "argument error"?

Copy link
Author

@ghost ghost Nov 10, 2023

Choose a reason for hiding this comment

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

@KN4CK3R

No. Added draft with FailedPrecondition (similar with gRPC spec) error in this branch. Would it be valid for current scenario?

Copy link
Member

Choose a reason for hiding this comment

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

Is it a mistake that only ErrQuotaTotalCount is a util.NewFailedPreconditionErrorf?

@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 14, 2023
@ghost ghost changed the title packages: Simpler approach to handle errors on package upload operation Refactoring error processing Nov 14, 2023
@ghost ghost changed the title Refactoring error processing Refactoring packages error processing Nov 14, 2023
@ghost ghost marked this pull request as draft November 14, 2023 03:25
@ghost ghost marked this pull request as ready for review November 14, 2023 03:29
@ghost ghost requested a review from KN4CK3R November 15, 2023 19:34
@ghost
Copy link
Author

ghost commented Nov 22, 2023

@KN4CK3R

I added some corrections and simplifactions to enhance the error handling process.

  1. Added ErrPayloadTooLarge and ErrLimitExceeded to Gitea errors.
  2. Added functions that get response codes from shared Gitea errors.
  3. Added variadic syntax for response codes to apiError(...) functions in all package types to make the response code an optional parameter (should keep the existing flexible format but make error assignment simpler).
  4. Added logging to errors in the container module; unified error format for all container module responses.
  5. Changed status code for file size limits errors to 413 Payload Too Large and user reached limit errors to 409 Conflict

I am not sure about this implementation or whether this approach might be applicable. This model might be useful because different internal error changes will be handled automatically, potentially reducing the number of accidental errors, but it requires additional attention to error messages since those messages might be passed to the client side.

@ghost ghost closed this Jan 18, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 8, 2024
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants