[CIVIS-1683][CIVIS-5258] FIX responses are no longer mutable; support snake_case key access for nested responses

CHANGELOG.md

@@ -6,6 +6,18 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## Unreleased
 
+### Breaking Changes from v1.x.x to v2.0.0
+
+(Changes documented in this section are not repeated in the following sections.)
+
+- A `civis.response.Response` object is no longer mutable.
+  More concretely, both the "setitem" (e.g., `response["foo"] = "bar"`)
+  and "setattr" (e.g., `response.foo = "bar"`) operations
+  would now raise an `CivisImmutableResponseError` exception. (#463)
+- Instantiating a `civis.response.Response` object no longer
+  accepts the boolean `snake_case` keyword argument;
+  snake-case keys at a `Response` object are now always available (and preferred). (#463)
+
 ### Added
 - Added error handling of file_id with type string passed to `civis.io.civis_file_to_table`. (#454)
 - Added support for Python 3.10 and 3.11 (#462)
@@ -19,6 +31,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Fixed typo in "Testing Your Code" example of the User Guide (#458)
 - Adding try-except to catch JSONDecodeErrors in CivisAPIError (#459)
 - civis.io.file_id_from_run_output now works for all job types (#461)
+- A nested `civis.response.Response` object now supports both snake-case and camel-case
+  for key access. Previously, only the non-Pythonic camel-case keys were available. (#463)
 
 ### Deprecated
 ### Removed

civis/io/_files.py

@@ -94,7 +94,7 @@ def _single_upload(buf, name, client, **kwargs):
     # http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-post-example.html
     # Note that the payload must have "key" first and "file" last.
     url = file_response.upload_url
-    form = file_response.upload_fields
+    form = file_response.upload_fields._data_snake
     form_key = OrderedDict(key=form.pop('key'))
     form_key.update(form)
 

civis/ml/tests/test_model.py

@@ -747,7 +747,7 @@ def test_modelpipeline_classmethod_constructor_errors(mock_future):
         _model.ModelPipeline.from_existing(1, 1, client=mock_client)
 
 
-def _container_response_stub(from_template_id=8387):
+def _container_response_stub(from_template_id=8387, *, drop_arguments=None):
     arguments = {
         'MODEL': 'sparse_logistic',
         'TARGET_COLUMN': 'brushes_teeth_much',
@@ -762,6 +762,8 @@ def _container_response_stub(from_template_id=8387):
         'DEPENDENCIES': 'A B C D',
         'GIT_CRED': 9876
     }
+    for arg in (drop_arguments or []):
+        del arguments[arg]
     notifications = {
         'urls': [],
         'failureEmailAddresses': [],
@@ -824,9 +826,9 @@ def test_modelpipeline_classmethod_constructor_defaults(mock_future):
 
     # checks that it works with a registration template and train template
     for template_id in [TRAIN_ID_PROD, REGISTRATION_ID_PROD]:
-        container_response_stub = _container_response_stub(template_id)
-        del container_response_stub.arguments['PARAMS']
-        del container_response_stub.arguments['CVPARAMS']
+        container_response_stub = _container_response_stub(
+            template_id, drop_arguments=["PARAMS", "CVPARAMS"]
+        )
         mock_client = mock.Mock()
         mock_client.scripts.get_containers.return_value = container_response_stub  # noqa
         mock_client.credentials.get.return_value = Response({'name': 'Token'})

civis/parallel.py

@@ -20,7 +20,6 @@
 
 import civis
 from civis.base import CivisAPIError
-
 from civis.futures import _ContainerShellExecutor, CustomScriptExecutor
 
 try:
@@ -192,7 +191,7 @@ def infer_backend_factory(required_resources=None,
         try:
             custom_args = state.arguments
             state = client.scripts.get_containers(template.script_id)
-            state.arguments = custom_args
+            state._replace("arguments", custom_args)
         except civis.base.CivisAPIError as err:
             if err.status_code == 404:
                 raise RuntimeError('Unable to introspect environment from '
@@ -204,20 +203,20 @@ def infer_backend_factory(required_resources=None,
 
     # Default to this container's resource requests, but
     # allow users to override it.
-    state.required_resources.update(required_resources or {})
+    state.required_resources._data_snake.update(required_resources or {})
 
     # Update parameters with user input
     params = params or []
     for input_param in params:
-        for param in state.params:
+        for i, param in enumerate(list(state.params)):
             if param['name'] == input_param['name']:
-                param.update(input_param)
+                param._data_snake.update(input_param)
                 break
         else:
-            state.params.append(input_param)
+            state._data_snake["params"].append(input_param)
 
     # Update arguments with input
-    state.arguments.update(arguments or {})
+    state.arguments._data_snake.update(arguments or {})
 
     # Set defaults on other keyword arguments with
     # values from the current script

civis/response.py

@@ -15,6 +15,10 @@ def __str__(self):
         return self.error_message
 
 
+class CivisImmutableResponseError(Exception):
+    pass
+
+
 def _response_to_json(response):
     """Parse a raw response to a dict.
 
@@ -94,7 +98,11 @@ def convert_response_data_type(response, headers=None, return_type='snake'):
         return Response(data, headers=headers)
 
 
-class Response(dict):
+def _raise_response_immutable_error():
+    raise CivisImmutableResponseError("Response object is not mutable")
+
+
+class Response:
     """Custom Civis response object.
 
     Attributes
@@ -109,42 +117,90 @@ class Response(dict):
         Number of API calls remaining before rate limit is reached.
     rate_limit : int
         Total number of calls per API rate limit period.
-
-    Notes
-    -----
-    The main features of this class are that it maps camelCase to snake_case
-    at the top level of the json object and attaches keys as attributes.
-    Nested object keys are not changed.
     """
-    def __init__(self, json_data, snake_case=True, headers=None):
+    def __init__(self, json_data, *, headers=None):
         self.json_data = json_data
-        if headers is not None:
-            # this circumvents recursive calls
-            self.headers = headers
-            self.calls_remaining = headers.get('X-RateLimit-Remaining')
-            self.rate_limit = headers.get('X-RateLimit-Limit')
+        self.headers = headers
+        self.calls_remaining = (
+            int(x)
+            if (x := (headers or {}).get('X-RateLimit-Remaining')) else x
+        )
+        self.rate_limit = (
+            int(x)
+            if (x := (headers or {}).get('X-RateLimit-Limit')) else x
+        )
 
-        # Keys to update for this response object.
-        self_updates = {}
+        self._data_camel = {}
+        self._data_snake = {}
 
         if json_data is not None:
             for key, v in json_data.items():
-                if snake_case:
-                    key = camel_to_snake(key)
 
                 if isinstance(v, dict):
-                    val = Response(v, False)
+                    val = Response(v)
                 elif isinstance(v, list):
                     val = [Response(o) if isinstance(o, dict) else o
                            for o in v]
                 else:
                     val = v
 
-                self_updates[key] = val
+                self._data_camel[key] = val
+                self._data_snake[camel_to_snake(key)] = val
 
-        self.update(self_updates)
-        # Update self.__dict__ at the end to avoid replacing the update method.
-        self.__dict__.update(self_updates)
+    def __setattr__(self, key, value):
+        if key == "__dict__":
+            self.__dict__.update(value)
+        elif key in ("json_data", "headers", "calls_remaining", "rate_limit",
+                     "_data_camel", "_data_snake"):
+            self.__dict__[key] = value
+        else:
+            _raise_response_immutable_error()
+
+    def __setitem__(self, key, value):
+        _raise_response_immutable_error()
+
+    def __getitem__(self, item):
+        try:
+            return self._data_snake[item]
+        except KeyError:
+            return self._data_camel[item]
+
+    def __getattr__(self, item):
+        try:
+            return self.__getitem__(item)
+        except KeyError as e:
+            raise AttributeError(f"Response object has no attribute {str(e)}")
+
+    def __len__(self):
+        return len(self._data_snake)
+
+    def __repr__(self):
+        return repr(self._data_snake)
+
+    def get(self, key, default=None):
+        try:
+            return self.__getitem__(key)
+        except KeyError:
+            return default
+
+    def items(self):
+        return self._data_snake.items()
+
+    def __eq__(self, other):
+        if isinstance(other, dict):
+            return self._data_snake == other
+        elif isinstance(other, Response):
+            return self._data_snake == other._data_snake
+        else:
+            raise TypeError(f"Response and {type(other)} can't be compared")
+
+    def __setstate__(self, state):
+        """Set the state when unpickling, to avoid RecursionError."""
+        self.__dict__ = state
+
+    def _replace(self, key, value):
+        """Only used within this repo; `key` assumed to be in snake_case."""
+        self._data_snake[key] = value
 
 
 class PaginatedResponse:

civis/tests/mocks.py

@@ -72,15 +72,15 @@ def create_client_mock_for_container_tests(
                                    'container_id': script_id,
                                    'state': state})
     if state == 'failed':
-        mock_container_run['error'] = 'None'
+        mock_container_run._replace("error", "None")
     c.scripts.post_containers_runs.return_value = mock_container_run_start
     c.scripts.get_containers_runs.return_value = mock_container_run
     c.scripts.list_containers_runs_outputs.return_value = (run_outputs or [])
     c.jobs.list_runs_outputs.return_value = (run_outputs or [])
     c.jobs.list_runs_logs.return_value = (log_outputs or [])
 
     def change_state_to_cancelled(script_id):
-        mock_container_run.state = "cancelled"
+        mock_container_run._replace("state", "cancelled")
         return mock_container_run
 
     c.scripts.post_cancel.side_effect = change_state_to_cancelled

civis/tests/test_futures.py

@@ -1,7 +1,9 @@
+import json
 from operator import itemgetter
 from unittest import mock
 
 import pytest
+import requests
 
 from civis import APIClient, response
 from civis.base import CivisAPIError, CivisJobFailure
@@ -133,7 +135,7 @@ def _check_executor(from_template_id=None):
         future = bpe.submit("foo")
 
     # Mock and test running, future.job_id, and done()
-    mock_run.state = "running"
+    mock_run._replace("state", "running")
     assert future.running(), "future is incorrectly marked as not running"
     assert future.job_id == job_id, "job_id not stored properly"
     assert not future.done(), "future is incorrectly marked as done"
@@ -145,11 +147,11 @@ def _check_executor(from_template_id=None):
     assert not future.running(), "running() did not return False as expected"
 
     # Mock and test done()
-    mock_run.state = "succeeded"
+    mock_run._replace("state", "succeeded")
     assert future.done(), "done() did not return True as expected"
 
     # Test cancelling all jobs.
-    mock_run.state = "running"
+    mock_run._replace("state", "running")
     bpe.cancel_all()
     assert future.cancelled(), "cancel_all() failed"
 
@@ -316,7 +318,7 @@ def _setup_client_mock(job_id=-10, run_id=100, n_failures=8,
         n_failures, failure_is_error)
 
     def change_state_to_cancelled(job_id):
-        mock_container_run.state = "cancelled"
+        mock_container_run._replace("state", "cancelled")
         return mock_container_run
 
     c.scripts.post_cancel.side_effect = change_state_to_cancelled
@@ -331,15 +333,14 @@ def test_cancel_finished_job():
     # Set up a mock client which will give an exception when
     # you try to cancel any job.
     c = _setup_client_mock()
-    err_resp = response.Response({
+    err_resp = requests.Response()
+    err_resp._content = json.dumps({
         'status_code': 404,
         'error': 'not_found',
         'errorDescription': 'The requested resource could not be found.',
-        'content': True})
-    err_resp.json = lambda: err_resp.json_data
+        'content': True}).encode()
     c.scripts.post_cancel.side_effect = CivisAPIError(err_resp)
-    c.scripts.post_containers_runs.return_value.state = 'running'
-
+    c.scripts.post_containers_runs.return_value._replace("state", "running")
     fut = ContainerFuture(-10, 100, polling_interval=1, client=c,
                           poll_on_creation=False)
     assert not fut.done()

civis/tests/test_parallel.py

@@ -219,7 +219,7 @@ def test_infer(mock_make_factory, mock_job):
                                         'CIVIS_RUN_ID': "test_run"}):
         civis.parallel.infer_backend_factory(client=mock_client)
 
-    expected = dict(mock_job)
+    expected = mock_job._data_snake
     del expected['from_template_id']
     del expected['id']
     mock_make_factory.assert_called_once_with(