Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ServiceApiClient: refactor construction, use thread-local instances #257

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import os
from collections.abc import Callable
from contextvars import ContextVar

import jinja2
from flask import current_app, make_response, render_template
Expand All @@ -7,6 +9,8 @@
from notifications_utils import logging, request_helper
from notifications_utils.base64_uuid import base64_to_uuid, uuid_to_base64
from notifications_utils.clients.statsd.statsd_client import StatsdClient
from notifications_utils.local_vars import LazyLocalGetter
from werkzeug.local import LocalProxy
from werkzeug.routing import BaseConverter, ValidationError

from app.asset_fingerprinter import AssetFingerprinter
Expand All @@ -16,7 +20,20 @@
metrics = GDSMetrics()
statsd_client = StatsdClient()
asset_fingerprinter = AssetFingerprinter()
service_api_client = ServiceApiClient()

memo_resetters: list[Callable] = []

#
# "clients" that need thread-local copies
#

_service_api_client_context_var: ContextVar[ServiceApiClient] = ContextVar("service_api_client")
get_service_api_client: LazyLocalGetter[ServiceApiClient] = LazyLocalGetter(
_service_api_client_context_var,
lambda: ServiceApiClient(app=current_app),
)
memo_resetters.append(lambda: get_service_api_client.clear())
service_api_client = LocalProxy(get_service_api_client)


class Base64UUIDConverter(BaseConverter):
Expand Down Expand Up @@ -61,8 +78,6 @@ def create_app(application):

register_errorhandlers(application)

service_api_client.init_app(application)


def init_app(application):
application.after_request(useful_headers_after_request)
Expand All @@ -76,6 +91,14 @@ def inject_global_template_variables():
}


def reset_memos():
"""
Reset all memos registered in memo_resetters
"""
for resetter in memo_resetters:
resetter()


# https://www.owasp.org/index.php/List_of_useful_HTTP_headers
def useful_headers_after_request(response):
response.headers.add("X-Robots-Tag", "noindex, nofollow")
Expand Down
15 changes: 7 additions & 8 deletions app/notify_client/service_api_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,15 @@ def generate_headers(self, api_token):


class ServiceApiClient:
def __init__(self):
self.api_client = None

def init_app(self, application):
def __init__(self, app):
self.api_client = OnwardsRequestNotificationsAPIClient(
base_url=application.config["API_HOST_NAME"],
api_key="a" * 75,
"x" * 100,
base_url=app.config["API_HOST_NAME"],
)
self.api_client.service_id = application.config["ADMIN_CLIENT_USER_NAME"]
self.api_client.api_key = application.config["ADMIN_CLIENT_SECRET"]
# our credential lengths aren't what NotificationsAPIClient's __init__ will expect
# given it's designed for destructuring end-user api keys
self.api_client.service_id = app.config["ADMIN_CLIENT_USER_NAME"]
self.api_client.api_key = app.config["ADMIN_CLIENT_SECRET"]

def get_service(self, service_id):
"""
Expand Down
4 changes: 2 additions & 2 deletions requirements.in
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ Flask-WTF==1.2.1

whitenoise==6.2.0 #manages static assets

notifications-python-client==8.0.1
notifications-python-client==10.0.0

# Run `make bump-utils` to update to the latest version
notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.1.1
notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.6.1

# gds-metrics requires prometheseus 0.2.0, override that requirement as later versions bring significant performance gains
prometheus-client==0.15.0
Expand Down
4 changes: 2 additions & 2 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ markupsafe==2.1.1
# wtforms
mistune==0.8.4
# via notifications-utils
notifications-python-client==8.0.1
notifications-python-client==10.0.0
# via -r requirements.in
notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.1.1
notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@82.6.1
# via -r requirements.in
ordered-set==4.1.0
# via notifications-utils
Expand Down
8 changes: 3 additions & 5 deletions tests/app/notify_client/test_service_api_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,15 @@


def test_client_gets_service(mocker):
client = ServiceApiClient()
client = ServiceApiClient(mocker.MagicMock())
mock_api_client = mocker.patch.object(client, "api_client")

client.get_service("foo")
mock_api_client.get.assert_called_once_with("/service/foo")


def test_client_onward_headers(app_, rmock, service_id, sample_service):
client = ServiceApiClient()
client.init_app(app_)
client = ServiceApiClient(app_)

rmock.get(
"{}/service/{}".format(
Expand All @@ -41,8 +40,7 @@ def test_client_onward_headers(app_, rmock, service_id, sample_service):


def test_client_no_onward_headers(app_, rmock, service_id, sample_service):
client = ServiceApiClient()
client.init_app(app_)
client = ServiceApiClient(app_)

rmock.get(
"{}/service/{}".format(
Expand Down
4 changes: 3 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,22 @@
import requests_mock
from flask import Flask, current_app

from app import create_app
from app import create_app, reset_memos


@pytest.fixture
def app_(request):
app = Flask("app")
create_app(app)

reset_memos()
ctx = app.app_context()
ctx.push()

yield app

ctx.pop()
reset_memos()


@pytest.fixture(scope="function")
Expand Down