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

[8.0] Support for Eloquent API Resources #1702

Merged
merged 18 commits into from
May 10, 2018
Merged

[8.0] Support for Eloquent API Resources #1702

merged 18 commits into from
May 10, 2018

Conversation

asahasrabuddhe
Copy link
Contributor

@asahasrabuddhe asahasrabuddhe commented Apr 11, 2018

Hello,

This PR aims to address the following issues:

#1515
#1659
#1351

Laravel 5.5 introduced to us Eloquent API resources where we could easily describe the format that we wanted for our API responses. This package currently does not support the new API resources. Through this PR, I have added a new engine to handle API Resources, updated the configuration and added a new test case to test the new functionality.

We can use the package as below now:

DataTables::of(App\Http\Resources\UserResource::collection(App\User::all()))->toJson();

or

datatables()->of(App\Http\Resources\UserResource::collection(App\User::all()))->toJson();

or

DataTables::resource(App\Http\Resources\UserResource::collection(App\User::all()))->toJson();

or

datatables->resource(App\Http\Resources\UserResource::collection(App\User::all()))->toJson();

Please review and let me know if there are any corrections and/or enhancements that can be done here if this PR is considered to be merged.

Thanks!

@yajra
Copy link
Owner

yajra commented Apr 17, 2018

Wow thanks! Will review this as soon as I can. ❤️

@yajra
Copy link
Owner

yajra commented Apr 17, 2018

Just to be clear, this is only for collection and no support yet for pagination right?

@asahasrabuddhe
Copy link
Contributor Author

asahasrabuddhe commented Apr 17, 2018

It does support pagination. I have tested with these types of URLs and got proper responses.

http://localhost/datatable?draw=1&start=10&length=10
{
draw: 1,
recordsTotal: 50,
recordsFiltered: 50,
data: [
{
email: "francis71@example.org",
name: "Doug McLaughlin I"
},
{
email: "chandler.cassin@example.org",
name: "Conner Anderson"
},
{
email: "feil.javon@example.org",
name: "Prof. Allen Turcotte PhD"
},
{
email: "dkuhic@example.org",
name: "Jordan Marks"
},
{
email: "helga78@example.com",
name: "Arvid Crooks"
},
{
email: "corwin.emelie@example.net",
name: "Russel Morissette"
},
{
email: "rae.price@example.com",
name: "Shyann Beahan"
},
{
email: "omedhurst@example.net",
name: "Therese Monahan"
},
{
email: "larkin.russ@example.com",
name: "Percival Lynch"
},
{
email: "rosamond.hermiston@example.net",
name: "Mrs. Zoey Stiedemann"
}
],
input: {
draw: "1",
start: "10",
length: "10"
}
}

Copy link
Owner

@yajra yajra left a comment

Choose a reason for hiding this comment

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

Please also remove file []? Thanks!

'eloquent' => \Yajra\DataTables\EloquentDataTable::class,
'query' => \Yajra\DataTables\QueryDataTable::class,
'collection' => \Yajra\DataTables\CollectionDataTable::class,
'anonymousresourcecollection' => \Yajra\DataTables\ApiResourceDataTable::class,
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we use shorter syntax like resource? Also this matches, public function resource($resource).

{
return $this;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this class can inherit from CollectionDataTable to avoid duplicate codes?

@yajra
Copy link
Owner

yajra commented Apr 17, 2018

Oh what I mean in pagination is App\User::paginate(10) as the resource value. :)

-- sorry, accidentally closed --

@yajra yajra closed this Apr 17, 2018
@yajra yajra reopened this Apr 17, 2018
@asahasrabuddhe
Copy link
Contributor Author

Oh what I mean in pagination is App\User::paginate(10) as the resource value. :)

This is not supported. I did not realise that this was supported for eloquent or collection engines. I can work on it if that is a requirement.

In the meanwhile, let me work on the couple of changes you have requested and do another commit.

@yajra
Copy link
Owner

yajra commented Apr 17, 2018

It's not yet a requirement but I think some users will look for it once this is merged. :)

@asahasrabuddhe
Copy link
Contributor Author

It's not yet a requirement but I think some users will look for it once this is merged. :)

This is working now. Look :

return DataTables::of(App\Http\Resources\UserResource::collection(App\User::paginate(10)))->toJson();

URL: http://localhost/datatable

Response:

{
draw: 0,
recordsTotal: 10,
recordsFiltered: 10,
data: [
{
email: "pkessler@example.com",
name: "Lucio Kessler"
},
{
email: "obergnaum@example.net",
name: "Hertha Lebsack"
},
{
email: "erohan@example.org",
name: "Tressie Dooley"
},
{
email: "pcassin@example.org",
name: "Florence West IV"
},
{
email: "joy53@example.net",
name: "Maggie Wunsch"
},
{
email: "nsporer@example.org",
name: "Peggie Hills"
},
{
email: "josiah79@example.com",
name: "Gerry Block"
},
{
email: "mitchell98@example.org",
name: "Jeffrey Kunze Jr."
},
{
email: "bbogisich@example.com",
name: "Alene Murray"
},
{
email: "ankunding.lavon@example.com",
name: "Suzanne Feeney"
}
],
input: [ ]
}

URL: http://localhost/datatable?draw=1&start=0&length=5

Response:

{
draw: 1,
recordsTotal: 10,
recordsFiltered: 10,
data: [
{
email: "pkessler@example.com",
name: "Lucio Kessler"
},
{
email: "obergnaum@example.net",
name: "Hertha Lebsack"
},
{
email: "erohan@example.org",
name: "Tressie Dooley"
},
{
email: "pcassin@example.org",
name: "Florence West IV"
},
{
email: "joy53@example.net",
name: "Maggie Wunsch"
}
],
input: {
draw: "1",
start: "0",
length: "5"
}
}

URL: http://localhost/datatable?draw=1&start=5&length=5

Response:

{
draw: 1,
recordsTotal: 10,
recordsFiltered: 10,
data: [
{
email: "nsporer@example.org",
name: "Peggie Hills"
},
{
email: "josiah79@example.com",
name: "Gerry Block"
},
{
email: "mitchell98@example.org",
name: "Jeffrey Kunze Jr."
},
{
email: "bbogisich@example.com",
name: "Alene Murray"
},
{
email: "ankunding.lavon@example.com",
name: "Suzanne Feeney"
}
],
input: {
draw: "1",
start: "5",
length: "5"
}
}

@yajra
Copy link
Owner

yajra commented Apr 17, 2018

The result only return the first page (10 records) and does not reflect the total count.

recordsTotal: 10,
recordsFiltered: 10,

@asahasrabuddhe
Copy link
Contributor Author

Now working on the remaining suggestions. Will do a commit soon! :)

@asahasrabuddhe
Copy link
Contributor Author

The result only return the first 10 page and does not reflect the total count.

I am getting the total count from collection->count. Will need to look this into detail. Can we have this part integrated first and then fix this later?

@asahasrabuddhe
Copy link
Contributor Author

The result only return the first page (10 records) and does not reflect the total count.

So in case of 50 users, if someone does this,

return DataTables::of(App\Http\Resources\UserResource::collection(App\User::paginate(10)))->toJson();

We will show totalRecords = 50 and filteredRecords=10?

@asahasrabuddhe
Copy link
Contributor Author

How about this now?

Code:

return DataTables::of(App\Http\Resources\UserResource::collection(App\User::paginate(10)))->toJson();

URL: http://localhost/datatable

Response:

{
draw: 0,
recordsTotal: 50,
recordsFiltered: 10,
data: [
{
email: "pkessler@example.com",
name: "Lucio Kessler"
},
{
email: "obergnaum@example.net",
name: "Hertha Lebsack"
},
{
email: "erohan@example.org",
name: "Tressie Dooley"
},
{
email: "pcassin@example.org",
name: "Florence West IV"
},
{
email: "joy53@example.net",
name: "Maggie Wunsch"
},
{
email: "nsporer@example.org",
name: "Peggie Hills"
},
{
email: "josiah79@example.com",
name: "Gerry Block"
},
{
email: "mitchell98@example.org",
name: "Jeffrey Kunze Jr."
},
{
email: "bbogisich@example.com",
name: "Alene Murray"
},
{
email: "ankunding.lavon@example.com",
name: "Suzanne Feeney"
}
],
input: [ ]
}

URL: http://localhost/datatable?draw=1&start=5&length=5

Response:

{
draw: 1,
recordsTotal: 50,
recordsFiltered: 10,
data: [
{
email: "nsporer@example.org",
name: "Peggie Hills"
},
{
email: "josiah79@example.com",
name: "Gerry Block"
},
{
email: "mitchell98@example.org",
name: "Jeffrey Kunze Jr."
},
{
email: "bbogisich@example.com",
name: "Alene Murray"
},
{
email: "ankunding.lavon@example.com",
name: "Suzanne Feeney"
}
],
input: {
draw: "1",
start: "5",
length: "5"
}
}

URL: http://localhost/datatable?draw=1&start=5&length=5

Response:

{
draw: 1,
recordsTotal: 50,
recordsFiltered: 10,
data: [
{
email: "nsporer@example.org",
name: "Peggie Hills"
},
{
email: "josiah79@example.com",
name: "Gerry Block"
},
{
email: "mitchell98@example.org",
name: "Jeffrey Kunze Jr."
},
{
email: "bbogisich@example.com",
name: "Alene Murray"
},
{
email: "ankunding.lavon@example.com",
name: "Suzanne Feeney"
}
],
input: {
draw: "1",
start: "5",
length: "5"
}
}

@asahasrabuddhe
Copy link
Contributor Author

@yajra Is this better?

@yajra
Copy link
Owner

yajra commented Apr 17, 2018

@asahasrabuddhe looking good. Noticed something though but a bit trivial:

recordsFiltered: 10,
length: "5"

The result above does not sync well, but I guess this can be fixed by making the paginated value equal to the length request.

Will do some actual testing but this looks like good to go :).

*/
public static function canCreate($source)
{
return is_array($source) || $source instanceof AnonymousResourceCollection;
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can remove is_array here since it's handled by CollectionDataTable.

return $source instanceof AnonymousResourceCollection;

/**
* Factory method, create and return an instance for the DataTable engine.
*
* @param array|Illuminate\Http\Resources\Json\AnonymousResourceCollection $source
Copy link
Owner

Choose a reason for hiding this comment

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

@param should be * @param \Illuminate\Http\Resources\Json\AnonymousResourceCollection $source

*/
public static function create($source)
{
if (is_array($source)) {
Copy link
Owner

Choose a reason for hiding this comment

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

We can remove the is_array part and let dt collection do the job.

    /**
     * Factory method, create and return an instance for the DataTable engine.
     *
     * @param \Illuminate\Http\Resources\Json\AnonymousResourceCollection $source
     * @return ApiResourceDataTable|DataTableAbstract
     */
    public static function create($source)
    {
        return new ApiResourceDataTable($source);
    }

@asahasrabuddhe
Copy link
Contributor Author

Hello @yajra ,

I went through your feedback and made some more changes. I have setup a small demo here that demonstrates two use-cases.

  1. User:all()
  2. User::paginate(30)

On the link, you will see the table along with the response coming from the package. Please review.

Link: http://35.196.1.30/datatable

Regards,
Ajitem

@yajra
Copy link
Owner

yajra commented Apr 18, 2018

I've seen the link and it works great. However, I think you accidentally deleted the count() method on code clean-up or it's intentional? I think the count override is needed to get the total count from paginator?

Anyways, will test this actual code later too. Thanks a lot!

@yajra
Copy link
Owner

yajra commented Apr 18, 2018

BTW, I think API resource is only available on 5.6. We need to check too if this will break 5.5 installations.

@asahasrabuddhe
Copy link
Contributor Author

Hello @yajra ,

I've seen the link and it works great. However, I think you accidentally deleted the count() method on code clean-up or it's intentional? I think the count override is needed to get the total count from paginator?

Thanks! The count method was intentionally removed.

BTW, I think API resource is only available on 5.6. We need to check too if this will break 5.5 installations.

API Resources were initially introduced in Laravel 5.5 Please refer the link below.

https://laravel.com/docs/5.5/releases

I don't think this will break 5.5 Nevertheless, I will do a test on my end and share results with you.

Regards,
Ajitem

@asahasrabuddhe
Copy link
Contributor Author

Hi,

I have checked on both 5.5 and 5.6. Here are the links:

http://dtl56.theapptest.xyz/datatable
http://dtl55.theapptest.xyz/datatable

Please review.

@asahasrabuddhe
Copy link
Contributor Author

Also, until we explicitly pass an API Resource to the Datatable instance, the new engine won't load. So, I don't think we are risking breaking any of existing installations.

@yajra
Copy link
Owner

yajra commented Apr 18, 2018

@asahasrabuddhe thanks for the feedback. I now found on whats confusing me. On the link you provided:

Collection returns:

    "recordsTotal": 50,
    "recordsFiltered": 50,

While paginate returns:

    "recordsTotal": 30,
    "recordsFiltered": 30,

Given that you are using the same data, we then expect that the records total for paginated results should be the same with collection. This is where we need to use the paginator total to reflect the actual total records. However, I think filtered records needs more work be done or we can just set it always equal to total records?

@asahasrabuddhe
Copy link
Contributor Author

@yajra I think it is best if we try to implement support for Paginate in a separate PR. I tried using User::paginate() with DataTables::of() method without wrapping it in the UserResource::collection() but it gave me an error of no engine found.

What I think is with Paginate, we can set start, length properties of datatable by default for the first load. They will remain the same until the user chooses to customise them.

Handling this separately will allow us to think and implement it in a better way. As far as I know, I have not seen a single datatable example that supports paging links in the response. What do you think?

@asahasrabuddhe
Copy link
Contributor Author

When I convert a pagianted resource to collection, i get all records on page 1. For now, we can leave it as it is. User::paginate can be used under ::collection(), ::resource() or ::of(). I will now start working on proper support for paginated resource for all of these methods.

We can discuss that and implement it separately from here as this PR is more focused on allowing use of API resources with the plugin.

@yajra
Copy link
Owner

yajra commented Apr 28, 2018

I agree with you on separating the support for paginated source.

On the other hand, lets wait some feedback from other users maybe for a week. If no feedbacks, I guess I can merge the PR then. Thanks!

@asahasrabuddhe
Copy link
Contributor Author

Hello @yajra

Any updates here?

Regards,
Ajitem

@yajra yajra changed the title Support for Eloquent API Resources [8.0] Support for Eloquent API Resources May 10, 2018
@yajra yajra merged commit 8fb61c3 into yajra:8.0 May 10, 2018
@yajra
Copy link
Owner

yajra commented May 10, 2018

Released on v8.5.0, thanks a lot 🍻

@yajra yajra mentioned this pull request May 10, 2018
yajra added a commit that referenced this pull request May 10, 2018
* 8.0:
  Bump v8.5.0 🚀
  [8.0] Support for Eloquent API Resources (#1702)
@yajra yajra added the need docs label Oct 2, 2018
@shamsuljewel
Copy link

Hi I am using API Resource and it working great, until I have tried add another resource into it. like
return [ 'id' => $this->id, 'name' => $this->name, 'users' => UserResource::collection($this->whenLoaded('getusers')),

when I loaded getusers then it's not using the UserResoure, it's just return me back the resource from database!!

"users": { "collects": "App\\Http\\Resources\\V1\\UserResource", "collection": [ { "resource": { "id": "141", "email": "test.platform@doorhub.io", "permissions": [], "last_login": "2019-10-14 16:50:23", "first_name": "Test",

where as I expected id, userName, etc.

Is it supported? or I am doing wrong?

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

Successfully merging this pull request may close these issues.

3 participants