Migrating FCM Send APIs to the New Exceptions #297

hiranya911 · 2019-06-13T21:04:11Z

Introduced new FCM-specific error types in the messaging module.
Updated the shared error handling code in _utils to handle platform errors.
Implemented logic for handling errors raised by requests and googleapiclient packages (latter used by FCM batch operations).
Updated FCM send APIs to raise new exception types.
Updated affected tests and docs.

bklimt

This seems mostly fine, but that error parsing code sure is convoluted. Maybe we could find a cleaner way to refactor it?

firebase_admin/_utils.py

bklimt · 2019-06-14T04:14:37Z

firebase_admin/_utils.py

+    This can be used to handle errors returned by Google Cloud Platform (GCP) APIs.
+
+    Args:
+        error: An error raised by the reqests module while making an HTTP call to a GCP API.


s/reqests/requests/

bklimt · 2019-06-14T04:15:30Z

firebase_admin/_utils.py

+        error: An error raised by the reqests module while making an HTTP call to a GCP API.
+        handle_func: A function that can be used to handle platform errors in a custom way. When
+            specified, this function will be called with four arguments -- parsed error response,
+            error message, source exception from requests and the HTTP response object.


Personally, I'd prefer an oxford comma in there. :-) But more seriously, please make this a bulleted list so that it's easier to scan what the four arguments are.

bklimt · 2019-06-14T04:22:58Z

firebase_admin/_utils.py

+    if handle_func:
+        exc = handle_func(error_dict, message, error, error.response)
+
+    return exc if exc else handle_requests_error(error, message, code)


If might be a little cleaner than all this if you can make handle_requests_error have the same format as handle_func. The advantages would be:

You could have the handle_func default to handle_requests_error and not have any of this branching code, and

It would be easier to document handle_func by pointing to handle_requests_error as an example implementation.

I've sort of done both, but not quite the way described in your comment. PTAL.

bklimt · 2019-06-14T04:27:55Z

firebase_admin/_utils.py

    """Constructs a ``FirebaseError`` from the given requests error.

    Args:
        error: An error raised by the reqests module while making an HTTP call.
        message: A message to be included in the resulting ``FirebaseError`` (optional). If not
            specified the string representation of the ``error`` argument is used as the message.
-        status: An HTTP status code that will be used to determine the resulting error type
-            (optional). If not specified the HTTP status code on the error response is used.
+        code: An HTTP status code or GCP error code that will be used to determine the resulting


Yeah, it still seems sketchy to me to mix types here. Would it be possible to make this one or the other?

It occurs to me that handle_platform_error_from_requests extracts code, which is then ignored if a handle_func is set. Maybe if handle_requests_error is a handle_func, then the code doesn't need to be passed in, and handle_requests_error could call _parse_platform_error itself?

I'm not sure if that's a good idea, but it's worth thinking about.

handle_requests_error is supposed to be service-agnostic. I expect to use it for non-OP errors as well (we already do so in the instance_id module, which is a non-OP endpoint). So I extracted the OP-specific parsing to a new helper method named _handle_func_requests and made its signature consistent with handle_func.

But I do still need some branching since we allow handle_func to return None.

bklimt · 2019-06-14T04:29:59Z

firebase_admin/_utils.py

+
+
+def _lookup_error_type(code):
+    """Maps an error code to an exception type."""


Please document what error code means here.

hiranya911 · 2019-06-14T20:11:42Z

Thanks for the review @bklimt. This is ready for another look.

bklimt

LGTM. Thanks for your patience.

* Introduced the exceptions module (#296) * Added the exceptions module * Cleaned up the error handling logic; Added tests * Updated docs; Fixed some typos * Migrating FCM Send APIs to the New Exceptions (#297) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated remaining messaging APIs to new error types (#298) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Introducing TokenSignError to represent custom token creation errors (#302) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Migrated custom token API to new error types * Raising FirebaseError from create_session_cookie() API (#306) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Migrated custom token API to new error types * Migrated create cookie API to new error types * Improved error message computation * Refactored the shared error handling code * Fixing lint errors * Renamed variable for clarity * Introducing UserNotFoundError type (#309) * Added UserNotFoundError type * Fixed some lint errors * Some formatting updates * Updated docs and tests * New error handling support in create/update/delete user APIs (#311) * New error handling support in create/update/delete user APIs * Fixing some lint errors * Error handling improvements in email action link APIs (#312) * New error handling support in create/update/delete user APIs * Fixing some lint errors * Error handling update in email action link APIs * Project management API migrated to new error types (#314) * Error handling updated for remaining user_mgt APIs (#315) * Error handling updated for remaining user_mgt APIs * Removed unused constants * Migrated token verification APIs to new exception types (#317) * Migrated token verification APIs to new error types * Removed old AuthError type * Added new exception types for revoked tokens * Migrated the db module to the new exception types (#318) * Migrating db module to new exception types * Error handling for transactions * Updated integration tests * Restoring the old txn abort behavior * Updated error type in snippet * Added comment * Adding a few overlooked error types (#319) * Adding some missing error types * Updated documentation * Removing the ability to delete user properties by passing None (#320) * Some types renamed to be PEP8 compliant (#330) * Upgraded Cloud Firestore and Cloud Storage dependencies (#325) * Added documentation for error codes (#339) * A few API doc updates (#340) * Added documentation for error codes * Updated API docs

* Introduced the exceptions module (#296) * Added the exceptions module * Cleaned up the error handling logic; Added tests * Updated docs; Fixed some typos * Migrating FCM Send APIs to the New Exceptions (#297) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated remaining messaging APIs to new error types (#298) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Introducing TokenSignError to represent custom token creation errors (#302) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Migrated custom token API to new error types * Raising FirebaseError from create_session_cookie() API (#306) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Migrated custom token API to new error types * Migrated create cookie API to new error types * Improved error message computation * Refactored the shared error handling code * Fixing lint errors * Renamed variable for clarity * Introducing UserNotFoundError type (#309) * Added UserNotFoundError type * Fixed some lint errors * Some formatting updates * Updated docs and tests * New error handling support in create/update/delete user APIs (#311) * New error handling support in create/update/delete user APIs * Fixing some lint errors * Error handling improvements in email action link APIs (#312) * New error handling support in create/update/delete user APIs * Fixing some lint errors * Error handling update in email action link APIs * Project management API migrated to new error types (#314) * Error handling updated for remaining user_mgt APIs (#315) * Error handling updated for remaining user_mgt APIs * Removed unused constants * Migrated token verification APIs to new exception types (#317) * Migrated token verification APIs to new error types * Removed old AuthError type * Added new exception types for revoked tokens * Migrated the db module to the new exception types (#318) * Migrating db module to new exception types * Error handling for transactions * Updated integration tests * Restoring the old txn abort behavior * Updated error type in snippet * Added comment * Adding a few overlooked error types (#319) * Adding some missing error types * Updated documentation * Removing the ability to delete user properties by passing None (#320) * Adding beginning of _MLKitService (#323) * Adding beginning of _MLKitService * Added License and Docstring * Firebase ML Kit Get Model API implementation (#326) * added GetModel * Added tests for get_model * Firebase ML Kit Delete Model API implementation (#327) * implement delete model * Firebase ML Kit List Models API implementation (#331) * implemented list models plus tests * Implementation of Model, ModelFormat, TFLiteModelSource and subclasses (#335) * Implementation of Model, ModelFormat, ModelSource and subclasses * Firebase ML Kit Create Model API implementation (#337) * create model plus long running operation handling * Model.wait_for_unlocked * Firebase ML Kit Update Model API implementation (#343) * Firebase ML Kit Create Model API implementation * Firebase ML Kit Publish and Unpublish Implementation (#345) * Firebase ML Kit Publish and Unpublish Implementation * Firebase ML Kit TFLiteGCSModelSource.from_tflite_model implementation and conversion helpers (#346) * Firebase ML Kit TFLiteGCSModelSource.from_tflite_model implementation * support for tensorflow lite conversion helpers (version 1.x) * Quick pass at filling in missing docstrings (#367) * Quick pass at filling in missing docstrings * More punctuation * Modify Operation Handling to not require a name for Done Operations (#371) * Firebase ML Kit Modify Operation Handling to not require a name for Done Operations * Adding support for TensorFlow 2.x * rename from mlkit to ml (#373) * Adding File naming capability to from_saved_model and from_keras_model. (#375) adding File naming capability for ModelSource * Firebase ML Modify Operation Handling Code to match rpc codes not html codes (#390) * Firebase ML Modify Operation Handling Code to match actual codes * apply database fix too * Mlkit fix date handling2 (#391) * Fix create/update date handling * Skip unrelated failing tests (until sync) * Firebase Ml Fix upload file naming (#392) * Fix File Naming * Integration tests for Firebase ML (#394) * Integration tests for Firebase ML * Fixing lint errors for Py3 (#401) * Fixing lint errors for Py3 * Removed dependency on six * Fixing a couple of merge errors * Modifying operation handling to support backend changes (#423) * modifying operation handling to support backend changes * Firebase ML Changing service endpoint (#421) * Mlkit add headers (#445) * add Headers * fixed test (#448) * Adding tensorflow and keras so we don't skip tests (#449) * Adding tensorflow and keras so we don't skip tests * Add additional instructions for integration tests for ml Co-authored-by: Hiranya Jayathilaka <[email protected]> Co-authored-by: Kevin Cheung <[email protected]>

hiranya911 added 7 commits June 12, 2019 16:48

Migrated FCM send APIs to the new error handling regime

3218695

Moved error parsing logic to _utils

e734cf9

Refactored OP error handling code

8df345a

Fixing a broken test

ce4a789

Added utils for handling googleapiclient errors

3b6b78d

Added tests for new error handling logic

20d2e87

Updated public API docs

c0cf6f7

hiranya911 requested a review from bklimt June 13, 2019 21:07

hiranya911 assigned bklimt Jun 13, 2019

Fixing test for python3

0ea0f82

bklimt reviewed Jun 14, 2019

View reviewed changes

Cleaning up the error code lookup code

130a42f

hiranya911 assigned hiranya911 and unassigned bklimt Jun 14, 2019

hiranya911 added 2 commits June 14, 2019 11:54

Cleaning up the error parsing APIs

de29a6b

Cleaned up error parsing logic; Updated docs

c536f5b

hiranya911 assigned bklimt and unassigned hiranya911 Jun 14, 2019

bklimt approved these changes Jun 20, 2019

View reviewed changes

hiranya911 assigned hiranya911 and unassigned bklimt Jun 20, 2019

hiranya911 merged commit 2879a22 into error-handling-revamp Jun 20, 2019

hiranya911 deleted the hkj-fcm-errors branch June 20, 2019 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrating FCM Send APIs to the New Exceptions #297

Migrating FCM Send APIs to the New Exceptions #297

Uh oh!

hiranya911 commented Jun 13, 2019

Uh oh!

bklimt left a comment

Uh oh!

Uh oh!

bklimt Jun 14, 2019

Uh oh!

hiranya911 Jun 14, 2019

Uh oh!

bklimt Jun 14, 2019

Uh oh!

bklimt Jun 14, 2019

Uh oh!

hiranya911 Jun 14, 2019

Uh oh!

bklimt Jun 14, 2019

Uh oh!

hiranya911 Jun 14, 2019

Uh oh!

bklimt Jun 14, 2019

Uh oh!

hiranya911 Jun 14, 2019

Uh oh!

hiranya911 commented Jun 14, 2019

Uh oh!

bklimt left a comment

Uh oh!

Uh oh!



		def _lookup_error_type(code):
		"""Maps an error code to an exception type."""

Migrating FCM Send APIs to the New Exceptions #297

Migrating FCM Send APIs to the New Exceptions #297

Uh oh!

Conversation

hiranya911 commented Jun 13, 2019

Uh oh!

bklimt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Jun 14, 2019

Uh oh!

bklimt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!