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

Remove unspecified error? #12

Open
jutuon opened this issue Jun 13, 2024 · 3 comments
Open

Remove unspecified error? #12

jutuon opened this issue Jun 13, 2024 · 3 comments

Comments

@jutuon
Copy link

jutuon commented Jun 13, 2024

I received this FcmResponse

FcmResponse {
    http_status_code: 404,
    response_json_object: {
        "error": Object {
            "code": Number(404),
            "message": String("Requested entity was not found."),
            "status": String("NOT_FOUND"),
            "details": Array [
                Object {
                    "@type": String("type.googleapis.com/google.firebase.fcm.v1.FcmError"),
                    "errorCode": String("UNREGISTERED"),
                },
            ],
        },
    },
    retry_after: None,
}

That is FcmError enum variant UNREGISTERED. If similar JSON response is also used for enum variant UNSPECIFIED_ERROR then current code detects FcmResponseError::Unknown and not the correct FcmResponseError::Unspecified.

The FCM v1 API docs do not have exact JSON location for the FcmError. Only the successful JSON respose is documented for send HTTP route. Because of that I think it would be the best to remove the FcmResponseError::Unspecified. Should I do PR which does that?

@rj76
Copy link
Owner

rj76 commented Jun 14, 2024

From what I understand from the docs is that the response of the UNSPECIFIED_ERROR doesn't contain a http status code and will be handled in the next if check returning FcmResponseError::Unspecified and not Unknown. You're suggesting this if will never be reached or succeed?

EDIT: it always has a http status code, it's just not known in that case...

@jutuon
Copy link
Author

jutuon commented Jun 14, 2024

You're suggesting this if will never be reached or succeed?

I can't say never as API docs are incomplete, but if UNSPECIFIED_ERROR has same JSON location as the UNREGISTERED, then the if condition for FcmResponseError::Unspecified will evaluate to false (and the next if condition most likely will be true, so FcmResponseError::Unknown is returned).

The point which I'm trying to make is that API which can't be implemented reliably (because the current FCM API docs are incomplete) should be removed. From library user standpoint the FcmResponseError::Unspecified and FcmResponseError::Unknown already are equivelent as there is no clear error handling case for those (perhaps retrying sending after a while might be best).

@rj76
Copy link
Owner

rj76 commented Jun 17, 2024

I would also have implemented it this way from the docs, but yeah you make a valid point in those being unclear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants