Improve error handling and documentation across the SDK

src/citrine/_rest/collection.py

@@ -18,7 +18,21 @@
 
 
 class Collection(Generic[ResourceType], Pageable):
-    """Abstract class for representing collections of REST resources."""
+    """Base class for server-backed resource collections.
+
+    A Collection provides CRUD operations for a specific
+    resource type. All collections support at minimum:
+
+    * ``get(uid)`` — fetch one resource by UID
+    * ``list()`` — paginate over all resources
+    * ``register(model)`` — create a new resource
+    * ``update(model)`` — update an existing resource
+    * ``delete(uid)`` — delete a resource
+
+    Concrete collections (e.g. ``ProjectCollection``,
+    ``DatasetCollection``) may add additional operations.
+
+    """
 
     _path_template: str = NotImplemented
     _dataset_agnostic_path_template: str = NotImplemented
@@ -50,23 +64,66 @@ def build(self, data: dict):
         """Build an individual element of the collection."""
 
     def get(self, uid: Union[UUID, str]) -> ResourceType:
-        """Get a particular element of the collection."""
+        """Fetch a single resource by its unique identifier.
+
+        Parameters
+        ----------
+        uid : UUID or str
+            The unique identifier of the resource.
+
+        Returns
+        -------
+        ResourceType
+            The resource object.
+
+        Raises
+        ------
+        ValueError
+            If ``uid`` is None (the object may not be
+            registered yet).
+        NotFound
+            If no resource with the given UID exists.
+
+        """
         if uid is None:
-            raise ValueError("Cannot get when uid=None.  Are you using a registered resource?")
+            raise ValueError(
+                "Cannot retrieve a resource with uid=None. "
+                "This usually means the object has not been "
+                "registered with the platform yet. Call "
+                ".register() first to obtain a server-assigned "
+                "UID.")
         path = self._get_path(uid)
         data = self.session.get_resource(path, version=self._api_version)
         data = data[self._individual_key] if self._individual_key else data
         return self.build(data)
 
     def register(self, model: CreationType) -> CreationType:
-        """Create a new element of the collection by registering an existing resource."""
+        """Create a new resource on the platform.
+
+        Parameters
+        ----------
+        model : ResourceType
+            The resource to register. After registration, the
+            returned object will have a platform-assigned UID.
+
+        Returns
+        -------
+        ResourceType
+            The registered resource with server-assigned fields.
+
+        Raises
+        ------
+        ModuleRegistrationFailedException
+            If the platform rejects the resource.
+
+        """
         path = self._get_path()
         try:
             data = self.session.post_resource(path, model.dump(), version=self._api_version)
             data = data[self._individual_key] if self._individual_key else data
             return self.build(data)
         except NonRetryableException as e:
-            raise ModuleRegistrationFailedException(model.__class__.__name__, e)
+            raise ModuleRegistrationFailedException(model.__class__.__name__, e) from e
 
     def list(self, *, per_page: int = 100) -> Iterator[ResourceType]:
         """
@@ -94,14 +151,39 @@ def list(self, *, per_page: int = 100) -> Iterator[ResourceType]:
                                         per_page=per_page)
 
     def update(self, model: CreationType) -> CreationType:
-        """Update a particular element of the collection."""
+        """Update an existing resource on the platform.
+
+        Parameters
+        ----------
+        model : ResourceType
+            The resource with updated fields. Must have a
+            valid ``uid`` (i.e., be previously registered).
+
+        Returns
+        -------
+        ResourceType
+            The updated resource as returned by the server.
+
+        """
         url = self._get_path(model.uid)
         updated = self.session.put_resource(url, model.dump(), version=self._api_version)
         data = updated[self._individual_key] if self._individual_key else updated
         return self.build(data)
 
     def delete(self, uid: Union[UUID, str]) -> Response:
-        """Delete a particular element of the collection."""
+        """Delete a resource by its unique identifier.
+
+        Parameters
+        ----------
+        uid : UUID or str
+            The unique identifier of the resource to delete.
+
+        Returns
+        -------
+        Response
+            The server response.
+
+        """
         url = self._get_path(uid)
         data = self.session.delete_resource(url, version=self._api_version)
         return Response(body=data)

src/citrine/_rest/pageable.py

@@ -1,6 +1,9 @@
+from logging import getLogger
 from typing import Optional, Iterable, Dict, Tuple, Callable, Union, Sequence
 from uuid import UUID
 
+logger = getLogger(__name__)
+
 
 class Pageable():
     """Class that allows paging."""
@@ -87,6 +90,10 @@ def _fetch_page(self,
         try:
             next_uri = data.get('next', "")
         except AttributeError:
+            logger.warning(
+                "Response data is not a dict (type: %s); "
+                "pagination may be incomplete.",
+                type(data).__name__)
             next_uri = ""
 
         # A 'None' collection key implies response has a top-level array

src/citrine/_serialization/properties.py

@@ -95,9 +95,13 @@ def serialize(self, value: DeserializedType,
                   base_class: typing.Optional[type] = None) -> SerializedType:
         if not isinstance(value, self.underlying_types):
             base_name = self._error_source(base_class)
+            value_repr = repr(value)
+            if len(value_repr) > 100:
+                value_repr = value_repr[:100] + "..."
             raise ValueError(
-                f'{type(value)} {value} is not one of valid types: '
-                f'{self.underlying_types}{base_name}'
+                f'{type(value).__name__} is not one of valid '
+                f'types: {self.underlying_types}{base_name}. '
+                f'Value: {value_repr}'
             )
         return self._serialize(value)
 
@@ -107,9 +111,13 @@ def deserialize(self, value: SerializedType,
             if isinstance(value, self.underlying_types):
                 return value  # Don't worry if it was already deserialized
             base_name = self._error_source(base_class)
+            value_repr = repr(value)
+            if len(value_repr) > 100:
+                value_repr = value_repr[:100] + "..."
             raise ValueError(
-                f'{type(value)} {value} is not one of valid types: '
-                f'{self.serialized_types}{base_name}'
+                f'{type(value).__name__} is not one of valid '
+                f'types: {self.serialized_types}{base_name}. '
+                f'Value: {value_repr}'
             )
         return self._deserialize(value)
 
@@ -129,9 +137,16 @@ def deserialize_from_dict(self, data: dict) -> DeserializedType:
             next_value = value.get(field)
             if next_value is None:
                 if self.default is None and not self.optional:
-                    msg = "Unable to deserialize {} into {}, missing a required field: {}".format(
-                        data, self.underlying_types, field)
-                    raise ValueError(msg)
+                    data_preview = str(data)
+                    if len(data_preview) > 200:
+                        data_preview = data_preview[:200] + "..."
+                    raise ValueError(
+                        "Unable to deserialize into {}: "
+                        "missing required field '{}'. "
+                        "Data: {}".format(
+                            self.underlying_types,
+                            field, data_preview)
+                    )
                 # This occurs if a `field` is unexpectedly not present in the data dictionary
                 # or if its value is null.
                 # Use the default value and stop traversing, even if we have not yet reached

src/citrine/_session.py

@@ -19,6 +19,7 @@
     CitrineException,
     Conflict,
     NotFound,
+    ServerError,
     Unauthorized,
     UnauthorizedRefreshToken,
     WorkflowNotReadyException)
@@ -117,7 +118,16 @@ def _refresh_access_token(self) -> None:
                                             json=data)
 
         if response.status_code != 200:
-            raise UnauthorizedRefreshToken()
+            raise UnauthorizedRefreshToken(
+                "Failed to refresh authentication token.",
+                hint=(
+                    "Your API key may have expired or been "
+                    "revoked. Generate a new one at "
+                    "{}://{}/account/api-keys or set the "
+                    "CITRINE_API_KEY environment variable."
+                    .format(self.scheme, self.authority)
+                )
+            )
         self.access_token = response.json()['access_token']
         self.access_token_expiration = datetime.fromtimestamp(
             jwt.decode(
@@ -171,13 +181,13 @@ def checked_request(self, method: str, path: str,
                 self._refresh_access_token()
                 response = self._request_with_retry(method, uri, **kwargs)
         except AttributeError:
-            # Catch AttributeErrors and log response
-            # The 401 status will be handled further down
-            logger.error("Failed to decode json from response: {}".format(response.text))
+            # json() returned non-dict (e.g. list/string); 401 handled below
+            logger.debug("Response JSON is not a dict (status %s): %s",
+                         response.status_code, response.text[:200])
         except ValueError:
-            # Ignore ValueErrors thrown by attempting to decode json bodies. This
-            # might occur if we get a 401 response without a JSON body
-            pass
+            # Response has no JSON body (common for 401); 401 handled below
+            logger.debug("Response with status %s has no JSON body",
+                         response.status_code)
 
         if 200 <= response.status_code <= 299:
             logger.info('%s %s %s', response.status_code, method, path)
@@ -205,11 +215,17 @@ def checked_request(self, method: str, path: str,
                 raise Conflict(path, response)
             elif response.status_code == 425:
                 logger.debug('%s %s %s', response.status_code, method, path)
-                msg = 'Cant execute at this time. Try again later. Error: {}'.format(response.text)
+                msg = 'Cannot execute at this time. Try again later. Error: {}'.format(response.text)
                 raise WorkflowNotReadyException(msg)
             else:
                 logger.error('%s %s %s', response.status_code, method, path)
-                raise CitrineException(response.text)
+                raise ServerError(
+                    method=method, path=path,
+                    status_code=response.status_code,
+                    response_text=response.text,
+                    request_id=response.headers.get(
+                        'x-request-id')
+                )
 
     @staticmethod
     def _extract_response_stacktrace(response: Response) -> Optional[str]:
@@ -258,11 +274,11 @@ def _extract_response_json(path, response) -> dict:
                     lacked the required 'application/json' Content-Type in the header.""")
 
         except JSONDecodeError as err:
-            logger.info('Response at path %s with status code %s failed json parsing with'
-                        ' exception %s. Returning empty value.',
-                        path,
-                        response.status_code,
-                        err.msg)
+            logger.warning(
+                'Response at path %s with status code %s failed JSON '
+                'parsing (%s). Returning empty dict — downstream code '
+                'may behave unexpectedly.',
+                path, response.status_code, err.msg)
 
         return extracted_response
 

src/citrine/_utils/batcher.py

@@ -36,7 +36,11 @@ def batch(self, objects: Iterable[DataConcepts], batch_size) -> List[List[DataCo
         for obj in objects:
             if obj.to_link() in seen:  # Repeat in the iterable; don't add it to the batch
                 if seen[obj.to_link()] != obj:  # verify that it's a replicate
-                    raise ValueError(f"Colliding objects for {obj.to_link()}")
+                    raise ValueError(
+                        "Colliding objects for {}: two different "
+                        "objects share the same identifier. Ensure "
+                        "each object has a unique UID.".format(
+                            obj.to_link()))
             else:
                 by_type[obj.typ].append(obj)
                 for scope in obj.uids:
@@ -80,7 +84,16 @@ def batch(self, objects: Iterable[DataConcepts], batch_size) -> List[List[DataCo
                 local_set = {index.get(x, x) for x in depends[obj] if index.get(x, x) in obj_set}
                 full_set = set(local_set)
                 if len(full_set) > batch_size:
-                    raise ValueError(f"Object {obj.name} has more than {batch_size} dependencies.")
+                    sample = [getattr(d, 'name', str(d))
+                              for d in list(full_set)[:10]]
+                    raise ValueError(
+                        "Object '{}' has {} dependencies, "
+                        "exceeding batch_size={}. First {}: {}. "
+                        "Increase batch_size or simplify the "
+                        "dependency graph.".format(
+                            obj.name, len(full_set),
+                            batch_size, len(sample), sample)
+                    )
 
                 for subobj in local_set:
                     full_set.update(depends[subobj])

src/citrine/_utils/functions.py

@@ -32,8 +32,10 @@ def validate_type(data_dict: dict, type_name: str) -> dict:
     data_dict_copy = data_dict.copy()
     if 'type' in data_dict_copy:
         if data_dict_copy['type'] != type_name:
-            raise Exception(
-                "Object type must be {}, but was instead {}.".format(type_name, data_dict['type']))
+            raise TypeError(
+                "Object type must be '{}', but was '{}'. "
+                "Verify you are passing the correct object type."
+                .format(type_name, data_dict['type']))
     else:
         data_dict_copy['type'] = type_name
 
@@ -258,7 +260,7 @@ def migrate_deprecated_argument(
         if new_arg is None:
             return old_arg
         else:
-            raise ValueError(f"Cannot specify both \'{new_arg_name}\' and \'{new_arg_name}\'")
+            raise ValueError(f"Cannot specify both \'{new_arg_name}\' and \'{old_arg_name}\'")
     elif new_arg is None:
         raise ValueError(f"Please specify \'{new_arg_name}\'")
     return new_arg