Added ApiProblemException.

[simensen]

Fun times in testing land, let me tell you what. :)

[Crell]

I am not convinced of the utility of this. It means using exceptions as flow-control, which is generally a bad idea. (I know Symfony does it for 40x errors, which is not necessarily a compliment.) Every time I've used ApiProblem, it's been on the receiving end of an Exception, in a catch (or Symfony kernel::EXCEPTION listener). An ApiProblem is a domain object. Use it as such.

[simensen]

@Crell That is fair. The utility is subtle and almost strictly related to functional testing of API calls on the client side. Without any additional context I can see where this might not be obvious. I'll share my use case to see if this makes more sense.

I have an API Test Case that expects the happy case to be an application/hal+json content type. If not, the API Test Case consumes the rendered output of an ApiProblem instance from the response body if the application/api-problem+json content type is detected. Otherwise, it throws a RuntimeException for an unexpected content type.

Here is part of my ApiTestCase:

<?php
class ApiTestCase extends WebTestCase
    /**
     * @param callable $configureContainer
     * @param $expectedResponseCode
     * @param $method
     * @param $uri
     * @param array $params
     * @param array $files
     * @param array $server
     * @param null $content
     * @return ApiResponse
     * @throws \RuntimeException
     * @throws ApiProblemException
     */
    public function makeContainerizedApiRequest(
        callable $configureContainer,
        $expectedResponseCode,
        $method,
        $uri,
        $content = null,
        array $params = [],
        array $files = [],
        array $server = []
    ) {
        $client = $this->createClient(['environment' => 'test']);

        $configureContainer($client->getKernel()->getContainer());

        $client->request($method, $uri, [], [], [
            'HTTP_ACCEPT' => 'application/hal+json; application/api-problem+json',
        ], $content);
        $response = $client->getResponse();

        if (500 === $response->getStatusCode()) {
            print_r([
                'uri' => $uri,
                'headers' => $response->headers,
                'content' => $response->getContent(),
            ]);
        }

        $this->assertEquals($expectedResponseCode, $response->getStatusCode());

        if ($response->headers->get('Content-Type') === 'application/hal+json') {
            // This is the expected response from a successful API call.
            return new ApiResponse($client);
        }

        if ($response->headers->get('Content-Type') === 'application/api-problem+json') {
            // This is a KNOWN exceptional case.
            throw new ApiProblemException(ApiProblem::fromJson($response->getContent()));
        }

        // This is an exceptional case.
        throw new \RuntimeException("Unsupported Content Type detected");
    }

    /**
     * @param $expectedResponseCode
     * @param $method
     * @param $uri
     * @param array $params
     * @param array $files
     * @param array $server
     * @param null $content
     * @return ApiResponse
     * @throws \RuntimeException
     * @throws ApiProblemException
     */
    public function makeApiRequest(
        $expectedResponseCode,
        $method,
        $uri,
        $content = null,
        array $params = [],
        array $files = [],
        array $server = []
    ) {
        return $this->makeContainerizedApiRequest(
            function () {},
            $expectedResponseCode,
            $method,
            $uri,
            $content,
            $params,
            $files,
            $server
        );
    }
}

I feel like this is a valid use case for an Exception.

It also makes the PHPUnit tests look pretty nice:

<?php
class GetBrandControllerTest extends ApiTestCase
{
    /** @test */
    public function it_gets_an_existing_brand()
    {
        $brandId = (string) Uuid::uuid4();
        $brand = new Brand($brandId, 'AAA', 'Abba Abba Alma');

        // Ensure that our expected Brand Read Model exists
        $this->getBrandReadModelRepository()->save($brand);

        // Discover the URI for our Brand.
        $uri = $this->discoverUri('dt:inventory:brand', [
            'brandId' => $brandId,
        ]);

        // Make an API Request that we assume will work and will
        // return an API Response that we can work with.
        $apiResponse = $this->makeApiGetRequest(
            Response::HTTP_OK,
            $uri
        );

        $hal = $apiResponse->getHal();

        $this->assertEquals($uri, $hal->getUri());
        $this->assertEquals($brandId, $hal->getData()['brandId']);
        $this->assertEquals('AAA', $hal->getData()['productLineCode']);
        $this->assertEquals('Abba Abba Alma', $hal->getData()['name']);
    }

    /**
     * @test
     * @expectedException        ApiProblemException
     * @expectedExceptionCode    404
     * @expectedExceptionMessage Not Found
     */
    public function it_gets_an_error_on_a_missing_brand()
    {
        $brandId = (string) Uuid::uuid4();

        $uri = $this->discoverUri('dt:inventory:brand', [
            'brandId' => $brandId,
        ]);

        // This brand does not exist. We should make an API Request
        // and expect that an API Problem will be the result with
        // a code of 404 and message of Not Found.
        $apiResponse = $this->makeApiGetRequest(
            Response::HTTP_NOT_FOUND,
            $uri
        );
    }
}

The key part is that second test. Doing this I'm able to expect an Exception using PHPUnit.

I could of course change my ApiResponse object (which is only used for testing) to optionally contain the ApiProblem but it might be more work for me.

If you are not OK with including this Exception I might find a way to distribute it in my own package somehow. I've been working on building out my own ApiProblemBundle so I could probably just ship it in that.

I think it would be a useful addition to the core package, though. :) For testing!

[simensen]

but it might be more work for me

By that, I mean that I'd have to check to see if the API Response was really an API Problem or not. Right now that is all built in. If I ever get anything back from an API request that is not the happy path I get an exception right away.

I'm sure I could still swing this but I'd love to hear whether you think this use of Exceptions is still too flow controlly in this context. :)

[Crell]

You're making an ApiProblemBundle? Is it similar to the one I did that you have access to? ;-)

I can see the cleanup you're after; I agree the second version is nicer. But I am still not comfortable with baking in a test-only use case to the ApiProblem object itself. That feels like the wrong place for that logic, and I fear it would encourage people to use ApiProblemException as a flow-control tool. Rather, that logic belongs as part of an api-problem-aware (lower-case, meaning the spec, not this library) testing client that knows to check for and convert problematic responses.

I'm not sure how to do that in a way that isn't coupled to a specific HTTP implementation, such as HttpFoundation. A utility that converts an api-problem Response to an exception and throws it (or similar) seems useful, but I don't know if this library is the place for it. (If so, it should at least not be in the ApiProblem class itself.)

[simensen]

Well, I'm not actually using asException anywhere yet, so if that method is the problem you have, I'm happy to remove that.

Even in my current implementation I am just throwing new ApiProblemException($apiProblem). I thought it fit well with some of the other similarly named methods:

• toXml
• fromXml
• toJson
• fromJson
• toException

---

You're making an ApiProblemBundle? Is it similar to the one I did that you have access to? ;-)

Similar in that it is essentially on listener class? If so, yes. :)

We discussed this in November or December. I've started using your ApiProblem package (not the Bundle) on other Symfony based projects. As these projects are Symfony based I needed to integrate ApiProblem into them.

I asked you if you were going to release your Bundle and you said maybe but that if it did happen it would not be for awhile. You said that I should roll my own in the meantime. You also mentioned that these types of event listeners are pretty thin and that there is not a whole lot to them. So I created my own Bundle for internal use.

I did my best to create mine from scratch and not leverage any code that I did not own. However, as you mentioned yourself, the event listener is very simple so there are bound to be similarities, especially since most of it is Symfony boilerplate.

Right now my implementation is sitting in a client's repo and it would take effort for me to extract it. However, at some point, I'm going to want to be actually spend the time to extract it into its own package so that I can use it in other projects. I suppose we can talk about it more at that point to see where you are at with your Bundle and whether it makes sense to release my publicly or only make it available internally for dflydev's use.

Either way, if you have concerns about this maybe we can take it up on IRC? :)

[Crell]

I've thought about this a bit more (aka, just saw it). I don't want to put toException() on the ApiProblem class itself. However, I'm OK with the exception class that composes the problem. Can you just do that, then refactor the test accordingly? (The exception should maybe extend RuntimeException, too.)

[Crell]

I guess we're not going to get back to this anytime soon. :smile: