Mutable Evil

Once upon a time I stumbled upon a bug. I saw it in the code. Well, to be more precise, I wrote a test for it (that passed) and then spent 3 hours looking for it elsewhere. And then, when everything else was ruled out, I saw it in the code and realized that my test (and pretty much everything else) was wrong.

But let’s start from the beginning, so I had a piece of PHP Symfony code, looking like this (simplified):

class ActionController extends Controller
{
    public function processAction(Request $request)
    {
        $encryptor = $this->container->get("object_encryptor");
        $encryptor->setComponentId($request->get("component"));
        $encryptor->setProjectId($request->get("project"));
        $data = json_decode($request->getContent());
        $config = $encryptor->decrypt($data);
        $runner = $this->container->get('runner');
        $output = $runner->run($config, $request->get("component"));
        return new JsonResponse($output->getProcessOutput());
    }
}

The thing takes JSON from the request, passes it through the decrypt method of $encryptor and runs it. The result is then returned as a JSON response. The encryptor has a component parameter set which allows it to decipher specific set of encrypted values. Pretty simple, ain't it?

Still, it didn't work. I created a request with the encrypted value, ran the API call and the value was not decrypted and everything failed. Because everything is thoroughly tested and known to work, I immediately suspected that the encryptor is not initialized properly. So I wrote a test to check this:

class ActionControllerTest extends WebTestCase
{
    public function testDecryptNewSuccess()
    {
        // initialize encryptor
        $encryptor = self::$container->get('object_encryptor');
        $encryptor->setProjectId('123');
        $encryptor->setComponentId('dummy-component');
        $encryptor->setStackId(parse_url(
            self::$container->getParameter('storage_api'),
            PHP_URL_HOST
        ));
        $encrypted = $encryptor->encrypt(
           'password',
           ComponentWrapper::class
        );
        // create the API request and execute it
        $request = $this->prepareRequest(
            'decrypt',
            ["#password" => $encrypted]
        );
        self::$container->get('request_stack')->push($request);
        self::$container->set(
            'storage_api',
            $this->getStorageMock()
        );
        $ctrl = new ActionController();
        $ctrl->setContainer(self::$container);
        $ctrl->preExecute($request);
        $response = $ctrl->processAction($request);
        // verify
        $this->assertEquals(200, $response->getStatusCode());
        $this->assertEquals(
           '{"password":"password"}', 
           $response->getContent()
        );
    }
}

The test was doing the exact same thing that I was doing manually (create JSON with encrypted #password and feed it to the API endpoint). And surprise! surprise! It passed without problems. So I assumed that my first assumption was wrong and that turned my attention to other potential problems.

With more experiments I gathered more information and everything was pointing at the encryptor not being initialized properly. So I gave the action method one more thorough eagle-eye look and almost immediately spotted that the encryptor is indeed not initialized properly, because it is missing quite a crucial statement:

$encryptor->setStackId(parse_url(
    $this->container->getParameter(‘storage_api’),
    PHP_URL_HOST
));

The fix is trivial (add that line to the controller), but the question is, why is the test passing? That is also, in fact, really simple. First, I get the encryptor service from the DI container:

$encryptor = self::$container->get(‘object_encryptor’);

Then call a method which changes the internal state of that service:

$encryptor->setStackId(parse_url(
    $this->container->getParameter(‘storage_api’),
    PHP_URL_HOST
));

Then I pass the DI container to the controller being tested:

$ctrl->setContainer(self::$container);

So obviously, the DI container is the same (this is because I’m not using the standard way of testing via clients (and I’m doing that because I need to mock the storage_api service)) and it obviously creates the service only once (that’s one of the features of DI container). So obviously, the call to

self::$container->get(‘object_encryptor’)

is going to return the same object both in the test and controller. So obviously, that object is going to keep its state. That means that it’s obviously going to be initialized properly in the controller, because I initialized it at the beginning of the tests.

Solution? Apart from redesigning the whole thing, this will do:

public function testDecryptNewSuccess()
{
    // initialize encryptor
    $encryptor = clone self::$container->get(‘object_encryptor’);
    ...

Because I’m not testing the encryptor, I shouldn't have used the one from the container in the first place. Unfortunately its initialization is a bit complex, so to save time I want to use the one from the DI container. But that means, I must take extra care not to modify it!

Morals of the story:

• The DI container is a global variable.
• So are all its services.
• That means they share state between tests and tested stuff.
• If that state is changed in the test, it’s going to be changed in the tested method too.
• A global mutable object is the root of all evil (right after premature optimization).
• Use Symfony createClient when possible, because that does not share the container.

Since we all already know all of that, this article was completely useless, but thanks for reading anyway.