Repository: brettcannon/gidgethub (python)

Issue:
Provide a caching GitHubAPI mixin
Using [`ETag` and `Last-Modified`](https://developer.github.com/v3/#conditional-requests), a mixin which overrides `abc.GitHubAPI._request()` could pass in the appropriate headers and catch a `304` status code to return a cached result. This should only be used on `GET` with no `body`.

Pull request:
Provide a cache for GitHubAPI
Closes #20 

Patch:
diff --git a/README.rst b/README.rst
index a75470d..8593aee 100644
--- a/README.rst
+++ b/README.rst
@@ -70,6 +70,8 @@ Changelog
 
 - Add type hints.
 
+- Add a *cache* argument to ``gidgethub.abc.GitHubAPI``.
+
 
 2.2.0
 '''''''''''''''''''
diff --git a/docs/abc.rst b/docs/abc.rst
index 0432521..efdcba6 100644
--- a/docs/abc.rst
+++ b/docs/abc.rst
@@ -24,12 +24,20 @@ does not require an update to the library, allowing one to use
 experimental APIs without issue.
 
 
-.. class:: GitHubAPI(requester, *, oauth_token=None)
+.. class:: GitHubAPI(requester, *, oauth_token=None, cache=None)
 
     Provide an :py:term:`abstract base class` which abstracts out the
     HTTP library being used to send requests to GitHub. The class is
-    initialized with the requester's name and (optionally) their
-    OAuth token.
+    initialized with the requester's name and optionally their
+    OAuth token and a cache object.
+
+    To allow for
+    `conditional requests <https://developer.github.com/v3/#conditional-requests>`_,
+    one can provide a :class:`collections.abc.MutableMapping` object
+    for the *cache* argument to cache requests. It is up to the
+    caching object to provide any caching scheme that is desired
+    (e.g. the ``Cache`` classes provided by the
+    `cachetools package <https://pypi.org/project/cachetools/>`_).
 
     There are common arguments across methods that make requests to
     GitHub. The *url_vars* argument is used to perform
@@ -54,6 +62,9 @@ experimental APIs without issue.
         This leads to :exc:`~gidgethub.RateLimitExceeded` being raised
         when the rate limit has been execeeded.
 
+    .. versionchanged:: 2.3
+        Introduced the *cache* argument to the constructor.
+
 
     .. attribute:: requester
 
diff --git a/docs/aiohttp.rst b/docs/aiohttp.rst
index 27c89af..80e7818 100644
--- a/docs/aiohttp.rst
+++ b/docs/aiohttp.rst
@@ -3,7 +3,7 @@
 
 .. module:: gidgethub.aiohttp
 
-.. class:: GitHubAPI(session, requester, *, oauth_token=None)
+.. class:: GitHubAPI(session, requester, *, oauth_token=None, cache=None)
 
     An implementation of :class:`gidgethub.abc.GitHubAPI` using
     `aiohttp <https://aiohttp.readthedocs.io>`_. Typical usage will be::
diff --git a/docs/tornado.rst b/docs/tornado.rst
index 22bf61e..4985abc 100644
--- a/docs/tornado.rst
+++ b/docs/tornado.rst
@@ -3,7 +3,7 @@
 
 .. module:: gidgethub.tornado
 
-.. class:: GitHubAPI(requester, *, oauth_token=None)
+.. class:: GitHubAPI(requester, *, oauth_token=None, cache=None)
 
     An implementation of :class:`gidgethub.abc.GitHubAPI` using
     `Tornado <http://www.tornadoweb.org/>`_.
diff --git a/docs/treq.rst b/docs/treq.rst
index 55df6bb..dcf0af7 100644
--- a/docs/treq.rst
+++ b/docs/treq.rst
@@ -3,7 +3,7 @@
 
 .. module:: gidgethub.treq
 
-.. class:: GitHubAPI(requester, *, oauth_token=None)
+.. class:: GitHubAPI(requester, *, oauth_token=None, cache=None)
 
     An implementation of :class:`gidgethub.abc.GitHubAPI` using
     `treq <https://treq.readthedocs.io>`_. Typical usage will be::
diff --git a/gidgethub/abc.py b/gidgethub/abc.py
index 268959e..d2240ad 100644
--- a/gidgethub/abc.py
+++ b/gidgethub/abc.py
@@ -1,19 +1,26 @@
 """Provide an abstract base class for easier requests."""
 import abc
 import json
-from typing import Any, AsyncGenerator, Dict, Mapping, Tuple, Optional
+from typing import Any, AsyncGenerator, Dict, Mapping, MutableMapping, Tuple
+from typing import Optional as Opt
 
 from . import sansio
 
 
+# Value represents etag, last-modified, data, and next page.
+CACHE_TYPE = MutableMapping[str, Tuple[Opt[str], Opt[str], Any, Opt[str]]]
+
+
 class GitHubAPI(abc.ABC):
 
     """Provide an idiomatic API for making calls to GitHub's API."""
 
-    def __init__(self, requester: str, *, oauth_token: str = None) -> None:
+    def __init__(self, requester: str, *, oauth_token: Opt[str] = None,
+                 cache: Opt[CACHE_TYPE] = None) -> None:
         self.requester = requester
         self.oauth_token = oauth_token
-        self.rate_limit: Optional[sansio.RateLimit] = None
+        self._cache = cache
+        self.rate_limit: Opt[sansio.RateLimit] = None
 
     @abc.abstractmethod
     async def _request(self, method: str, url: str, headers: Mapping,
@@ -25,15 +32,26 @@ class GitHubAPI(abc.ABC):
         """Sleep for the specified number of seconds."""
 
     async def _make_request(self, method: str, url: str, url_vars: Dict,
-                            data: Any, accept: str) -> Tuple[bytes, Optional[str]]:
+                            data: Any, accept: str) -> Tuple[bytes, Opt[str]]:
         """Construct and make an HTTP request."""
         filled_url = sansio.format_url(url, url_vars)
         request_headers = sansio.create_headers(self.requester, accept=accept,
                                                 oauth_token=self.oauth_token)
+        etag = last_modified = None
         # Can't use None as a "no body" sentinel as it's a legitimate JSON type.
         if data == b"":
             body = b""
             request_headers["content-length"] = "0"
+            if method == "GET" and self._cache is not None:
+                try:
+                    etag, last_modified, data, more = self._cache[filled_url]
+                except KeyError:
+                    pass
+                else:
+                    if etag is not None:
+                        request_headers["if-none-match"] = etag
+                    if last_modified is not None:
+                        request_headers["if-modified-since"] = last_modified
         else:
             charset = "utf-8"
             body = json.dumps(data).encode(charset)
@@ -42,7 +60,14 @@ class GitHubAPI(abc.ABC):
         if self.rate_limit is not None:
             self.rate_limit.remaining -= 1
         response = await self._request(method, filled_url, request_headers, body)
-        data, self.rate_limit, more = sansio.decipher_response(*response)
+        if response[0] != 304 or (etag is None and last_modified is None):
+            data, self.rate_limit, more = sansio.decipher_response(*response)
+            has_cache_details = ("etag" in response[1]
+                                 or "last-modified" in response[1])
+            if self._cache is not None and has_cache_details:
+                etag = response[1].get("etag")
+                last_modified = response[1].get("last-modified")
+                self._cache[filled_url] = etag, last_modified, data, more
         return data, more
 
     async def getitem(self, url: str, url_vars: Dict = {},
diff --git a/gidgethub/aiohttp.py b/gidgethub/aiohttp.py
index 3db6c29..5bfe212 100644
--- a/gidgethub/aiohttp.py
+++ b/gidgethub/aiohttp.py
@@ -1,5 +1,5 @@
 import asyncio
-from typing import Mapping, Tuple
+from typing import Any, Mapping, Tuple
 
 import aiohttp
 
@@ -8,10 +8,10 @@ from . import abc as gh_abc
 
 class GitHubAPI(gh_abc.GitHubAPI):
 
-    def __init__(self, session: aiohttp.ClientSession, requester: str,
-                 *, oauth_token: str = None) -> None:
+    def __init__(self, session: aiohttp.ClientSession, *args: Any,
+                 **kwargs: Any) -> None:
         self._session = session
-        super().__init__(requester, oauth_token=oauth_token)
+        super().__init__(*args, **kwargs)
 
     async def _request(self, method: str, url: str, headers: Mapping,
                        body: bytes = b'') -> Tuple[int, Mapping, bytes]:


Test patch:
diff --git a/gidgethub/test/test_abc.py b/gidgethub/test/test_abc.py
index 9110441..a6052be 100644
--- a/gidgethub/test/test_abc.py
+++ b/gidgethub/test/test_abc.py
@@ -5,6 +5,7 @@ import types
 
 import pytest
 
+from .. import RedirectionException
 from .. import abc as gh_abc
 from .. import sansio
 
@@ -12,13 +13,15 @@ from .. import sansio
 class MockGitHubAPI(gh_abc.GitHubAPI):
 
     DEFAULT_HEADERS = {"x-ratelimit-limit": "2", "x-ratelimit-remaining": "1",
-                       "x-ratelimit-reset": "0"}
+                       "x-ratelimit-reset": "0",
+                       "content-type": "application/json"}
 
-    def __init__(self, status_code=200, headers=DEFAULT_HEADERS, body=b''):
+    def __init__(self, status_code=200, headers=DEFAULT_HEADERS, body=b'', *,
+                 cache=None):
         self.response_code = status_code
         self.response_headers = headers
         self.response_body = body
-        super().__init__("test_abc", oauth_token="oauth token")
+        super().__init__("test_abc", oauth_token="oauth token", cache=cache)
 
     async def _request(self, method, url, headers, body=b''):
         """Make an HTTP request."""
@@ -183,3 +186,86 @@ async def test_delete():
     data = await gh.delete("/fake")
     assert gh.method == "DELETE"
     assert data is None
+
+
+class TestCache:
+
+    @pytest.mark.asyncio
+    async def test_if_none_match_sent(self):
+        etag = "12345"
+        cache = {"https://api.github.com/fake": (etag, None, "hi", None)}
+        gh = MockGitHubAPI(cache=cache)
+        await gh.getitem("/fake")
+        assert "if-none-match" in gh.headers
+        assert gh.headers["if-none-match"] == etag
+
+
+    @pytest.mark.asyncio
+    async def test_etag_received(self):
+        cache = {}
+        etag = "12345"
+        headers = MockGitHubAPI.DEFAULT_HEADERS.copy()
+        headers["etag"] = etag
+        gh = MockGitHubAPI(200, headers, b'42', cache=cache)
+        data = await gh.getitem("/fake")
+        url = "https://api.github.com/fake"
+        assert url in cache
+        assert cache[url] == (etag, None, 42, None)
+        assert data == cache[url][2]
+
+    @pytest.mark.asyncio
+    async def test_if_modified_since_sent(self):
+        last_modified = "12345"
+        cache = {"https://api.github.com/fake": (None, last_modified, "hi", None)}
+        gh = MockGitHubAPI(cache=cache)
+        await gh.getitem("/fake")
+        assert "if-modified-since" in gh.headers
+        assert gh.headers["if-modified-since"] == last_modified
+
+    @pytest.mark.asyncio
+    async def test_last_modified_received(self):
+        cache = {}
+        last_modified = "12345"
+        headers = MockGitHubAPI.DEFAULT_HEADERS.copy()
+        headers["last-modified"] = last_modified
+        gh = MockGitHubAPI(200, headers, b'42', cache=cache)
+        data = await gh.getitem("/fake")
+        url = "https://api.github.com/fake"
+        assert url in cache
+        assert cache[url] == (None, last_modified, 42, None)
+        assert data == cache[url][2]
+
+    @pytest.mark.asyncio
+    async def test_hit(self):
+        url = "https://api.github.com/fake"
+        cache = {url: ("12345", "67890", 42, None)}
+        gh = MockGitHubAPI(304, cache=cache)
+        data = await gh.getitem(url)
+        assert data == 42
+
+    @pytest.mark.asyncio
+    async def test_miss(self):
+        url = "https://api.github.com/fake"
+        cache = {url: ("12345", "67890", 42, None)}
+        headers = MockGitHubAPI.DEFAULT_HEADERS.copy()
+        headers["etag"] = "09876"
+        headers["last-modified"] = "54321"
+        gh = MockGitHubAPI(200, headers, body=b"-13", cache=cache)
+        data = await gh.getitem(url)
+        assert data == -13
+        assert cache[url] == ("09876", "54321", -13, None)
+
+    @pytest.mark.asyncio
+    async def test_ineligible(self):
+        cache = {}
+        gh = MockGitHubAPI(body=b"42", cache=cache)
+        url = "https://api.github.com/fake"
+        await gh.getitem(url)
+        assert url not in cache
+
+    @pytest.mark.asyncio
+    async def test_redirect_without_cache(self):
+        cache = {}
+        gh = MockGitHubAPI(304, cache=cache)
+        with pytest.raises(RedirectionException):
+            await gh.getitem("/fake")