fix: Handle GRPC Error Codes. #42

gautamomento · 2022-01-29T15:32:25Z

Fix for #23, #24 and #27

Based on https://www.notion.so/momentohq/GRPC-Status-Code-Mapping-35f76a4abc634e26b1129a649e1b329d

Fix for #23 - CacheValueError is a confusing name for surfacing INVALID_ARGUMENT errors. It can be easily confused with something related to setting a value in Cache. Proposing `CacheValueError` is renamed to `InvalidArgumentError` Fix for #24 - Given the type-safety in python, customers can accidentally pass unsupported types. This results in a ClientSdkException when in fact it is an Invalid Input Error. We already type check our keys, values, ttl, so just adding one for cache name. Though it may be just wise to let the type errors pass through.

gautamomento · 2022-01-31T16:08:26Z

I will be waiting for conclusion on https://www.notion.so/momentohq/GRPC-Status-Code-Mapping-35f76a4abc634e26b1129a649e1b329d

tylerburdsall · 2022-02-01T17:26:04Z

src/momento/_utilities/_data_validation.py

+    if cache_name is None or not isinstance(cache_name, str):
+        raise errors.InvalidArgumentError('Cache name must be a non-None value with `str` type')


Minor nit: it's a little more Pythonic to check non-null via:

if not cache_name or not isistance(cache_name, str)

Sweet. Thank you.

I actually ended up keeping this as not check blocks '' empty strings. That is fine but just breaks the internal decision that we only perform client SDK validations that can't be done on the server side.

rlinehan

A few places where the doc strings need to be updated, but otherwise looks good!

src/momento/errors.py

rlinehan · 2022-02-01T19:06:05Z

src/momento/simple_cache_client.py

            CreateCacheResponse

        Raises:
            InvalidInputError: If cache name is None.


I think this should be removed now

rlinehan · 2022-02-01T19:06:18Z

src/momento/simple_cache_client.py


        Raises:
            CacheNotFoundError: If an attempt is made to delete a MomentoCache that doesn't exits.
            InvalidInputError: If cache name is None.


I think this should be removed now.

rlinehan · 2022-02-01T19:06:33Z

src/momento/simple_cache_client.py

            ClientSdkError: For any SDK checks that fail.
-            CacheValueError: If provided cache_name is empty.
+            InvalidArgumentError: If provided cache_name is empty.
            CacheExistsError: If cache with the given name already exists.


Suggested change

CacheExistsError: If cache with the given name already exists.

AlreadyExistsError: If cache with the given name already exists.

rlinehan · 2022-02-01T19:08:01Z

src/momento/simple_cache_client.py

            DeleteCacheResponse

        Raises:
            CacheNotFoundError: If an attempt is made to delete a MomentoCache that doesn't exits.


Suggested change

CacheNotFoundError: If an attempt is made to delete a MomentoCache that doesn't exits.

NotFoundError: If an attempt is made to delete a MomentoCache that doesn't exits.

…nt to check on SDK

bruuuuuuuce · 2022-02-01T19:46:50Z

src/momento/_cache_service_errors_converter.py

-    grpc.StatusCode.ALREADY_EXISTS: errors.CacheExistsError,
-    grpc.StatusCode.INVALID_ARGUMENT: errors.CacheValueError,
-    grpc.StatusCode.NOT_FOUND: errors.CacheNotFoundError,
+    grpc.StatusCode.INVALID_ARGUMENT: errors.BadRequestError,


should this be errors.InvalidArgumentError?

Nope. These are the naming conventions we will be following
https://www.notion.so/momentohq/GRPC-Status-Code-Mapping-35f76a4abc634e26b1129a649e1b329d

The second column:
BadRequest*

For SDK client side checks we will be using
InvalidArgument*

Co-authored-by: Ruth Linehan <1530016+rlinehan@users.noreply.github.com>

src/momento/simple_cache_client.py

rlinehan · 2022-02-01T21:25:29Z

src/momento/_utilities/_data_validation.py

-    if cache_name is None:
-        raise errors.InvalidInputError('Cache Name cannot be None')
+    if cache_name is None or not isinstance(cache_name, str):
+        raise errors.InvalidArgumentError('Cache name must be a non-None value with `str` type')


this is a nitpick that you can feel free ignore - I feel like saying a non-None value with str type is redundant, since str is not None? So could just say must be a non-empty string.

Co-authored-by: Ruth Linehan <1530016+rlinehan@users.noreply.github.com>

…lient-sdk-python into u/gautamomento/errorhandling

rlinehan

gautamomento added 2 commits January 29, 2022 07:25

fix broken error

86e2901

gautamomento requested review from rlinehan and removed request for rlinehan January 31, 2022 15:06

gautamomento marked this pull request as draft January 31, 2022 15:15

fix tests

a718abc

gautamomento force-pushed the u/gautamomento/errorhandling branch from 5340927 to a718abc Compare February 1, 2022 16:00

gautamomento added 2 commits February 1, 2022 08:00

rename errors

5ad2c40

remove unwanted import

38a961c

This was linked to issues Feb 1, 2022

CacheValueError name is confusing #23

Closed

Errors for invalid input are inconsistent #24

Closed

Handle grpc status codes (e.g. RESOURCE_EXHAUSTED) #27

Closed

gautamomento marked this pull request as ready for review February 1, 2022 17:22

gautamomento requested review from bruuuuuuuce, rlinehan and tylerburdsall February 1, 2022 17:22

tylerburdsall reviewed Feb 1, 2022

View reviewed changes

gautamomento changed the title ~~fix: Rename Error and tighten cache name validations~~ fix: Handle GRPC Error Codes. Feb 1, 2022

update check

a2b8400

rlinehan reviewed Feb 1, 2022

View reviewed changes

fix: use None check, not seems to checking empty str that we don't wa…

77aaab7

…nt to check on SDK

bruuuuuuuce reviewed Feb 1, 2022

View reviewed changes

gautamomento and others added 5 commits February 1, 2022 11:53

fix: doc strings

6d2a0e2

Update src/momento/errors.py

37dd8fa

Co-authored-by: Ruth Linehan <1530016+rlinehan@users.noreply.github.com>

fix docstrings

2ac9333

rename exception

ffa2e11

update docstring

d0cb7ae

gautamomento requested review from rlinehan and tylerburdsall February 1, 2022 21:15

rlinehan previously approved these changes Feb 1, 2022

View reviewed changes

tylerburdsall previously approved these changes Feb 1, 2022

View reviewed changes

Update src/momento/simple_cache_client.py

09a4f68

Co-authored-by: Ruth Linehan <1530016+rlinehan@users.noreply.github.com>

gautamomento dismissed stale reviews from tylerburdsall and rlinehan via 09a4f68 February 1, 2022 21:28

rlinehan previously approved these changes Feb 1, 2022

View reviewed changes

gautamomento added 2 commits February 1, 2022 13:29

fix error message

e4e9718

Merge branch 'u/gautamomento/errorhandling' of github.com:momentohq/c…

6c05a57

…lient-sdk-python into u/gautamomento/errorhandling

gautamomento dismissed rlinehan’s stale review via 6c05a57 February 1, 2022 21:29

gautamomento requested a review from rlinehan February 1, 2022 21:31

rlinehan approved these changes Feb 1, 2022

View reviewed changes

gautamomento merged commit 8f68a1f into main Feb 1, 2022

gautamomento deleted the u/gautamomento/errorhandling branch February 1, 2022 21:33

		if cache_name is None or not isinstance(cache_name, str):
		raise errors.InvalidArgumentError('Cache name must be a non-None value with `str` type')

	CacheExistsError: If cache with the given name already exists.
	AlreadyExistsError: If cache with the given name already exists.

	CacheNotFoundError: If an attempt is made to delete a MomentoCache that doesn't exits.
	NotFoundError: If an attempt is made to delete a MomentoCache that doesn't exits.

fix: Handle GRPC Error Codes. #42

fix: Handle GRPC Error Codes. #42

Uh oh!

Conversation

gautamomento commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gautamomento commented Jan 31, 2022

Uh oh!

tylerburdsall Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rlinehan 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlinehan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gautamomento commented Jan 29, 2022 •

edited

Loading

tylerburdsall Feb 1, 2022 •

edited

Loading