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

refactor(http)!: cleanup ApiError #1737

Open
wants to merge 6 commits into
base: old-next
Choose a base branch
from

Conversation

vilgotf
Copy link
Member

@vilgotf vilgotf commented May 15, 2022

It's other variants are outdated, and the message field is covered by the Display impl on Error.

The other variants were only useful prior to the API V8 error changes.
@github-actions github-actions bot added c-http Affects the http crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code. labels May 15, 2022
@7596ff 7596ff self-requested a review May 20, 2022 19:43
http/src/error.rs Outdated Show resolved Hide resolved
@7596ff 7596ff removed their request for review May 30, 2022 21:43
@zeylahellyer
Copy link
Member

I'm unsure I understand this PR. Doesn't this remove support for parsing field errors? https://discord.com/developers/docs/reference#error-messages

@vilgotf
Copy link
Member Author

vilgotf commented Jun 20, 2022

I'm unsure I understand this PR. Doesn't this remove support for parsing field errors? https://discord.com/developers/docs/reference#error-messages

ApiError has three variants:

  1. General has a message (String) and error code
  2. Ratelimited has a message (String), global (bool) and retry_after (f64)
  3. Message has an optional array of embed field errors

Let's analyze these fields. The error message is not something users are supposed to match on, and it's already provided through the Display implementation on Error. The message variant seems completely out of date.

This leaves the JSON error code, global and retry_after.
This PR keeps just global and retry_after in an optional Ratelimit struct, attached to ErrorType::Response. I suppose however that the JSON error code should be preserved too, and I propose that we keep ApiError but scale it down to:

enum ApiError {
    Code(u64),
    Ratelimit(Ratelimit),
}

struct Ratelimit {
    global: bool,
    retry_after: f64,
}

@vilgotf
Copy link
Member Author

vilgotf commented Jun 20, 2022

Also I dislike the api_error module, so I'll move everything under error

@vilgotf vilgotf changed the title refactor(http)!: only expose ratelimit info on response error refactor(http)!: cleanup ApiError Jun 20, 2022
@zeylahellyer
Copy link
Member

zeylahellyer commented Jun 30, 2022

The message variant seems completely out of date.

In what way is it out of date? If it is, shouldn't we get it up to date? These object and array error messages detailing form body errors are still returned by the API

@vilgotf
Copy link
Member Author

vilgotf commented Jun 30, 2022

I thought they were replaced/deprecated in API v8, other API error responses are also undocumented which makes it look unsupported to me.
But still this PR only removes the functionality of MessageApiError, which I highly doubt is used for any sort of error handling, especially since it should be handled by twilight-validate.

@zeylahellyer
Copy link
Member

zeylahellyer commented Jun 30, 2022

I thought they were replaced/deprecated in API v8, other API error responses are also undocumented which makes it look unsupported to me.

Quite the opposite, they were introduced in API v8. If you send this request:

echo '{"embeds": [{"type": "rich"}]}' | http post https://discord.com/api/v10/channels/:id/messages

You'll get this response:

{
    "code": 50035,
    "errors": {
        "embeds": {
            "0": {
                "description": {
                    "_errors": [
                        {
                            "code": "BASE_TYPE_REQUIRED",
                            "message": "This field is required"
                        }
                    ]
                }
            }
        }
    },
    "message": "Invalid Form Body"
}

This response is what the MessageApiError variant attempted to do, perhaps incorrectly. I would rather we fix that than remove it, as it is a documented structure in the API. I can understand if you're not enthusiastic about that, so I can do that if you'd like

@vilgotf
Copy link
Member Author

vilgotf commented Jun 30, 2022

So is the Array, Object and Response types stable? I.e. {"errors":{"<SOMETHING>":{"N":{"<SOMETHINGELSE>":{"_errors":[{"code":"<CODE>","message":"<MESSAGE>"}]}}}} is the stable representation of Array Errors (for some value of SOMETHING, SOMETHINGELSE, CODE, and MESSAGE and integer N)?

@vilgotf vilgotf requested a review from zeylahellyer July 19, 2022 15:38
@itohatweb itohatweb self-requested a review September 16, 2022 22:19
@zeylahellyer
Copy link
Member

Triage: deferring research to after 0.15

@zeylahellyer zeylahellyer added the w-do-not-merge PR is blocked or deferred label Feb 4, 2023
@vilgotf
Copy link
Member Author

vilgotf commented Jan 27, 2024

So is the Array, Object and Response types stable? I.e. {"errors":{"<SOMETHING>":{"N":{"<SOMETHINGELSE>":{"_errors":[{"code":"<CODE>","message":"<MESSAGE>"}]}}}} is the stable representation of Array Errors (for some value of SOMETHING, SOMETHINGELSE, CODE, and MESSAGE and integer N)?

This is more or less correct according to Discords OpenAPI spec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-http Affects the http crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code. w-do-not-merge PR is blocked or deferred
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants