phly / PhlyRestfully

ZF2 module for creating RESTful JSON APIs using HAL and API-Problem
108 stars 45 forks source link

API Problem Questions... #48

Closed telkins closed 11 years ago

telkins commented 11 years ago

(This issue should probably be marked as a question.)

I'm currently focusing on implementing input handlers to filter and validate input on POST/create and PUT/update requests. When validation fails, I would like to make sure that an API Problem response with sufficient information is returned.

It's no problem returning an API Problem response, since PhlyRestfully does this automatically, for the most part. It's quite nice. I'm struggling, though, in trying to find ways to populate some of the Problem API fields.

Here's a typical response that might be returned from an invalid POST/create request:

{
    "describedBy": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html",
    "title": "PhlyRestfully\\Exception\\CreationException",
    "httpStatus": 406,
    "detail": "invalid input fields:mainContactId"
}

When I throw an exception, I'm throwing a PhlyRestfully\Exception\CreationException, as you can see. I'm also setting the exception's code property to 406, which seems to be the most appropriate for indicating invalid arguments. PhlyRestfully uses the code to generate the httpStatus field. It uses the exception's message to set the detail field. But, the title field is set based on a variety of conditions, including the httpStatus/code, the ApiProblem class's problemStatusTitles property, and the name of the exception.

Here are my questions:

  1. Is there a better way to populate the title?
  2. If the general approach to titles is to remain unchanged, can problemStatusTitles be expanded to match what we find in Zend\Http\Response::recommendedReasonPhrases...or at least be customizable?
  3. How does one go about setting the describedBy field?
  4. Should a PhlyRestfully Exception class be created/expanded that allows one to provide some/more of the API Problem fields in a more direct manner?
  5. Is there a better overall approach that I could/should be taking that I've missed?

I believe that covers my questions at this point in time. Thanks in advance for any help.

telkins commented 11 years ago

@weierophinney Why can't I see this on the issues list? I submitted it a few days ago, it's not closed, but I can't see it listed. Am I missing something?

telkins commented 11 years ago

@weierophinney Nevermind...I see that I had some filtering on that I didn't realize. My bad. :-(

weierophinney commented 11 years ago

On the current master/develop branches, besides throwing an exception from a resource listener, you can also specifically return an ApiProblem instance, which allows you set any fields you want however you want; see the ApiProblem constructor for details on how you would do so.

I'm definitely open to expanding the mapping of titles to include more common status code => title pairs; feel free to submit a pull request if you already have some in mind.

telkins commented 11 years ago

@weierophinney Thanks for the feedback.

I see how one can use an ApiProblem instance to do some of these things, but I really liked the idea of being able to simply throw a PhlyRestfully\Exception\CreationException instance in order "to indicate an inability to create an item and automatically report it." However, doing the latter appears to mean losing some of the power of using an ApiProblem instance.

Any ideas on how to make PhlyRestfully\Exception\CreationException automagically provide these fields (more) nicely to the ApiProblem instance later on? If not, then I will either abandon the exception class or put try..catch wrappers around all of my listeners' onCreate() methods to handle translating the exception class to a well-populated ApiProblem instance.

As for "expanding the mapping of titles to include more common status code => title pairs", I'm considering submitting a PR that simply lists all of the codes in Zend\Http\Response, or at least a larger subset. Thoughts?

telkins commented 11 years ago

@weierophinney FYI, I've attempted to address the expansion of PhlyRestfully\ApiProblem::problemStatusTitles in issue #55 and PR #56.

weierophinney commented 11 years ago

@telkins One thing we could do is use the code and message from the thrown exception. Right now, it assumes they're empty. If we used the code and message, you could simply throw the exception like you currently like, but provide the code (httpStatus) and message (title and/or description). Would that work for you?

telkins commented 11 years ago

@weierophinney The short answer to your question is yes and no.

The long(er) answer....

Using the code and message is what is already supported...unless I'm missing something.

The JSON API Problem output that I mentioned in this question was generated by the following code:

        return new \PhlyRestfully\Exception\CreationException($detail, 406);

So, the message becomes the API Problem detail and the code is used to generate the httpStatus. The title is determined by a combination of factors, it seems, but if the code is found in problemStatusTitles, then that message is used, otherwise it's the class name of the Exception itself. There doesn't appear to be a way to directly set/affect describedBy.

So, getting httpStatus, describedBy, detail, and title out of code and message seems a bit difficult...at least to me. So, unless you (or someone else) can see a different way to accomplish this, then my plan is to use the built-in exceptions, like PhlyRestfully\Exception\CreationException where it makes sense and gives me what I want, and to use PhlyRestfully\ApiProblem otherwise. I think this might be a bit more coding and a little uglier in the resource listeners, but it's not as terrible as I had first imagined. Since the listeners are very focused on a specific operation, then the number of possible ways to go wrong doesn't seem like it will become unwieldy.

I should also admit that I'm still struggling a bit when it comes to events/listeners. I can easily see how they work when shown to me in examples, but I'm not yet to the point where I'm able to find ways to use them where they haven't been shown to me. It's possible there are ways to use events/listeners here to make things a little cleaner, to apply DRY, and to take advantage of both ZF2 and PhlyRestfully. I just can't see it...yet. ;-)

I hope that makes sense.

weierophinney commented 11 years ago

So, getting httpStatus, describedBy, detail, and title out of code and message seems a bit difficult

That's solveable -- I can add setters for the describedBy and title fields on the exceptions. Usage would then be like this:

$ex = new CreationException($detail, 406);
$ex->setTitle('Parent not found');
$ex->setDescribedBy('http://example.org/api/problems/406');
throw $ex;

If that sounds reasonable, I'll add that today.

telkins commented 11 years ago

@weierophinney That sounds great. I wasn't sure how best to approach this. Obviously, PhlyRestfully\Exception\CreationException could be extended to have properties beyond its superclass, but I wasn't sure if that was considered good practice or not. Often I look at the way you and others I try to follow implement different things and I've never noticed you extending an exception class in such a way. So, if you're happy with such a solution and feel it's a good design, then I'm all for it.

Actually, I did just remember something. I know PhlyRestfully is intended to "talk JSON" and to respond with either a HAL or a Problem API response. I was wondering if, in the future, there would be a desire/need to have PhlyRestfully provide different kinds of responses, based on which one was configured, for example. I don't know if that makes sense, but if it does, then the extended Exception classes might be too "tied down to" the kind of response that will be provided in the event of a problem.

So...that would be my only concern. Otherwise, as I said, that sounds great.

weierophinney commented 11 years ago

@telkins --

I was wondering if, in the future, there would be a desire/need to have PhlyRestfully provide different kinds of responses, based on which one was configured,

Yes, this is planned. HAL is actually not JSON specific, and can be used in XML, too; I'd like to do that in the future. Additionally, I'd like to allow using vnd.error instead of API-Problem if desired -- which would require a different way of aggregating and reporting errors. The formats are not terribly different, however, so I think making these changes to the exception classes will not introduce odd verbiage.

telkins commented 11 years ago

@weierophinney Nice, then, on all counts. :-)