G'day:
Context
In yesterday's article - TDD & Symfony & Monolog: adding some logging to that endpoint from last time - I added some logging logic into this address-lookup web service endpoint I'm putting together. At the end of it, I thought the logic - which I had slung into the controller - was in the wrong place, and I needed to shimmy it into an intermediary service class between the controller and the upstream web service adapter (this will make more sense once you see the code below). Today I've done that refactoring.
Where I was
Here's the controller code:
public function doGet(string $postcode) : JsonResponse
{
try {
$response = $this->addressServiceAdapter->get($postcode);
$this->logUnexpectedFailures($response, $postcode);
return new JsonResponse(
[
'postcode' => $postcode,
'addresses' => $response->getAddresses(),
'message' => $response->getMessage()
],
$response->getHttpStatus()
);
} catch (\Exception $e) {
return new JsonResponse(
[
'postcode' => $postcode,
'addresses' => [],
'message' => $e->getMessage()
],
HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR
);
}
}
private const RESPONSES_TO_LOG = [
HttpStatusCode::HTTP_UNAUTHORIZED => Level::Critical,
HttpStatusCode::HTTP_FORBIDDEN => Level::Critical,
HttpStatusCode::HTTP_TOO_MANY_REQUESTS => Level::Warning,
HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR => Level::Warning
];
// …
private function logUnexpectedFailures(
AddressService\Response $response,
string $postcode
): void {
$statusCode = $response->getHttpStatus();
if (array_key_exists($statusCode, self::RESPONSES_TO_LOG)) {
$this->logger->log(
self::RESPONSES_TO_LOG[$statusCode],
AddressService\Adapter::ERROR_MESSAGES[$statusCode],
['postcode' => $postcode, 'message' => $response->getMessage()]
);
}
}
What's going on here is that the upstream web service could return with a number of cautionary HTTP status codes which we really need to know about if they happen, so I've added some logging in to make sure we know.
And the adapter code it calls:
public function get(string $postCode) : AddressService\Response
{
$response = $this->makeRequest($postCode);
$lookupResult = $this->extractValidLookupResult($response);
return $this->handleValidatedResponse($response, $lookupResult);
}
After getting the response from the upstream third-party web service, I validate what comes back (valid JSON with the schema I expect, etc; this could throw an exception if the data is no good), I pass it to a handler that returns well-formed data:
private function handleValidatedResponse(
ResponseInterface $response,
array $lookupResult
): AddressService\Response {
$statusCode = $response->getStatusCode();
if ($statusCode == HttpFoundationResponse::HTTP_OK) {
return $this->handleSuccessResponse($lookupResult);
}
return $this->handleFailureResponse($lookupResult, $statusCode);
}
private function handleSuccessResponse(array $lookupResult): AddressService\Response
{
if (
!array_key_exists("addresses", $lookupResult)
|| !is_array($lookupResult["addresses"])
|| count(array_filter($lookupResult["addresses"], fn($address) => !is_string($address)))
) {
throw new AddressService\Exception("Response JSON schema is not valid");
}
return new AddressService\Response(
$lookupResult["addresses"],
HttpFoundationResponse::HTTP_OK
);
}
private function handleFailureResponse(
array $lookupResult,
int $statusCode
): AddressService\Response {
if (array_key_exists("Message", $lookupResult) && is_string($lookupResult["Message"])) {
return new AddressService\Response([], $statusCode, $lookupResult["Message"]);
}
return new AddressService\Response([], $statusCode, "No failure message returned from service");
}
There's no need to worry too much about the details there. Just that we're returning data from a third-party web service, and the results could be good, bad or ugly.
This approach of having a controller calling the adapter directly was fine for the first iteration of this code, where it was basically getting the data from the adapter and returning it. Once I added in the logging - and the conditionality (business logic) around it - it occurred to me that the controller was not the right place for this. Logging is nothing to do with servicing an HTTP request, after all. And that's all a controller is supposed to do. I also couldn't push the logging into the adapter, as that's not an adapter's job either. It's just to provide an application-friendly interface to something that doesn't have such an interface. And nothing else.
So I'm shifting that logging logic into a service.
Where I am now
There's the controller now:
class PostcodeLookupController extends AbstractController
{
public function __construct(private readonly PostcodeLookupService $postcodeLookupService)
{
}
public function doGet(string $postcode) : JsonResponse
{
$response = $this->postcodeLookupService->lookup($postcode);
return new JsonResponse(
[
'postcode' => $postcode,
'addresses' => $response->getAddresses(),
'message' => $response->getMessage()
],
$response->getHttpStatus()
);
}
}
Note how it is receiving a PostcodeLookupService now. It was receiving the Adapter and the Logger before. And also note how it's really not doing much any more. Asking for some data from the service, returning it. Simple. Excellent.
And the service:
class PostcodeLookupService
{
private const RESPONSES_TO_LOG = [
HttpStatusCode::HTTP_UNAUTHORIZED => Level::Critical,
HttpStatusCode::HTTP_FORBIDDEN => Level::Critical,
HttpStatusCode::HTTP_TOO_MANY_REQUESTS => Level::Warning,
HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR => Level::Warning
];
public function __construct(
private readonly GetAddress\Adapter $adapter,
private readonly LoggerInterface $addressServiceLogger
) {
}
public function lookup(string $postcode): GetAddress\Response
{
try {
return $this->getAddresses($postcode);
} catch (GetAddress\Exception $e) {
return $this->handleAdapterException($e, $postcode);
}
}
private function getAddresses(string $postcode): GetAddress\Response
{
$response = $this->adapter->get($postcode);
$this->logUnexpectedFailures($response, $postcode);
return $response;
}
private function logUnexpectedFailures(
GetAddress\Response $response,
string $postcode
): void {
$statusCode = $response->getHttpStatus();
if (array_key_exists($statusCode, self::RESPONSES_TO_LOG)) {
$this->addressServiceLogger->log(
self::RESPONSES_TO_LOG[$statusCode],
GetAddress\Adapter::ERROR_MESSAGES[$statusCode],
['postcode' => $postcode, 'message' => $response->getMessage()]
);
}
}
private function handleAdapterException(
GetAddress\Exception|\Exception $e,
string $postcode
): GetAddress\Response {
$this->addressServiceLogger->log(
Level::Error,
$e->getMessage(),
['postcode' => $postcode, 'message' => $e->getMessage()]
);
return new GetAddress\Response(
[],
HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR,
$e->getMessage()
);
}
}
All the logic around calling the Adapter for the data, the error handling and logging is now here instead. I've also made a point of refactoring the main try / catch so that the "what it's doing" and "handling errors" is nicely separate.
All the code for this iteration of this work is tagged as 1.10 on Github.
Righto.
--
Adam
