From: Jacob Date: Sun, 7 Sep 2025 20:58:49 +0000 (-0500) Subject: README and docs X-Git-Url: https://git.jacobcasper.com/?a=commitdiff_plain;p=close-be.git README and docs --- diff --git a/README.md b/README.md index e69de29..3a60fe2 100644 --- 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 d3eb003..521aa32 100644 --- a/app.py +++ b/app.py @@ -20,6 +20,7 @@ def create_app(): @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() @@ -34,6 +35,10 @@ def create_app(): @app.route("/api/", methods=["GET"]) @app.route("/api/", 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. @@ -67,6 +72,7 @@ def create_app(): @app.route("/test//", 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. diff --git a/redis_client.py b/redis_client.py index 19b2d99..dc0efe2 100644 --- 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: + """Atomically increment the hit counter for url by 1.""" 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") @@ -30,9 +31,11 @@ class RedisClient: 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]: + """Retrieve test validation parameters by id.""" test_parts = self.r.smembers(f"test:{test_id}") if not test_parts: return set()