Showing posts with label MVC. Show all posts
Showing posts with label MVC. Show all posts

Wednesday 1 February 2023

PHP: refactoring logic out of a controller and into a service class

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

Sunday 11 July 2021

Another thought on controllers and where the buck should stop

G'day:

I just wrote an article on what a controller ought to limit itself to: "What logic should be in a controller? (and a wee bit of testing commentary)". But then I read an old question from Mingo on the article that inspired that, and I have some thoughts on that too.

In most of my code examples around controller methods, I have this sort of method signature:

function handleGet(rawArgs)

What I mean by "raw args" is "all the stuff from an HTTP request, including: query string parameters and arguments (URL scope if yer a CFMLer), request body keys and values (form scope), general request metadata (CGI scope), cookies, and headers. Kind of like how Symfony would do it in the PHP world.

In CFML land things are seldom (never?) this organised. One seems to get a hotchpotch of things possibly put into a request context or something, or possibly - as in CFWheels - actually nothing(!!!) gets passed into the controller method; you just need to know the magical place to go look for them. And by "them" I mean a struct that has the form and URL scope munged into it. Ugh. Anyhow, there's all these elements of an HTTP request and the application lifecycle (application, session, request scopes) available to yer CFML code somehow.

And the controller is the only place one should ever access those. Your business logic should never be tightly-coupled the notion of "this stuff came from a specific sort of HTTP request", or from a specific stage in the application's lifecycle. Or even be aware of the ideas of HTTP requests and the like.

Your model tier should just get "values". Ideally by the time you are applying any actual application logic to them, the values will have been modelled into their own objects, not simply a bunch of primitive values.

I guess one could consider a controller to be similar in role to a repository class, just the other way around. A repository encapsulates the mapping between [for example] storage records - which it fetches - and collections of objects - which it returns - to the business-logic tier. A controller is slightly skinnier than that, it takes the values needed from that selection of HTTP request components (URL scope, form scope, what-have-you), and just passes them as independent values - distinct from how they arrived - to the model. I guess it's not a direct parallel because the controller doesn't do the "mapping" part that is intrinsic to a repository; it just removes the context from the values it receives, and leaves it up to the model tier to know what to do with them. But anyhow, there's a clear separation of concerns here, and the separation is that only the controller should ever deal with these things: CGI scope, the value returned from GetHttpRequestData(), form scope, URL scope, cookie scope, application scope, session scope, request scope.

Righto.

--
Adam

What logic should be in a controller? (and a wee bit of testing commentary)

G'day:

This topic has come up for me twice from different directtions in the last week or so, so I'm gonna dump some thoughts. I've actually discussed this before in "I actively consider what I think ought to go into a controller", and the conclusion I came to doesn't quite fit with how I'm writing code now so I'm gonna revise it a bit.

To start with though, I'll reiterate this question: "should this go in the controller?", and I'll repeat Mingo's answer that is still pithy but spot on:

Isn't the answer always "No"?
~ mjhagen 2017

This is a good starting point. 95% of the time if yer asking yerself that question, Mingo has answered it for you there.

The example of what I'd put in a controller from that article is along these lines:

class ContentController {
    function handleGet(rawArgs){
        args = validationService.validate(rawArgs)
        
        content = contentService.getById(args.id)
        
        response = {
            articles = content.published,
            socialContent = {
                twitter = content.twitter,
                facebook = content.facebook
            }
        }
        return new Response(response)
    }
}

It's not the worst code I've written, but I now think this is wrong. The problem lies with how I had a habit of abusing the Anaemic Domain Model pattern in the past, where I had a bunch of really skinny service classes, and used them to apply behaviour to behaviourless model objects that were just bags of data. Not great.

Looking at that code now, I see these lines:

args = validationService.validate(rawArgs)
content = contentService.getById(args.id)

And I think "nah, those don't belong there. They belong in the model too". I'm doing too much "asking" when I should be "telling" (see "TellDontAsk" by Martin Fowler).

Basically the model here should know what it is to be "valid", so just give it the raw data and let it crack on with it.

My generic controller method these days would be formed along these lines:

function handleRequest(rawRequestValues) {
    try {
        dataFromModel = someModel.getSomeDataFromThisLot(rawRequestValues)
        
        renderedResponse = viewService.renderView("someView", dataFromModel)
        
        return new HtmlResponse(renderedResponse)
        
    } catch (ClientException e) {
        return new ClientErrorResponse(e)
    }
}

Here we clearly have a separation of controller, model and view. It's the controller's job to marshal getting values to a model, and getting the values from that to a view, deal with any error responses that might arise due to those two, or return what came back from the view tier as the response. That's it.

There's an assumption that the framework will deal with any unhandled exceptions there as a controlled 5xx type response. Also there could well be more catch statements, if different types of exception could bubble out of the model, for instance a ValidationException which returns details of validation failures in its response; or a 404 response being returned if a UserNotFoundException came back from some business-logic validation or whatever. But that's the pattern.

The key here is that the only time I'm using a value created by the model is to pass it to the view. I do not pass it to anything else in the interim, like some other model call. That action is not controller logic. It's business logic that we get an x and then pass it to a y. It should be encapsulated in the model.

On the other hand if there was more than one piece of view data to be derived directly from the incoming request values, then that would to me still possibly be legit to be in the controller, eg this is OK:

dataFromModel = someModel.getSomeDataFromThisLot(rawRequestValues)
moreDataFromDifferentModel = someModelOther.getSomeDifferentDataFromThisLot(rawRequestValues)

This would not be OK:

dataFromModel = someModel.getSomeDataFromThisLot(rawRequestValues)
moreDataFromDifferentModel = someModelOther.getSomeDifferentDataFromThisLot(dataFromModel.someValue)

It's a small distinction. But the thing to focus on more than that small example is just to be thinking "no" when you ask yerself "does this belong in the controller?". You're more likely to be right than wrong.


How do we apply that pattern to the example in the old article? Like this I think:

// UserContentController.cfc
component {

    function init(ViewService viewService, UserContentFactory userContentFactory) {
        variables.viewService = arguments.viewService
        variables.userContentFactory = arguments.userContentFactory
    }

    function getContent(rawArgs) {
        try {
            userContent = userContentFactory.getUserContent().loadContentByFilters(rawArgs)
            
            renderedResponse = viewService.renderView("userContentView", userContent)
            
            return new HtmlResponse(renderedResponse)
            
        } catch (ValidationException, e) {
            return new ClientErrorResponse(400, e)
        } catch (UserNotFoundException e) {
            return new ClientErrorResponse(404, e)
        }
    }
}

(I've changed what the controller is returning so as to still integrate the view tier into the example).

I've done away with the controller handling the validation itself, and left that to the model. If things don't pan out: the model will let the controller know. That's it's job. And it's just the controller's job to do something about it. Note that in this case I don't really need both catches. I could just group the exceptions into one ClientException, probably. But I wanted to demonstrate two potential failures from the logic in loadContentByFilters.


What's with this factory I'm using? It's just one of my idiosyncrasies. I like my models' constructors to take actual valid property values, like this:

// UserContent.cfc
component accessors=true invokeImplicitAccessor=true {

    property publishedContent;
    property twitterContent;
    property facebookContent;

    function init(publishedContent, twitterContent, facebookContent) {
        variables.publishedContent = arguments.publishedContent
        variables.twitterContent = arguments.twitterContent
        variables.facebookContent = arguments.facebookContent
    }

Our UserContent represents the data that are those content items. However we've not been given the content items, we've just been given a means to get them. So we can't just create a new object in our controller and slap the incoming values into them. We need to have another method on the UserContent model that works with what the controller can pass it:

function loadContentByFilters(required struct filters) {
    validFilters = validationService.validate(filters, getValidationRules()) // @throws ValidationException
    
    user = userFactory.getById(validFilters.id) // @throws UserNotFoundException
    
    variables.publishedContent = contentService.getUserContent(validFilters)
    variables.twitterContent = twitterService.getUserContent(validFilters)
    variables.facebookContent = facebookService.getUserContent(validFilters)
}

And this demonstrates that to do that work, UserContent needs a bunch of dependencies.

I'm not going to pass these in the constructor because they aren't 100% needed for the operation of a UserContent object, and I want the constructor focusing on its data. So instead these need to be injected as properties:

// UserContent.cfc
component accessors=true invokeImplicitAccessor=true {

    property publishedContent;
    property twitterContent;
    property facebookContent;

    function init(publishedContent, twitterContent, facebookContent) {
        variables.publishedContent = arguments.publishedContent
        variables.twitterContent = arguments.twitterContent
        variables.facebookContent = arguments.facebookContent
    }
    
    function setValidationService(ValidationService validationService) {
        variables.validationService = arguments.validationService
    }
    
    function setUserFactory(UserFactory userFactory) {
        variables.userFactory = arguments.userFactory
    }
    
    function setContentService(UserContentService contentService) {
        variables.contentService = arguments.contentService
    }
    
    function setTwitterService(TwitterService twitterService) {
        variables.twitterService = arguments.twitterService
    }
    
    function setFacebookService(FacebookService facebookService) {
        variables.facebookService = arguments.facebookService
    }

That's all a bit of a mouthful every time we want a UserContent object that needs to use alternative loading methods to get its data, so we hide all that away in our dependency injection set-up, and use a factory to create the object, set its properties, and then return the object:

// UserContentFactory.cfc
component {

    function init(
        ValidationService validationService,
        UserFactory userFactory,
        UserContentService contentService,
        TwitterService twitterService,
        FacebookService facebookService
    ) {
        variables.validationService = arguments.validationService
        variables.userFactory = arguments.userFactory
        variables.contentService = arguments.contentService
        variables.twitterService = arguments.twitterService
        variables.facebookService = arguments.facebookService
    }

    function getUserContent() {
        userContent = new UserContent()
        userContent.setValidationService(validationService)
        userContent.setUserFactory(userFactory)
        userContent.setContentService(contentService)
        userContent.setTwitterService(twitterService)
        userContent.setFacebookService(facebookService)
        
        return userContent
    }
}

The controller just needs to be able to ask the factory for a UserContent object, and then call the method it needs, passing its raw values:

userContent = userContentFactory.getUserContent().loadContentByFilters(rawArgs)

You'll noticed I kept the validation separate from the UserContent model:

function loadContentByFilters(required struct filters) {
    validFilters = validationService.validate(filters, getValidationRules()) // @throws ValidationException

(And then there's also this private method with the rules):

private function getValidationRules() {
    return {
        id = [
            {required = true},
            {type = "integer"}
        ],
        startDate = [
            {required = true},
            {type = "date"},
            {
                range = {
                    max = now()
                }
            }
        ],
        endDate = [
            {required = true},
            {type = "date"},
            {
                range = {
                    max = now()
                }
            }
        ],
        collection = [
            {callback = (collection) => collection.startDate.compare(collection.endDate) < 0}
        ]
    }
}

Validation is fiddly and needs to be accurate, so I don't believe how to validate some values is the job of the UserContent class. I believe it's just perhaps its job to know "what it is to be valid". Hence that separation of concerns. I could see a case for that private method to be its own class, eg UserContentValidationRules or something. But for here, just a private method is OK. Wherever those rules are homed, and whatever the syntax of defining them is, we then pass those and the data to be validated to a specialist validation service that does the business. In this example the validation service itself throws an exception if the validation fails. In reality it'd more likely return a collection of rules violations, and it'd be up to the model making the call to throw the exception. That's implementation detail not so relevant to the code here.


There's probably more off-piste code in this an on-~, but I think it shows how to keep yer domain / business logic out of your controllers, which should be very very light, and simply marshall the incoming request values to the places that need them to be able to come up with a response. That's all a controller ought to do.


Oh before I go. There's an attitude from some testing quarters that one doesn't test one's controllers. I don't actually agree with that, but even if I did: that whole notion is predicated on controllers being very very simple, like I show above. If you pile all (or any of ~) yer logic into yer controller methods: you do actually need to test them! Even in this case I'd still be testing the flow control around the try/catch stuff. If I didn't have that, I'd probably almost be OK if someone didn't test it. Almost.

Righto.

--
Adam