Skip to content

[FR] Add a typed error code to TopicManagementResponse.Error #1254

Open

Description

@SeHyeonCho

Is your feature request related to a problem? Please describe.
I subscribe tokens to topics and wanted to handle the token-related failures (mainly cleaning up stale tokens). This was harder than I expected, because TopicManagementResponse.Error only exposes a string (getReason()), and the possible values come from several different places:

  • The values aren't documented. The Javadoc only says the reason is a non-null, non-empty string.
  • Since 9.11.0 (feat(fcm): Migrate topic management to FCM v1 API #1240), subscribeToTopic() uses the FCM v1 API, while the *Legacy() methods still use Instance ID error codes. So a stale token can be reported as registration-token-not-registered (from NOT_FOUND) or as unregistered (from MessagingErrorCode.UNREGISTERED).
  • The fallback uses toLowerCase() with the default locale, so with tr-TR, INVALID_ARGUMENT becomes ınvalıd-argument.

Describe the solution you'd like
A non-breaking addition:

  • A new enum TopicManagementErrorCode (e.g. REGISTRATION_TOKEN_NOT_REGISTERED, INVALID_ARGUMENT, TOO_MANY_TOPICS, ..., UNKNOWN).
  • TopicManagementResponse.Error#getErrorCode(), mapped from the raw server code so it doesn't depend on the locale. Unrecognized codes return UNKNOWN.
  • getReason() and toString() stay as they are.
for (TopicManagementResponse.Error error : response.getErrors()) {
  switch (error.getErrorCode()) {
    case REGISTRATION_TOKEN_NOT_REGISTERED:
      removeToken(tokens.get(error.getIndex()));
      break;
    default:
      logger.warn("Topic subscription failed: {}", error.getReason());
  }
}

Open questions:

  1. Should NOT_FOUND and UNREGISTERED both map to REGISTRATION_TOKEN_NOT_REGISTERED?
  2. Should getErrorCode() return UNKNOWN or null (like FirebaseMessagingException#getMessagingErrorCode())?

Describe alternatives you've considered

  • Only fixing the locale issue with toLowerCase(Locale.ROOT), but that's still string-based.
  • Exposing a FirebaseMessagingException per error like SendResponse#getException(), but the legacy path has no per-token exception.

Additional context
Related: #1072, #1073. I have a patch with unit tests ready (existing tests unchanged, mvn clean test passes on JDK 8 and 17). Happy to open a PR if this direction sounds okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions