Merge branch 'server-side-fix'

* server-side-fix:
  Remove outdated comment
  Fail gracefully when no region is configured
  Don't use boto3 session directly in server completer

Author: jamesls

awsshell/resource/index.py

@@ -4,7 +4,7 @@
 
 * ResourceIndexBuilder - Takes a boto3 resource and converts into the
   index format we need to do server side completions.
-* CompleterQuery - Takes the index from ResourceIndexBuilder and looks
+* CompleterDescriber - Takes the index from ResourceIndexBuilder and looks
   up how to perform the autocompletion.  Note that this class does
   *not* actually do the autocompletion.  It merely tells you how
   you _would_ do the autocompletion if you made the appropriate
@@ -20,6 +20,7 @@
 
 import jmespath
 from botocore import xform_name
+from botocore.exceptions import BotoCoreError
 
 LOG = logging.getLogger(__name__)
 
@@ -86,8 +87,18 @@ def build_index(self, resource_data):
         return index
 
 
-class CompleterQuery(object):
-    """Describes how to autocomplete a resource."""
+class CompleterDescriber(object):
+    """Describes how to autocomplete a resource.
+
+    You give this class a service/operation/param and it will
+    describe to you how you can autocomplete values for the
+    provided parameter.
+
+    It's up to the caller to actually take that description
+    and make the appropriate service calls + filtering to
+    extract out the server side values.
+
+    """
     def __init__(self, resource_index):
         self._index = resource_index
 
@@ -126,71 +137,115 @@ def describe_autocomplete(self, service, operation, param):
                                 params={}, path=path)
 
 
-class ServerSideCompleter(object):
-    def __init__(self, session, builder):
-        # session is a boto3 session.
-        # It is a public attribute as it is intended to be
-        # changed if the profile changes.
-        self.session = session
-        self._loader = session._loader
-        self._builder = builder
+class CachedClientCreator(object):
+    def __init__(self, session):
+        #: A botocore.session.Session object.  Only the
+        #: create_client() method is used.
+        self._session = session
         self._client_cache = {}
-        self._completer_cache = {}
-        self._update_loader_paths()
+
+    def create_client(self, service_name):
+        if service_name not in self._client_cache:
+            client = self._session.create_client(service_name)
+            self._client_cache[service_name] = client
+        return self._client_cache[service_name]
+
+
+class CompleterDescriberCreator(object):
+    """Create and cache CompleterDescriber objects."""
+    def __init__(self, loader):
+        #: A botocore.loader.Loader
+        self._loader = loader
+        self._describer_cache = {}
+        self._services_with_completions = None
+
+    def create_completer_query(self, service_name):
+        """Create a CompleterDescriber for a service.
+
+        :type service_name: str
+        :param service_name: The name of the service, e.g. 'ec2'
+
+        :return: A CompleterDescriber object.
+
+        """
+        if service_name not in self._describer_cache:
+            query = self._create_completer_query(service_name)
+            self._describer_cache[service_name] = query
+        return self._describer_cache[service_name]
+
+    def _create_completer_query(self, service_name):
+        completions_model = self._loader.load_service_model(
+            service_name, 'completions-1')
+        cq = CompleterDescriber({service_name: completions_model})
+        return cq
+
+    def services_with_completions(self):
+        if self._services_with_completions is not None:
+            return self._services_with_completions
         self._services_with_completions = set(
             self._loader.list_available_services(type_name='completions-1'))
+        return self._services_with_completions
 
-    def _update_loader_paths(self):
-        completions_path = os.path.join(
-            os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
-            'data')
-        self._loader.search_paths.insert(0, completions_path)
-
-    def _get_completer_for_service(self, service_name):
-        if service_name not in self._completer_cache:
-            api_version = self._loader.determine_latest_version(
-                service_name, 'completions-1')
-            completions_model = self._loader.load_service_model(
-                service_name, 'completions-1', api_version)
-            cq = CompleterQuery({service_name: completions_model})
-            self._completer_cache[service_name] = cq
-        return self._completer_cache[service_name]
-
-    def _get_client(self, service_name):
-        if service_name in self._client_cache:
-            return self._client_cache[service_name]
-        client = self.session.client(service_name)
-        self._client_cache[service_name] = client
-        return client
+
+class ServerSideCompleter(object):
+    def __init__(self, client_creator, describer_creator):
+        self._client_creator = client_creator
+        self._describer_creator = describer_creator
 
     def retrieve_candidate_values(self, service, operation, param):
-        if service not in self._services_with_completions:
-            return
+        """Retrieve server side completions.
+
+        :type service: str
+        :param service: The service name, e.g. 'ec2', 'iam'.
+
+        :type operation: str
+        :param operation: The operation name, in the casing
+            used by the CLI (words separated by hyphens), e.g.
+            'describe-instances', 'delete-user'.
+
+        :type param: str
+        :param param: The param name, as specified in the service
+            model, e.g. 'InstanceIds', 'UserName'.
+
+        :rtype: list
+        :return: A list of possible completions for the
+            service/operation/param combination.  If no
+            completions were found an empty list is returned.
+
+        """
         # Example call:
-        # service='ec2', operation='terminate-instances',
-        # param='--instance-ids'.
-        # We need to convert this to botocore syntax.
-        # First try to load the resource model.
-        LOG.debug("Called with: %s, %s, %s", service, operation, param)
-        # Now convert operation to the name used by botocore.
-        client = self._get_client(service)
+        # service='ec2',
+        # operation='terminate-instances',
+        # param='InstanceIds'.
+        if service not in self._describer_creator.services_with_completions():
+            return []
+        try:
+            client = self._client_creator.create_client(service)
+        except BotoCoreError as e:
+            # create_client() could raise an exception if the session
+            # isn't fully configured (say it's missing a region).
+            # However, we don't want to turn off all server side
+            # completions because it's still possible to create
+            # clients for some services without a region, e.g. IAM.
+            LOG.debug("Error when trying to create a client for %s",
+                      service, exc_info=True)
+            return []
         api_operation_name = client.meta.method_to_api_mapping.get(
             operation.replace('-', '_'))
         if api_operation_name is None:
-            return
+            return []
         # Now we need to convert the param name to the
         # casing used by the API.
-        completer = self._get_completer_for_service(service)
+        completer = self._describer_creator.create_completer_query(service)
         result = completer.describe_autocomplete(
             service, api_operation_name, param)
         if result is None:
             return
-        # DEBUG:awsshell.resource.index:RESULTS:
-            # ServerCompletion(service=u'ec2', operation=u'DescribeInstances',
-            # params={}, path=u'Reservations[].Instances[].InstanceId')
         try:
             response = getattr(client, xform_name(result.operation, '_'))()
-        except Exception:
+        except Exception as e:
+            LOG.debug("Error when calling %s.%s: %s", service,
+                      result.operation, e, exc_info=True)
             return
         results = jmespath.search(result.path, response)
         return results

awsshell/shellcomplete.py

@@ -9,6 +9,7 @@
 logic, see awsshell.autocomplete.
 
 """
+import os
 import logging
 from prompt_toolkit.completion import Completer, Completion
 from awsshell import fuzzy
@@ -31,12 +32,20 @@ def __init__(self, completer, server_side_completer=None):
             server_side_completer = self._create_server_side_completer()
         self._server_side_completer = server_side_completer
 
-    def _create_server_side_completer(self):
-        import boto3.session
+    def _create_server_side_completer(self, session=None):
+        import botocore.session
         from awsshell.resource import index
-        session = boto3.session.Session()
-        builder = index.ResourceIndexBuilder()
-        completer = index.ServerSideCompleter(session, builder)
+        if session is None:
+            session = botocore.session.Session()
+        loader = session.get_component('data_loader')
+        completions_path = os.path.join(
+            os.path.dirname(os.path.abspath(__file__)),
+            'data')
+        loader.search_paths.insert(0, completions_path)
+
+        client_creator = index.CachedClientCreator(session)
+        describer = index.CompleterDescriberCreator(loader)
+        completer = index.ServerSideCompleter(client_creator, describer)
         return completer
 
     @property

tests/unit/test_resources.py

@@ -1,8 +1,23 @@
 """Index and retrive information from the resource JSON."""
 import pytest
+import mock
+
+from botocore.exceptions import NoRegionError
+
 from awsshell.resource import index
 
 
+@pytest.fixture
+def describer_creator():
+    class FakeDescriberCreator(object):
+        SERVICES = ['ec2']
+
+        def services_with_completions(self):
+            return self.SERVICES
+
+    return FakeDescriberCreator()
+
+
 def test_build_from_has_many():
     resource = {
         'service': {
@@ -172,10 +187,81 @@ def test_can_complete_query():
             }
         }
     }
-    q = index.CompleterQuery(built_index)
+    q = index.CompleterDescriber(built_index)
     result = q.describe_autocomplete(
         'dynamodb', 'DeleteTable', 'TableName')
     assert result.service == 'dynamodb'
     assert result.operation == 'ListTables'
     assert result.params == {}
     assert result.path == 'TableNames[]'
+
+
+def test_cached_client_creator_returns_same_instance():
+    class FakeSession(object):
+        def create_client(self, service_name):
+            return object()
+
+    cached_creator = index.CachedClientCreator(FakeSession())
+    ec2 = cached_creator.create_client('ec2')
+    s3 = cached_creator.create_client('s3')
+    assert ec2 != s3
+    # However, asking for a client we've already created
+    # should return the exact same instance.
+    assert cached_creator.create_client('ec2') == ec2
+
+
+def test_can_create_service_completers_from_cache():
+    class FakeDescriberCreator(object):
+        def load_service_model(self, service_name, type_name):
+            assert type_name == 'completions-1'
+            return "fake_completions_for_%s" % service_name
+
+        def services_with_completions(self):
+            return []
+
+    loader = FakeDescriberCreator()
+    factory = index.CompleterDescriberCreator(loader)
+    result = factory.create_completer_query('ec2')
+    assert isinstance(result, index.CompleterDescriber)
+    assert factory.create_completer_query('ec2') == result
+
+
+def test_empty_results_returned_when_no_completion_data_exists(describer_creator):
+    describer_creator.SERVICES = []
+
+    completer = index.ServerSideCompleter(
+        client_creator=None,
+        describer_creator=describer_creator,
+    )
+    assert completer.retrieve_candidate_values(
+        'ec2', 'run-instances', 'ImageId') == []
+
+
+def test_no_completions_when_cant_create_client(describer_creator):
+    client_creator = mock.Mock(spec=index.CachedClientCreator)
+    # This is raised when you don't have a region configured via config file
+    # env var or manually via a session.
+    client_creator.create_client.side_effect = NoRegionError()
+    completer = index.ServerSideCompleter(
+        client_creator=client_creator,
+        describer_creator=describer_creator)
+
+    assert completer.retrieve_candidate_values(
+        'ec2', 'foo', 'Bar') == []
+
+
+def test_no_completions_returned_on_unknown_operation(describer_creator):
+    client = mock.Mock()
+    client_creator = mock.Mock(spec=index.CachedClientCreator)
+    client_creator.create_client.return_value = client
+
+    client.meta.method_to_api_mapping = {
+        'describe_foo': 'DescribeFoo'
+    }
+
+    completer = index.ServerSideCompleter(
+        client_creator=client_creator,
+        describer_creator=describer_creator)
+
+    assert completer.retrieve_candidate_values(
+        'ec2', 'not_describe_foo', 'Bar') == []