Closed lindyhopchris closed 6 years ago
v0.9
released with:
Hi Chris,
Awesome to see how the road towards v1.0
is getting closer and closer!
I'm up for working on converting/modernizing the tests. But may I suggest using orchestral/testbench? I used it when I developed the dixieio/eloquent-model-future and it just made writing tests using laravel facades and configs a breeze and super reliable.
If like the approach I'm up for throwing a PR tonight. 😄
Hi @jstoone
Your help with the tests would be super appreciated and would definitely speed up getting to 1.0
.
I'd be happy to use testbench where it is not possible to write unit tests. Generally I'm trying to write the majority of units so that they are unit testable - i.e. their dependencies are Illuminate contracts. E.g. the LinkFactory
takes a UrlGenerator
dependency, which is a contract so should be unit tested as such.
There are however some parts of the package where that's not possible - and the part the really jumps to mind is anything to do with Eloquent models.
So what I'd suggest is we create two sub directories in the tests
folder - Unit
and Integration
. The Integration
folder can hold any tests that need testbench. And then maybe if you started with those, seeing as I've never used testbench before, and concentrate on the Eloquent stuff?
If that sounds like a plan, I'll push a commit to the develop
branch with the new test structure and then you can fork off that commit. (I was also planning to drop support for Laravel 5.3 on develop
, so will do that at the same time.)
@jstoone I've restructured the tests on develop
. I'm heading off now so will be back online tomorrow morning (UK time)
@lindyhopchris Sounds great, and the Unit
and Integration
split seems very obvious. I'll get started on the migration to testbench sometime tomorrow afternoon, and hopefully reach a point where I can submit a WIP PR, which we can use as a base for discussion.
Exciting times! 🔥
@jstoone great tip on the testbench repo - just used that in a package at work and it makes things a lot simpler.
i was going to start setting up the tests this evening unless you'd already started?
@lindyhopchris it is really a gem when it comes to testing features that are more tightly coupled to Laravel. Also great that you've had the opportunity to play around with it. 👍
I have started setting them up, but then life entered the door and denied me access to my spare time.
I was planning on finishing it around Saturday or Sunday, but with that said if you want to get it done tonight feel free to take over. I haven't produced any progress on the setup, mainly playing around to see what would feel the best. I'll pick something else on the checklist for the weekend json-api hackathon. 🎉
Also in general, if you feel like getting code reviews on your changes, just assign me and I'll review. That would also help me gain more knowledge regarding the package. There is always time for a code review or two during my day. 🙂
@jstoone didn't do any on it last night as I didn't want to cross over with anything you were doing (and only just seen your message now this morning). So if you want to carry on with the test stuff this weekend that will really help.
Thanks for offer of help re: code reviews. Will take you up on that!
@lindyhopchris alright! Awesome to finally get the PR pushed out and merged! 🎉
Is there anything that you would find handy, that I'd start working on now?
Thanks for your PR. Definitely some help with writing tests for the Eloquent classes. I'm doing some work on this package tonight so will let you know which class you could take once I've had a look over things this evening.
@lindyhopchris you're very welcome, it's great to be able to contribute.
Just throw me a bone when you're ready, and I'll figure out what to do with it once it's there. 😁
@jstoone was thinking if it's possible to test the make:json-api:resource
command, then that might be something you could do considering that you wrote the generator originally?
Just as an update, the latest changes on 1.x-dev
involve removing the hydrator and splitting adapters into resource and relationship adapters. This gives a lot simpler approach while giving much better opportunities to fully support relationships properly.
Here's what an adapter now looks like: https://github.com/cloudcreativity/laravel-json-api/blob/jj-abrams/tests/JsonApi/Posts/Adapter.php
And this one supports has-many polymorphic relationships: https://github.com/cloudcreativity/laravel-json-api/blob/jj-abrams/tests/JsonApi/Tags/Adapter.php#L32-L35
@lindyhopchris This is awesome news. A lot simpler indeed. Then I guess "full support for filtering, paging etc for relationship endpoints" is next on the list. This is the one thing that prevents me from updating my project. I guess there's not an easy workaround I could implement on my own? Anyway, great work, as always.
Then I guess "full support for filtering, paging etc for relationship endpoints" is next on the list.
Yes, plan is to do that next then tag as v1.0.0-alpha.1
. You'll be able to do filtering, paging etc for relationship endpoints in that release.
The only thing is that'll be in January as I'm away for 3 weeks from 26 December and work is too busy between now and Christmas to get it done before then.
@lindyhopchris I know you are more than super busy, and work should always come before hobby, but just to try and get an update on those two first items in the path to 1.0 list (filtering and with on single). Not to rush you, but to know what can be reasonably expected.
@JeanLucEsser no problem with you asking!!
So I've been working on the 1.0 release recently - it's on the develop
branch. The relationship stuff is now refactored and I've got all the different types of relationships supported for reading/writing. I need to add in the filtering/paging of to-many relationships, that's next on my list to do. Then I'll tag as 1.0.0-alpha.1
The with
for single records is actually pretty minor so I'll aim to get that into the same release.
I'm thinking that'll probably be tagged in the next couple of weeks. I'm using the develop
branch for some work stuff and will be forced to do a tagged version soon for a product release in work. So I'm confident there will be a tag in the next couple of weeks because work deadlines will force it to happen!
@lindyhopchris thanx for this update! I think the filtering/paging of the many relationships is not that trivial. Even in the spec I can't find a clear answer as to how it is supposed to work, when including a to-many relation, do you use the global page[size] to determine the number of relations you bring before asking for more on the relationships endpoint? I'm curious as to which approach you decided on, but I guess I'll soon see.
Good luck on your deadlines! Best
@JeanLucEsser
actually I interpreted this as straightforward. If your relationship needs to be paginated, it should never be serialized as resource identifiers in the primary resource. You should use a link for the relationship and then paginate when using that link.
For example, lets say there's a posts
resource with a comments
relationship. Realistically you're going to need to paginate the comments because there could be hundreds of them for one post.
Your post resource would look like this:
{
"type": "posts",
"id": "1",
"attributes": {
"title": "My First Post"
},
"relationships": {
"comments": {
"links": {
"related": "/api/posts/1/comments"
}
}
}
}
Then to get the first page of comments for that post you would do:
GET /api/posts/1/comments?page[number]=1&page[size]=20 HTTP/1.1
Accept: application/vnd.api+json
The version 1.0 branch will handle that relationship pagination automatically. It also handles filtering on the relationship so you could also do this:
GET /api/posts/1/comments?filter[author]=123 HTTP/1.1
Accept: application/vnd.api+json
To get all comments created by the author with id 123.
@lindyhopchris, This sounds like a really elegant and clean way of dealing with the pagination and filtering of the subresources! I have been thinking about it and only got the idea of something like page[comments][size]&page[comments][number] on the main resource endpoint, which is quite cumbersome for use and even more so for implementation. Using the sub-resource endpoint to begin with, is genial in its simplicity. I am wondering however, how would one specify in the Schema that the respective sub-resource identifiers should not be serialized. Would there be some standard callbacks in the library to provide for the 'showData' and 'showRelated' attributes of a linkage? Wouldn't it require a potentially expensive count of the sub-resources to know when they are too many to serialize as identifiers or would it be a default setting to hide data and show links for all to-many relationships? One more thing - do you plan on adding support as well for the dot syntax for filtering of a resource based on sub-resources, i.e.
GET /api/posts?filter[comments.author]=123 HTTP/1.1
Accept: application/vnd.api+json
(that should return all posts where the author with ID=123 has commented)?
So I'd never recommend variably including the resource identifiers... because otherwise the client doesn't know what it's getting. So in the posts
comments
relationship example, comments
would always be retrieved from the related endpoint and never have comment identifiers in the posts
relationship, regardless of how many comments there actually were. The client then always knows it needs to get them from that endpoint.
In terms of only providing the links and not data, you can already do that in your schema by not returning anything for data. You can see an example of that here: https://github.com/cloudcreativity/laravel-json-api/blob/develop/tests/dummy/app/JsonApi/Posts/Schema.php#L40-L42
The filtering logic is totally up to the application as this package just provides the hook for it and then each application can implement its own filtering logic. For the example you give, this would already be possible in the posts
adapter filter
method as follows:
if ($author = $filters->get('comments.author')) {
$builder->whereHas('comments', function ($query) use ($author) {
$query->where('author_id', $author);
});
}
Have moved the caching of API config to the 1.1.0
milestone as it is not blocking the path to 1.0
.
Good idea! Anything that can speed up official release of 1.0
!
I'd even argue that full documentation could be a work in progress not tied to the release. Let's not forget that you maintain a full demo application, that's the best doc we can get.
@JeanLucEsser good point, I'll take that off for now as you're right that it doesn't stop me from tagging 1.0.0
.
Just as an update for all on where I'm at with this...
alpha.1
is now tagged.alpha.2
will make Authorization more Laravel friendly.alpha.3
will deal with some of the remaining small issues.beta.1
will remove all classes currently marked as @deprecated
beta.2
will update the validation to sort out the remaining issues with it. As it's the oldest bit of the code, I'm going to write this from scratch but behind different interfaces. I'll deprecate the existing interfaces but they'll continue to work during the 1.x
series (although they will not be supported or documented).beta.3
deal with any remaining issues, hopefully only small changes at this point.1.0.0
will remove anything marked as deprecated during the beta except the old validation.1.0.0
will be Laravel 5.4-5.6 so will continue to support PHP 5.6. Although I'd love to drop it, we've got a production app that we just cannot update to PHP 7 until late this year.
I'll time 2.0.0
for the Laravel 5.7 release, upgrading to min PHP 7.1 and that'll also mean I can update the underlying neomerx/json-api
package to 2.0 at the same time.
I'm really curious about what alpha 2 will bring to the table. I'll take a look at Laravel Authorization and how it is currently implemented. All in all, that's a great roadmap. Thanx ^^
@JeanLucEsser the Authorization thing is mainly:
cloudcreativity/json-api
packages I can type-hint Laravel classes so that'll be an improvement.v1.0.0-alpha.2
released. New authorizer implementation documented here:
https://github.com/cloudcreativity/laravel-json-api/blob/master/docs/basics/security.md
I've also added it to the demo app.
Awsome. Just to be sure, should we require dev-release/1.0.0-alpha.2
, dev-develop
or dev-master#1.0.0-alpha.2
?
Require the tag:
$ composer require cloudcreativity/laravel-json-api:1.0.0-alpha.2
Closing this in favour of the milestone.
The only remaining issues relate to validation. We're planning to put in a new validation implementation as the current one was one of the first things we wrote and there's a number of improvements we can do now that the validation does not need to be framework-agnostic.
Our plan however is to leave the existing validation implementation for 1.0.0
so that people can upgrade in their own time during the 1.0
cycle. The current implementation will be marked as deprecated and removed for 2.0
.
Now that
neomerx/json-api
is at1.0
we want to get this library to1.0
as soon as possible - to give people confidence that the API is stable.The recent routing refactor and reducing the number of units per resource type (currently on
develop
- will be released as0.8
) puts in a lot of pre-1.0 changes that we wanted to do to simplify the package.Required 1.0 Features
with
when retrieving single records. See #62JsonApiController
so that it has the majority of the workflow that currently exists withinEloquentController
. Extend the Eloquent controller from this new controller.laravel/browser-kit-testing
.Addjson-api:cache
andjson-api:clear
commands to cache theApiInterface
object. This will give a performance benefit to HTTP requests along the lines ofroute:cache
.Required Internal Changes
Resource
class as it is a reserved word in PHP 7.0StandardObjectInterface
and related generic classes to a separate utility library.ValidatorErrorFactoryInterface
in thecloudcreativity/json-api
package rather than extending that interface inlaravel-json-api
.HttpServiceInterface
and instead inject theApiInterface
andRequestInterface
directly. This is possible as long as no services are created post the JSON-API middleware running.Opening this issue so that people can comment on any features that they think are needed for 1.0.