Merge pull request #4494 from AlexVelezLl/backend-abstract-class

Core functionality of `connect` and `make_request` methods on `Backend` class

contentcuration/automation/tests/appnexus/test_base.py

@@ -1,33 +1,41 @@
+import time
 import pytest
+import requests
+from unittest.mock import patch
 
 from automation.utils.appnexus.base import Adapter
 from automation.utils.appnexus.base import Backend
+from automation.utils.appnexus.base import BackendRequest
+from automation.utils.appnexus.base import SessionWithMaxConnectionAge
+from automation.utils.appnexus.errors import ConnectionError
 
 
 class MockBackend(Backend):
+    base_url = 'https://kolibri-dev.learningequality.org'
+    connect_endpoint = '/status'
     def connect(self) -> None:
         return super().connect()
 
     def make_request(self, request):
         return super().make_request(request)
 
-    @classmethod
-    def _create_instance(cls) -> 'MockBackend':
-        return cls()
+class ErrorBackend(Backend):
+    base_url = 'https://bad-url.com'
+    connect_endpoint = '/status'
+    def connect(self) -> None:
+        return super().connect()
+
+    def make_request(self, request):
+        return super().make_request(request)
 
 
 class MockAdapter(Adapter):
     def mockoperation(self):
         pass
 
 
-def test_backend_error():
-    with pytest.raises(NotImplementedError) as error:
-        Backend.get_instance()
-    assert "Subclasses should implement the creation of instance" in str(error.value)
-
 def test_backend_singleton():
-    b1, b2 = MockBackend.get_instance(), MockBackend.get_instance()
+    b1, b2 = MockBackend(), MockBackend()
     assert id(b1) == id(b2)
 
 
@@ -46,3 +54,62 @@ def test_adapter_backend_custom():
     b = MockBackend()
     a = Adapter(backend=b)
     assert a.backend is b
+
+def test_session_with_max_connection_age_request():
+    with patch.object(requests.Session, 'request') as mock_request:
+        session = SessionWithMaxConnectionAge()
+        session.request('GET', 'https://example.com')
+        assert mock_request.call_count == 1
+
+def test_session_with_max_connection_age_not_closing_connections():
+    with patch.object(requests.Session, 'close') as mock_close,\
+        patch.object(requests.Session, 'request') as mock_request:
+        session = SessionWithMaxConnectionAge(60)
+        session.request('GET', 'https://example.com')
+        time.sleep(0.1)
+        session.request('GET', 'https://example.com')
+
+        assert mock_close.call_count == 0
+        assert mock_request.call_count == 2
+
+def test_session_with_max_connection_age_closing_connections():
+    with patch.object(requests.Session, 'close') as mock_close,\
+        patch.object(requests.Session, 'request') as mock_request:
+        session = SessionWithMaxConnectionAge(1)
+        session.request('GET', 'https://example.com')
+        time.sleep(2)
+        session.request('GET', 'https://example.com')
+
+        assert mock_close.call_count == 1
+        assert mock_request.call_count == 2
+
+def test_backend_connect():
+    backend = MockBackend()
+    connected = backend.connect()
+
+    assert connected is True
+
+def test_backend_connect_error():
+    backend = ErrorBackend()
+    connected = backend.connect()
+
+    assert connected is False
+
+def test_backend_request():
+    request = BackendRequest('GET', '/api/public/info')
+
+    backend = MockBackend()
+    response = backend.make_request(request)
+
+    assert response.status_code == 200
+    assert len(response.__dict__) > 0
+
+def test_backend_request_error():
+    request = BackendRequest('GET', '/api/public/info')
+
+    backend = ErrorBackend()
+
+    with pytest.raises(ConnectionError) as error:
+        backend.make_request(request)
+
+    assert "Unable to connect to" in str(error.value)

contentcuration/automation/utils/appnexus/base.py

@@ -1,46 +1,181 @@
+import time
+import logging
+import requests
 from abc import ABC
 from abc import abstractmethod
 from builtins import NotImplementedError
+from requests.adapters import HTTPAdapter
+from requests.packages.urllib3.util.retry import Retry
+
+from . import errors
+
+
+class SessionWithMaxConnectionAge(requests.Session):
+    """
+        Session with a maximum connection age. If the connection is older than the specified age, it will be closed and a new one will be created.
+        The age is specified in seconds.
+    """
+    def __init__(self, age = 100):
+        super().__init__()
+        self.age = age
+        self.last_used = time.time()
+
+    def request(self, *args, **kwargs):
+        current_time = time.time()
+        if current_time - self.last_used > self.age:
+            self.close()
+
+        self.last_used = current_time
+
+        return super().request(*args, **kwargs)
 
 
 class BackendRequest(object):
-    """ Class that should be inherited by specific backend for its requests"""
-    pass
+    """ Class that holds the request information for the backend """
+    def __init__(
+        self,
+        method,
+        path,
+        params=None,
+        data=None,
+        json=None,
+        headers=None,
+        timeout=(5, 100),
+        **kwargs
+    ):
+        self.method = method
+        self.path = path
+        self.params = params
+        self.data = data
+        self.json = json
+        self.headers = headers
+        self.timeout = timeout
+        for key, value in kwargs.items():
+            setattr(self, key, value)
 
 
 class BackendResponse(object):
     """ Class that should be inherited by specific backend for its responses"""
-    pass
+    def __init__(self, **kwargs):
+        for key, value in kwargs.items():
+            setattr(self, key, value)
 
 
 class Backend(ABC):
     """ An abstract base class for backend interfaces that also implements the singleton pattern """
     _instance = None
-
-    def __new__(class_, *args, **kwargs):
-        if not isinstance(class_._instance, class_):
-            class_._instance = object.__new__(class_, *args, **kwargs)
-        return class_._instance
-
-    @abstractmethod
-    def connect(self) -> None:
+    session = None
+    base_url = None
+    connect_endpoint = None
+    max_retries=1
+    backoff_factor=0.3
+
+    def __new__(cls, *args, **kwargs):
+        if not isinstance(cls._instance, cls):
+            cls._instance = object.__new__(cls)
+        return cls._instance
+
+    def __init__(
+        self,
+        url_prefix="",
+    ):
+        self.url_prefix = url_prefix
+        if not self.session:
+            self._setup_session()
+
+    def _setup_session(self):
+        self.session = SessionWithMaxConnectionAge()
+
+        retry = Retry(
+            total=self.max_retries,
+            backoff_factor=self.backoff_factor,
+        )
+        adapter = HTTPAdapter(max_retries=retry)
+
+        self.session.mount("https://", adapter)
+        self.session.mount("http://", adapter)
+
+    def _construct_full_url(self, path):
+        """This method combine base_url, url_prefix, and path in that order, removing any trailing and leading slashes."""
+        url_array = []
+        if self.base_url:
+            url_array.append(self.base_url.rstrip("/"))
+        if self.url_prefix:
+            url_array.append(self.url_prefix.rstrip("/").lstrip("/"))
+        if path:
+            url_array.append(path.lstrip("/"))
+        return "/".join(url_array)
+
+    def _make_request(self, request):
+        url = self._construct_full_url(request.path)
+        try:
+            response = self.session.request(
+                request.method,
+                url,
+                params=request.params,
+                data=request.data,
+                headers=request.headers,
+                json=request.json,
+                timeout=request.timeout,
+            )
+            response.raise_for_status()
+            return response
+        except (
+            requests.exceptions.ConnectionError,
+            requests.exceptions.RequestException,
+            requests.exceptions.SSLError,
+        ) as e:
+            logging.exception(e)
+            raise errors.ConnectionError(f"Unable to connect to {url}")
+        except (
+            requests.exceptions.Timeout,
+            requests.exceptions.ConnectTimeout,
+            requests.exceptions.ReadTimeout,
+        ) as e:
+            logging.exception(e)
+            raise errors.TimeoutError(f"Timeout occurred while connecting to {url}")
+        except (
+            requests.exceptions.TooManyRedirects,
+            requests.exceptions.HTTPError,
+        ) as e:
+            logging.exception(e)
+            raise errors.HttpError(f"HTTP error occurred while connecting to {url}")
+        except (
+            requests.exceptions.URLRequired,
+            requests.exceptions.MissingSchema,
+            requests.exceptions.InvalidSchema,
+            requests.exceptions.InvalidURL,
+            requests.exceptions.InvalidHeader,
+            requests.exceptions.InvalidJSONError,
+        ) as e:
+            logging.exception(e)
+            raise errors.InvalidRequest(f"Invalid request to {url}")
+        except (
+            requests.exceptions.ContentDecodingError,
+            requests.exceptions.ChunkedEncodingError,
+        ) as e:
+            logging.exception(e)
+            raise errors.InvalidResponse(f"Invalid response from {url}")
+
+    def connect(self, **kwargs):
         """ Establishes a connection to the backend service. """
-        pass
-
-    @abstractmethod
-    def make_request(self, request) -> BackendResponse:
-        """ Make a request based on "request" """
-        pass
-
-    @classmethod
-    def get_instance(cls) -> 'Backend':
-        """ Returns existing instance, if not then create one. """
-        return cls._instance if cls._instance else cls._create_instance()
-
-    @classmethod
-    def _create_instance(cls) -> 'Backend':
-        """ Returns the instance after creating it. """
-        raise NotImplementedError("Subclasses should implement the creation of instance")
+        try:
+            request = BackendRequest(method="GET", path=self.connect_endpoint, **kwargs)
+            self._make_request(request)
+            return True
+        except Exception as e:
+            return False
+
+    def make_request(self, request):
+        """ Make a request to the backend service. """
+        response = self._make_request(request)
+        try:
+            info = response.json()
+            info.update({"status_code": response.status_code})
+            return BackendResponse(**info)
+        except ValueError as e:
+            logging.exception(e)
+            raise errors.InvalidResponse("Invalid response from backend")
 
 
 class BackendFactory(ABC):

contentcuration/automation/utils/appnexus/errors.py

@@ -0,0 +1,15 @@
+
+class ConnectionError(Exception):
+  pass
+
+class TimeoutError(Exception):
+  pass
+
+class HttpError(Exception):
+  pass
+
+class InvalidRequest(Exception):
+  pass
+
+class InvalidResponse(Exception):
+  pass

contentcuration/contentcuration/tests/utils/test_cloud_storage.py

@@ -7,4 +7,4 @@ class CloudStorageTestCase(TestCase):
     def test_backend_initialization(self):
         cloud_storage_instance = CloudStorage()
         self.assertIsNotNone(cloud_storage_instance)
-        self.assertIsInstance(cloud_storage_instance.get_instance(), CloudStorage)
+        self.assertIsInstance(cloud_storage_instance, CloudStorage)

contentcuration/contentcuration/tests/utils/test_recommendations.py

@@ -7,4 +7,4 @@ class RecommendationsTestCase(TestCase):
     def test_backend_initialization(self):
         recomendations = Recommendations()
         self.assertIsNotNone(recomendations)
-        self.assertIsInstance(recomendations.get_instance(), Recommendations)
+        self.assertIsInstance(recomendations, Recommendations)