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

feat: updating not found exception to give more information #12813

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bswhite1
Copy link

@bswhite1 bswhite1 commented May 21, 2024

Description

The standard exception was not very helpful. I could tell a file wasn't found, but no indication of why or what file. This uses the platformException.message that has the actual file path, and returns it to the user.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@bswhite1 bswhite1 changed the title improvement: updating not found exception to give more information feat: updating not found exception to give more information May 21, 2024
@russellwheatley
Copy link
Member

@bswhite1 - could you provide a code sample for the exception you're describing? Sounds like something we can test as well, it would be ideal to have an integration test 👌 . Thanks.

@russellwheatley russellwheatley added the blocked: customer-response Waiting for customer response, e.g. more information was requested. label May 22, 2024
@bswhite1
Copy link
Author

Added some debugging to the function.

This is the debug from the android sdk:
W/Firestore(18105): (24.11.0) [WriteStream]: (a26a33) Stream closed with status: Status{code=NOT_FOUND, description=No document to update: projects/project-name/databases/(default)/documents/CollectionA/DocumentB/CollectionC/missing_document, cause=null}.

This is the incoming platformException and it's fields:
I/flutter (18105): platformException.code : firebase_firestore
I/flutter (18105): platformException.details: {code: not-found, message: Some requested document was not found.}
I/flutter (18105): platformException.message: com.google.firebase.firestore.FirebaseFirestoreException: NOT_FOUND: No document to update: projects/project-name/databases/(default)/documents/CollectionA/DocumentB/CollectionC/missing_document

So given this data, the original returns: Some requested document was not found. and this would return No document to update: projects/project-name/databases/(default)/documents/CollectionA/DocumentB/CollectionC/missing_document

@google-oss-bot google-oss-bot added Needs Attention This issue needs maintainer attention. and removed blocked: customer-response Waiting for customer response, e.g. more information was requested. labels May 22, 2024
@russellwheatley
Copy link
Member

@bswhite1 - any chance you could write an integration test for this? Would be ideal to see if behaviour was the same across platforms. Thanks.

@bswhite1
Copy link
Author

Updated integration test.

@russellwheatley
Copy link
Member

@bswhite1 - You need to fix the analyse issues (trailing commas in the test). Also - macOS and iOS e2e are failing on that test update which needs resolving 🙏

@russellwheatley russellwheatley added blocked: customer-response Waiting for customer response, e.g. more information was requested. and removed Needs Attention This issue needs maintainer attention. labels Jul 26, 2024
@bswhite1
Copy link
Author

@russellwheatley I can't reproduce locally, so added some debug. If someone could start the workflow, that would be great.
https://github.com/firebase/flutterfire/actions/runs/10113353206

@google-oss-bot google-oss-bot added Needs Attention This issue needs maintainer attention. and removed blocked: customer-response Waiting for customer response, e.g. more information was requested. labels Jul 26, 2024
@bswhite1
Copy link
Author

I was able to get it the workflows running locally.
Seems to be some inconsistency between actual android firestore, emulated android firestore, emulated macOS firestore, and emulated ios firestore.

Actual android firestore:
platformException.code : firebase_firestore
platformException.details: {code: not-found, message: Some requested document was not found.}
platformException.message: com.google.firebase.firestore.FirebaseFirestoreException: NOT_FOUND: No document to update: projects/project-name/databases/(default)/documents/CollectionA/DocumentB/CollectionC/missing_document

emulated android firestore:
platformException.code: firebase_firestore
platformException.details: {code: not-found, message: Some requested document was not found.}
platformException.message: com.google.firebase.firestore.FirebaseFirestoreException: NOT_FOUND: no entity to update: app: "dev~flutterfire-e2e-tests"

emulated macOS firestore:
platformException.code: not-found
platformException.details: {message: Some requested document was not found., code: not-found}
platformException.message: Some requested document was not found.

emulated iOS firestore:
platformException.code: not-found
platformException.details: {message: Some requested document was not found., code: not-found}
platformException.message: Some requested document was not found.

Should I wrap that test with a TargetPlatform.android check? Or is this indicating a bug in macOS / iOS?

@bswhite1
Copy link
Author

@russellwheatley Do you have a preference on how to proceed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Attention This issue needs maintainer attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants