README and docs

diff --git a/README.md b/README.md
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..3a60fe28982e11186626199e26b8a4758e947cdd 100644 (file)
--- a/README.md
+++ b/README.md
@@ -0,0 +1,52 @@
+# Solution
+
+In my solution to the backend developer challenge, I've prepared a Flask service
+that implements a web API server, test runner, and API stats storage.
+
+As requested the Flask app stores data in Redis using Docker Compose.
+
+I created the service with the pyproject.toml standard for definition.
+
+After finishing my submission in a single file, I refactored it to separate
+concerns slightly more clearly, with the redis stats client in `redis_client`
+providing stats and testing specific functionality, and the Flask app with its
+route handlers in a `create_app` factory. While these sections do not
+perfectly separate out domain concerns, this has more to do with my unfamiliarity
+in creating a Flask `app` instance by hand in a production ready manner.
+I did attempt to do some basic dependency injection of the redis client, but when
+doing so the runner complained that the factory could not be called
+with 0 arguments, and I returned to just instantiating a singleton of the client
+in the context of one `app` instance.
+
+Additional features that I would include if I were to spend more time on the
+project would be:
+
+1) Implementing configuration for both the Flask `app` as well as the
+`RedisClient`. For the Redis client I'd want the endpoint of the Redis
+instance being used to be configurable, and to include proper auth for
+a production database or cluster.
+
+2) Implement the stats counter as a middleware. While developing I went
+back and forth between how the `/api/*` routes should handle hit counting,
+primarily related to validation. Once I decided to always count a hit,
+even if it fails testing, then I felt that it would be nice to have a
+decorator or middleware to add to automatically hit count every route
+server by the Flask app. I didn't feel like the additional time it would
+take to write that would be worth it when I had a correct submission.
+
+3) Move test running out to a background worker. Regardless of the
+intent of the test runner, since the data is already being persisted in
+the Redis DB, test runners could be parallelized. I _believe_ I used the
+atomic increment operator correctly to facilitate this use-case.
+This would also allow for:
+  - The user to receive feedback quickly on their job being submitted
+  - The application server to free up a thread
+  - Guarantee the test can be run for longer than an HTTP request may take to time out.
+  - It would remove the odd context handler I had to find to modify request context
+  - It would more accurately test the web service from a real client context, allowing scale or other concurrency issues to be found.
+
+4) I would include more test cases, as right now error handling and concerns are a bit interwoven and inconsistent.
+  - The RedisClient uses `zrevrange` even though it doesn't know _why_ it ought to.
+  - The Flask app handles uncaught redis exceptions as it may want to handle presenting exceptions to the client differently.
+  - The validation function is not unit tested, even though it is easily extractable. This is because I felt that the web service level
+  exception logic was too specific to generalize in a helper function at this time.

diff --git a/app.py b/app.py
index d3eb0039da16276dbf1028137105ee5a4a93546a..521aa32a59dba790888eda669841b0e02a9345f2 100644 (file)
--- a/app.py
+++ b/app.py
@@ -20,6 +20,7 @@ def create_app():
 
     @app.route("/stats/", methods=["GET"])
     def stats():
 
     @app.route("/stats/", methods=["GET"])
     def stats():

+        """Returns the hit counts for all routes served by this application."""

         redis_client.increment_hits("stats")
         try:
             stats = redis_client.get_stats()
         redis_client.increment_hits("stats")
         try:
             stats = redis_client.get_stats()


@@ -34,6 +35,10 @@ def create_app():
     @app.route("/api/", methods=["GET"])
     @app.route("/api/<path:subpath>", methods=["GET"])
     def api(subpath: Optional[str] = None):
     @app.route("/api/", methods=["GET"])
     @app.route("/api/<path:subpath>", methods=["GET"])
     def api(subpath: Optional[str] = None):

+        """Handles api routing for the entire application.
+        If it detects it is running in test mode it will apply additional validation to the route requested.
+        Test mode is detected using query parameters with a test UUID that should have been retrieved from /test.
+        """

         app.logger.debug("Handling subpath %s", subpath)
         # ASSUMPTION: As the app code is actually invoked, this path is being "handled".
         # If paths failing validation needed to not be incremented then I would move this logic after validation.
         app.logger.debug("Handling subpath %s", subpath)
         # ASSUMPTION: As the app code is actually invoked, this path is being "handled".
         # If paths failing validation needed to not be incremented then I would move this logic after validation.


@@ -67,6 +72,7 @@ def create_app():
 
     @app.route("/test/<int:num_requests>/", methods=["POST"])
     def test(num_requests: int):
 
     @app.route("/test/<int:num_requests>/", methods=["POST"])
     def test(num_requests: int):

+        """Creates a test runner that will execute num_requests requests against the /api/ endpoint of this app."""

         redis_client.increment_hits("test")
         # ASSUMPTION: The requirements don't specify if the test runner or the api path under test specify
         # which 3 path parts are valid, so I will have the test runner generate and store them.
         redis_client.increment_hits("test")
         # ASSUMPTION: The requirements don't specify if the test runner or the api path under test specify
         # which 3 path parts are valid, so I will have the test runner generate and store them.

diff --git a/redis_client.py b/redis_client.py
index 19b2d99583606e7dc1d0d2a525802bc9da95abdf..dc0efe2f51d3f852aeaaa7ff6eca3ac25b6e9dad 100644 (file)
--- a/redis_client.py
+++ b/redis_client.py
@@ -19,10 +19,11 @@ class RedisClient:
         self.r = redis.Redis(host="redis", port=6379, decode_responses=True)
 
     def increment_hits(self, url: str) -> int:
         self.r = redis.Redis(host="redis", port=6379, decode_responses=True)
 
     def increment_hits(self, url: str) -> int:

+        """Atomically increment the hit counter for url by 1."""

         return self.r.zincrby(f"hits", 1, url)
 
     def get_stats(self) -> list[tuple]:
         return self.r.zincrby(f"hits", 1, url)
 
     def get_stats(self) -> list[tuple]:

-        """Returns stats values from a Redis sorted set as a tuple of key-value pairs."""
+        """Returns stats values from a Redis sorted set as a tuple of key-value pairs in decreasing order."""

         stats = self.r.zrevrange("hits", 0, -1, withscores=True, score_cast_func=int)
         if stats is None:
             logger.debug("Uninitialized stats object")
         stats = self.r.zrevrange("hits", 0, -1, withscores=True, score_cast_func=int)
         if stats is None:
             logger.debug("Uninitialized stats object")


@@ -30,9 +31,11 @@ class RedisClient:
         return stats
 
     def create_test(self, test_id: str, paths: list[str]):
         return stats
 
     def create_test(self, test_id: str, paths: list[str]):

+        """Stores a test run identifier and its validation parameters as a Redis set."""

         return self.r.sadd(f"test:{test_id}", *paths)
 
     def get_test(self, test_id: str) -> set[str]:
         return self.r.sadd(f"test:{test_id}", *paths)
 
     def get_test(self, test_id: str) -> set[str]:

+        """Retrieve test validation parameters by id."""

         test_parts = self.r.smembers(f"test:{test_id}")
         if not test_parts:
             return set()
         test_parts = self.r.smembers(f"test:{test_id}")
         if not test_parts:
             return set()