Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] API Responses #19449

Closed
wants to merge 5 commits into from
Closed

[5.5] API Responses #19449

wants to merge 5 commits into from

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Jun 1, 2017

This PR Introduces a trait you can use on a Responsable:

class UserApiResponse implements Responsable
{
    use isApiResponse;

    public function __construct($resource)
    {
        $this->resource = $resource;
    }

    public function transformResource($resource)
    {
        return [
            'name' => $resource->name,
            'email' => $resource->email,
        ];
    }
}

The trait needs a $resource property to exist on your Responsable object, using this convention it's going to create a response for you based on the type of object you pass (Resource, Collection, Paginator), you have to implement a transformResource method to tell Laravel how to transform a single resource instance for the API.

Now in your controller you can return any of the following:

return new UserApiResponse(User::find(1));

return new UserApiResponse(User::paginate(15));

return new UserApiResponse(User::find([1,2]));

// You can pass a status code using:
return (new UserApiResponse($user))->withStatus(201);

// You can also do:
return UserApiResponse::with($user)->withStatus(201)->withMeta(['valid' => true]);

The responses will look like the following:

For a single resource:

{
  "data": {
    "name": "Prof. Alfonzo Schulist DDS",
    "email": "[email protected]"
  },
  "meta": {
    "value": true
  },
}

For when you pass a paginator:

{
    "data": [
        {
            "name": "Prof. Alfonzo Schulist DDS",
            "email": "[email protected]"
        },
        {
            "name": "Prof. Oma Armstrong PhD",
            "email": "[email protected]"
        }
    ],
    "meta": {
       "value": true,
        "current_page": 1,
        "from": 1,
        "last_page": 50,
        "next_page_url": "http://laravelnext.dev/controller?page=2",
        "path": "http://laravelnext.dev/controller",
        "per_page": 2,
        "prev_page_url": null,
        "to": 2,
        "total": 100
    }
}

For when you pass a collection:

{
    "data": [
        {
            "name": "Prof. Alfonzo Schulist DDS",
            "email": "[email protected]"
        },
        {
            "name": "Prof. Oma Armstrong PhD",
            "email": "[email protected]"
        }
    ],
    "meta": {
        "value": true
     },
}

@arcanedev-maroc
Copy link
Contributor

Hi @themsaid, can you change the metadata name to meta ?

Keep up the good work 👍

@arcanedev-maroc
Copy link
Contributor

arcanedev-maroc commented Jun 1, 2017

Hi again,

can you check the paginator response in your description:

{
    "data": [
        {
            "name": "Prof. Alfonzo Schulist DDS",
            "email": "[email protected]"
        },
        {
            "name": "Prof. Oma Armstrong PhD",
            "email": "[email protected]"
        }
    ],
    "meta": {
        "current_page": 1,
        "from": 1,
        "last_page": 50,
        "next_page_url": "http://laravelnext.dev/controller?page=2",
        "path": "http://laravelnext.dev/controller",
        "per_page": 2,
        "prev_page_url": null,
        "to": 2,
        "total": 100
    }
}

The last_page must be equals to 2 and the per_page equals to 50 (also the to value).

I know this is just an example but i'm confused to process it 😜

EDIT: NVM, otherwise you need to show 50 records in the data

{
if ($object instanceof Collection) {
return [
'data' => $this->user->map([$this, 'transformResource']),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be $object and not $this->user? Same applies below.

@sixlive
Copy link
Contributor

sixlive commented Jun 2, 2017

Would love to see a static factory especially with the withStatus() method.

return UserApiResponse::create(User::find(1))->withStatus(201);

I would also get some usage about adding misc meta. So maybe a ->withMeta($metaArray) or ->withMeta($key, $value) that would be merged in with the patination data if that exists.

Any thoughts on making the pagination meta its own key?

{
  "data": [
    {
      "name": "Prof. Alfonzo Schulist DDS",
      "email": "[email protected]"
    },
    {
      "name": "Prof. Oma Armstrong PhD",
      "email": "[email protected]"
    }
  ],
  "meta": {
    "pagination": {
      "current_page": 1,
      "from": 1,
      "last_page": 50,
      "next_page_url": "http://laravelnext.dev/controller?page=2",
      "path": "http://laravelnext.dev/controller",
      "per_page": 2,
      "prev_page_url": null,
      "to": 2,
      "total": 100
    }
  }
}

@themsaid
Copy link
Member Author

themsaid commented Jun 2, 2017

Added a ::with() static constructor as well as a withMeta() method.

return UserApiResponse::with(User::paginate(5))->withMeta(['test' => 123]);

@JacobBennett
Copy link
Contributor

This looks like an awesome addition! Would help a ton to be able to standardize the format that we use to return api data to the front end. With that in mind, are there any thoughts regarding a standard format when returning errors for api requests? For instance if I use a User::findOrFail(1), currently that will generate a HTML response instead of a JSON response. Maybe if using a Responsable, we can assume they want a JSON error instead and handle it appropriately?

Not sure if this is the correct venue for discussing it but figured I would give it a mention.

@sixlive
Copy link
Contributor

sixlive commented Jun 3, 2017

@JacobBennett I typically do this in the global exception handler since most of those types of exceptions you will want to handle in the same way. Also, you will want to look at the way 5.5 is going to render exceptions now, you might be able to bake it into that.

@themsaid
Copy link
Member Author

themsaid commented Jun 3, 2017

@JacobBennett in 5.5 if your API call has a Accept: application/json header laravel will transform all exceptions to JSON:

{
    "message": "Exception message"
}

@sixlive
Copy link
Contributor

sixlive commented Jun 4, 2017

Totally forgot to mention that. Would be nice if it would respond with JSON automatically if the route is an API route

@arcanedev-maroc
Copy link
Contributor

@sixlive, in your API routes (or Controllers) should respond with:

return response()->json(...);

Or something similar.

This PR is just a helper to transform objects without using external packages like fractal or even using response()->json(...).

@sixlive
Copy link
Contributor

sixlive commented Jun 4, 2017

Thanks! Well aware of what this PR is. Just making a side comment on global exception handling responses.

@DanielDarrenJones
Copy link

@sixlive This was added in a recent PR to fix for 5.5 #18732 and has been merged, the API should be standardised across Laravel going forward hopefully 👍

@sixlive
Copy link
Contributor

sixlive commented Jun 4, 2017

@DanielDarrenJones Oh sweet! Thanks mate! Totally missed that one!

@lkmadushan
Copy link
Contributor

Isn't it better to use class inheritance without using a trait. Something like,

ApiTransformer implements Responsable {
     ..
}
UserTransformer extends ApiTransformer {
    ...
}

Anyway it seems this is equivalent to league/fractal

@JacobBennett
Copy link
Contributor

Thanks for the heads up on the JSON response stuff for exceptions @themsaid, looks like 5.5 has it covered!

@antonkomarev
Copy link
Contributor

@lkmadushan you always could make your own class which will use trait under the hood.

use Illuminate\Database\Eloquent\Collection;
use Illuminate\Pagination\AbstractPaginator;

trait ApiResponseTrait
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we call this something else? Like ApiResponse or RespondsToApiCall or something without the Trait suffix..

Copy link
Contributor

@antonkomarev antonkomarev Jun 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @tillkruss. Suggesting one more option: HasApiResponse

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trait actually describes what the class is, so IsApiResponse sounds good to me.

Copy link
Contributor

@antonkomarev antonkomarev Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JosephSilber option is the best one.

*
* @var array
*/
public $metaData = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metadata is a single word, and should therefor not be camelcased.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't agree with it because result will be displayed in meta array key.

use Illuminate\Database\Eloquent\Collection;
use Illuminate\Pagination\AbstractPaginator;

trait ApiResponseTrait
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trait actually describes what the class is, so IsApiResponse sounds good to me.


use InvalidArgumentException;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Pagination\AbstractPaginator;
Copy link
Contributor

@lucasmichot lucasmichot Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should also require "illuminate/database": "5.5.*" and "illuminate/pagination": "5.5.*" in Illuminate/Http/composer.json

/**
* Create an HTTP response that represents the object.
*
* @return \Illuminate\Http\Response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@return \Illuminate\Http\JsonResponse instead

* Create an HTTP response that represents the object.
*
* @return \Illuminate\Http\Response
* @throws InvalidArgumentException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing backslash => @throws \InvalidArgumentException

@lucasmichot
Copy link
Contributor

Great @themsaid !

I think having withHeaders() and withOptions would be great too, so that we can use the headers and options values when returning the response:
return response()->json($this->transform(), $this->status, $this->headers, $this->options);

@themsaid
Copy link
Member Author

themsaid commented Jun 5, 2017

Thanks @lucasmichot and @JosephSilber, applied the following changes:

  • Renamed to isApiResponse
  • Renamed metaData to metadata
  • Added illuminate/pagination to composer.json
  • Changed the import to look for support/collection instead of eloquent/collection
  • Added withHeaders
  • Added withEncodingOptions

@@ -17,6 +17,7 @@
"php": ">=7.0",
"illuminate/session": "5.5.*",
"illuminate/support": "5.5.*",
"illuminate/pagination": "5.5.*",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be sorted ;-)

@JosephSilber
Copy link
Member

Shouldn't we use the ResponseTrait here?

@lucasmichot
Copy link
Contributor

I share @adiachenko opinions on that, @taylorotwell
@themsaid, any chance to release this code as a package ?

@themsaid
Copy link
Member Author

themsaid commented Jun 15, 2017

@taylorotwell said he'll think about it more, if he decided to not go for it then yes I think releasing it as a package would be nice :) Just hope this gets into the core since it's really simple but helps alot with creating responses.

@DanielDarrenJones
Copy link

I fully agree, JSON-API feels way to over spec for what 99% of people need.

Having something simple now would make such a huge improvement across the board for Laravel, is there a way the formatters could be pulled out to be configurable so if in the future it changes to JSON-API or another mapping it can be replaced by the user?

@joselfonseca
Copy link

This looks like what http://fractal.thephpleague.com does.

@deleugpn
Copy link
Contributor

deleugpn commented Jun 27, 2017

@joselfonseca I would go out on a limb here and say that 100% of people trying to PR API responses into Laravel have used and disliked Fractal.

@DanielDarrenJones
Copy link

@deleugpn Actually I really like fractal, especially when wrapped in https://github.com/spatie/laravel-fractal, that said it would be nice to have a unified global api response system in Laravel.

@joselfonseca
Copy link

@deleugpn Interesting, I find it quite good myself. Then again having something in Core would be nicer.

@adiachenko
Copy link

adiachenko commented Jun 28, 2017

@DanielDarrenJones Well, Fractal (kinda) does the job, but it's very awkward. Among other things:

  • it causes severe N+1 problems unless you are very careful about what you're doing and explicitly check whether relationship is loaded in Transformer's includes (which isn't even what suggested in the docs)
  • it performs complex query string parsing on each request
  • apparently, it has very rigid serializers. Say you namespace your response under data property, this will automatically propagate to relationships based on how Serialize interface is constructed which may not be what you want
  • it claims to offer multiple serializers out of the box, but it is apparently built with DataArraySerializer in mind and everything else is just an extra (btw, the one for JSON API won't work with clients that expect spec compliance like ember-data)
  • etc.

In short, there's no smoke without fire. Otherwise, we'll all be using Fractal by now instead of trying to push a similar solution into the core.

@taylorotwell taylorotwell reopened this Aug 17, 2017
@arcanedev-maroc
Copy link
Contributor

arcanedev-maroc commented Aug 17, 2017

Praise the 🌞 \[T]/

@taylorotwell
Copy link
Member

Something that would help me in this thread are answers to this question: what are we trying to solve? At the end of the day, implementations seems to boil down to something like this:

function transformPost($post)
{
    return [...];
}

function transformUser($user)
{
    return [...];
}

Everytime I start hacking on it a bit I come back to the core fact that we are just giving you a fancy way to declare a plain PHP function that returns an array. Is that all we're trying to do? Does everyone agree with that?

@themsaid
Copy link
Member Author

@taylorotwell basically yes, a way to convert an object to a PHP array to finally be converted to JSON.

We're also trying to bring the ability to automatically convert a Collection of objects or a pagination to a JSON format, where objects inside those collections are transformed using the toArray function we're adding in this feature.

@taylorotwell
Copy link
Member

And the main problem with using toArray on the model to do this is because you have multiple API versions? Or is there some other reason?

@DanielDarrenJones
Copy link

DanielDarrenJones commented Aug 18, 2017

@taylorotwell

The main reason for transformers in my opinion is to decouple the underlying attribute names or database table names from the API response, doing so means we can change the underlying data structure while still keeping consistencies for users.

It also gives us a chance to cast the type explicitly before it is transformed to JSON, this has its uses from time to time.

Having a base transformation structure of the model allows us to pull that in a different response templates and transform to standard API response formats much like JSONAPI or similar.

I think ultimately I am looking for something very similar to https://github.com/spatie/laravel-fractal, I want an easy way to model my API responses.

I think over the last year and a half especially since Laravel has become so close to Vue we have seen a strong shift towards API driven development and for Laravel to fully embrace this we need the tools to do it correctly.

@arcanedev-maroc
Copy link
Contributor

arcanedev-maroc commented Aug 18, 2017

I think the main reason of this feature is to have something similar to thephpleague/fractal out of the box.

Creating a plain PHP function doesn't seems as an Eloquent way to work with.

When creating a Transformer class, you can apply it to a Model, Eloquent Collection or a Pagination object.

Instead of using an external library, i think laravel already has more than 90% of features to handle the json responses (especially the transform method in Collection class).

Adding this feature is like adding the cherry on the cake.

UPDATE:
I use also the Nested Transformers:
UserWithPostsTransformer > UserTransformer (user model) + PostTransformer (posts relation)

@themsaid
Copy link
Member Author

@taylorotwell toArray could be used it tests or other places in the code, it's useful to have a function that's only purpose is to convert a resource to an API format, maybe do conversions, change float to integer, append strings, hide parts of a secret *******9876, etc...

@taylorotwell
Copy link
Member

OK thanks for your responses.

@lkmadushan
Copy link
Contributor

Is there any benefit using this instead of league/fractal?

@taylorotwell
Copy link
Member

@lkmadushan someone tried to answer that like 6 comments up.

@lkmadushan
Copy link
Contributor

@taylorotwell yeah. see those comments. but didn't see any special difference compared to league/fractal. it seems like we are doing same thing in different ways.

but It would be great if you can provide a JSON-API response format and a supported client for the frontend to consume JSON-API API service easily.

@Aferz
Copy link
Contributor

Aferz commented Aug 18, 2017

For me, the main purpose of features like this is being able to respond to a request in a very specific way with very specific data with common resources.

With attributes like hidden, append, or with inside models, we are just being able to declare general behaviour to this model for the whole application. Despite this is not a big problem, with this configuration we are assuming that when we want to display an user, we always want to display them with the same attributes, and we all know this is not true for every scenario.

We are trying to use common resources (Models) for specific responses. That's why generic behaviours doesn't match very well when we are trying to do such specific/independent things.

Fractal does a very good work in this area, but it is extremely verbose for a framework like Laravel and I and many like me think it could be done better. (No pretending to offense to PHPLeague, of course)

That's why I opened a PR (time ago) trying to figure out what is the Laravelish way to accomplish such work #18502. Is a bit old, and I didn't know about recent Responsable interface, so it could be handled better now, but you get the basic idea.

@deleugpn
Copy link
Contributor

@lkmadushan it's the same thing, but done right.

@taylorotwell
Copy link
Member

@deleugpn I'm curious to hear your problems with Fractal.

@antonkomarev
Copy link
Contributor

antonkomarev commented Aug 21, 2017

@lkmadushan JSON:API Server responses is a good thing to have out of the box, but JSON:API Client is task out of the Laravel scope. It's not a trivial job and there is no reason for the Laravel team to develop and maintain it.

@deleugpn
Copy link
Contributor

@taylorotwell at first, it's not beginners-friendly. You have to opt for dingo/api or spatie/laravel-factral. Although spatie does provide a good support for it, it's still pretty verbose.

As you mentioned yourself in the thread, we're basically seeking model-to-array transformation layer. The amount of overhead just for that is too high through Factral.

Factral include functionality is actually a Pandora's box. You think it's making your life easy by offering relationship transformation, but you end up with (already mentioned) N+1 problem unless you're extremely careful with covering all your entry points to the layer.

At the end of the day, I think we're trying to have simple (and laravel-friendly) transformers for simple things.

@taylorotwell
Copy link
Member

Done.

@arcanedev-maroc
Copy link
Contributor

PR: #20710

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.