feat(firebase_auth): add AuthExceptionStatusCode to FirebaseAuthException

[KristianBalaj]

Description

The FirebaseAuthException codes should be static typed to prevent any typo bugs in the client code. Therefore there should be an enum that ensures the AuthException type.

At the same time, this static typed code of the exception serves as an implicit documentation of the possible states of an FirebaseAuthException.

Related Issues

#3273

Checklist

• creating AuthExceptionStatusCode enum type.
• add parsing of the exception string codes to the FirebaseAuthException class via the statusCode getter method.

Breaking Change

• No, this is not a breaking change.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[KristianBalaj]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[helenaford]

Thanks for updating the class, it looks good although not sure if you would need e.code and e.statusCode (maybe you do). Would be good to see an example of how a user would use this class, assuming it would be exposed to the client? I think there maybe some exceptions missing such as invalid-verification-id, no-current-user, weak-password, email-already-in-use, invalid-phone-number. And, the tests in instance-e2e.dart should be updated to use AuthExceptionStatusCode.

[KristianBalaj]

@helenaford Thanks for the response!

Sure the tests. I was asking myself, the class isn't covered? I was working with the root as packages/firebase_auth/firebase_auth_platform_interface so when searching the references, nothing has been found in the tests locally.
I was reading the contribution guide from current version of master. After a bit of searching, I found the actual contribution guide #2963. (for now still only in draft). Now I'm on the right track 😇

I will also add the missing codes that I will find in the tests 👍

The actual usage of the enum instead of the string is the usage in type checked manner (eg. switch statement) which could be used in the catch block. Also the code could be forwarded to any other method, but still it is type checked and the client code is aware of the possible codes/states.

And of course I need to export the AuthExceptionStatusCode enum.

I would propose to move the e.code under the @deprecated attribute in favor of e.statusCode. In case of localization purposes of the code as a key the usage could be like e.statusCode.toString().

[Ehesp]

I'm in two minds about this one. The problem we have is currently there is 3 platforms (and potentially more in the future) all using shared codes and their own platforms ones.

Right now, we document common error codes returned and "stringify" the platform codes into a common format (e.g. "operation-not-allowed"). Generally if the underlying platform errors you can still read out the .code property and expect to handle it.

If we go down the route of adding enums, it means we have to add every single possibility for users to correctly handle them, and there is a LOT of codes. If Firebase update or add a new error code, we need also need to add this enum. If we add a new platform, we have to add every new error code (which isn't always obvious).

There's also the case of some error codes not being applicable (e.g. missing-phone-number on web via signInWithPhoneNumber). We assert that there must be a phone number in Dart anyway, so this will never be returned.

Personally I think the docs should just say reference the JavaScript API for a list of possible errors (which also may not be fully correct since we have Android/iOS in play).

TLDR; Right now we just let errors fall through to users in a formatted string, this change (or similar) would require us to know about every single error code and where they can pop up (which isn't easy to find and is error prone).

[feinstein]

@Ehesp I completely understand your reasoning, and had some suspicions things were the way they are because of the underlining platforms/firebase libs, so I don't disagree with you.

I just wish things were different and properly documented, as in our App we like to give users meaningful and personal erros and alerts. We believe this improves user experience and reduces frustration when an error happens.

Having just a catch-all "Ops, something went wrong, please contact customer service" isn't our goal, but unfortunately I see there's no escape from this, unless the underlying libraries provide better documentation.

[leonardocustodio]

@Ehesp, @feinstein Well I totally disagree.... Having error codes as plain string is horrible for people using the package. Firebase changed the errors to lower case and my apps stopped catching the errors properly and probably everybody else as well. It is way easier for people developing the package to know when error codes has changed because they are working in the package and more up to date with changes on firebase than developers that are just using the package.

I would suggest at least if the error codes change in the server side and you guys don't know about it that returned some kind of "unknown" enum temporarily until changing all error codes to the right ones and updating in the package.

I mean should the developers really keep checking from now and than if the error codes haven't changed and the app stopped working and than update all apps with new strings? Or just update a package when they change?

[Ehesp]

@leonardocustodio the PR is still open and being discussed so you're welcome to an opinion.

I personally think we should have both a string version and enum value, however if the string value doesn't match a handled enum, it returns unknown. I think removing the string value like this PR will mask any unhandled errors rather than leave them open to implementation.

Leaving this one open still, I'll get round to it at some point if no one carried it on.

[leonardocustodio]

Maybe could we make an extra option? The enum for general case and leave the string for more specific ones? I don't know. Not perfect but is one way to go?

[Ehesp]

I'm open to having:

FirebaseAuthException.code = 'permission-denied'
+
FirebaseAuthException.status = AuthExceptionStatusCode.permissionDenied

[leonardocustodio]

That would be nice. Or another option would be to have a helper that we could pass the value and it would return the enum.
So in the case the enum returned from the helper is unknown the person would still have the string in the first place.

Just a code snippet from RevenueCat library on how they handle the errors as example:

  static PurchasesErrorCode getErrorCode(PlatformException e) {
    var errorCode = int.parse(e.code);
    if (errorCode >= PurchasesErrorCode.values.length) {
      return PurchasesErrorCode.unknownError;
    }
    return PurchasesErrorCode.values[errorCode];
  }

But I think it is easier to have both values (code + status) like you said.

[feinstein]

@leonardocustodio AFAIK the status codes changed after a new version, with breaking changes documented, was announced.

I always check the Changelog for breaking changes before updating any library references, so I wasn't caught by surprise that my exceptions stopped working. Still it was very frustrating as well and that's why I opened the issue linked with this PR.

I suggest you orient your team to only update libraries after checking the Changelogs, this helps a lot in keeping surprises to the minimum.

I also like the ideas of enums, but I also agree there has to be some "catch all" code since the folks at Firebase don't have this properly documented and set in stone, so these suggestions seem positive IMO.

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[ollyde]

@feinstein not every developer checks the change logs. Infact I don't know any that do because usually we relay on 'deprecated flags'. This bug has actually caused a ton of silient errors across many Flutter apps using Firebase now; not good.

[feinstein]

@OllyDixon you can't put deprecated flags on exception strings or other values that just change the way they are returned and not their name. You can't put deprecated annotations on old behaviors, just on old names.

I know most people don't check the changelogs, but that's plain wrong. How else would they measure the impact of a change? You need to check for every major version change at least, by definition they might include breaking changes.

I am not arguing we don't need exception codes, just that "people don't check the changelogs" is an argument as strong as "people don't do unit tests". We can't guide our decisions based on bad practices that others are adopting.

[ollyde]

There are soltions for deprecation on this; I can think of a few that could have been implemented; perhaps they will be moving forwards with enums etc.
@feinstein try managing 31 projects. We don't have time to check the changelogs on 1000s of updates and tbh I'm sort of surprised you think anyone reads them and that is a good solution! 🙃 on most of the packages though pub.dev there aren't even changelogs. :-)

[feinstein]

All the packages that I use have changelogs.

If you have to manage 31 projects, than you are an exception on the field. Most teams won't have more than a handful of projects. I suggest you have a quality control were versions are vetted once and then propagated to your other projects. So your projects only use pre-approved versions, with migration notes.

I do think changelogs are amazing, I can adapt my code to the latest additions, I learn a lot from what the package has to offer. I am surprised actually that people don't look at them. If they are so useless, why bother wasting time writing them? Better remove them altogether.

Anyway, this is getting off topic and polluting the thread.

[ollyde]

@KristianBalaj, we implemented the new error codes across our projects. Will these be changed back, or are we safekeeping them? This is important, so customers do have a lousy experience again. We got a few 1-star reviews across our apps because of this issue.

[KristianBalaj]

Hey, just checking and I need to consent to the cla not only sign it.

Also there are still some formatting issues needed to be fixed and I can't right now (check the checks).

But not sure if it will be merged in the end by the maintainers.

[KristianBalaj]

@googlebot I consent.

[KristianBalaj]

Okay so obviously not my consent to googlebot :) @Ehesp

[ollyde]

@KristianBalaj does that mean the strings will be the same or there are enums?

[KristianBalaj]

This PR is about adding the enums that will be unchangable - static.

And will be covered with tests, so any upcomming changes should be covered.

As far as this will be merged by the maintainers. I am not the maintainer.

[ollyde]

@KristianBalaj perfect! I think enums is the best solution.

[Ehesp]

Yeah we're just trying to get messaging out of the door since we need to do some global version bumps. Once thats landed we'll get some PRs merged and released!

[kroikie]

I would be in favor of having enums while still providing the underlying code from the platform as a string. Given the number of platforms there is no way to have enums covering all the necessary exceptions. A catch-all unknown with the underlying code as a string seems to me to be a good compromise.