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

The path to 1.0 #60

Closed
15 tasks done
lindyhopchris opened this issue May 18, 2017 · 30 comments
Closed
15 tasks done

The path to 1.0 #60

lindyhopchris opened this issue May 18, 2017 · 30 comments
Milestone

Comments

@lindyhopchris
Copy link
Member

lindyhopchris commented May 18, 2017

Now that neomerx/json-api is at 1.0 we want to get this library to 1.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 as 0.8) puts in a lot of pre-1.0 changes that we wanted to do to simplify the package.

Required 1.0 Features

Required Internal Changes

  • Rename the Resource class as it is a reserved word in PHP 7.0
  • Extract StandardObjectInterface and related generic classes to a separate utility library.
  • Single ValidatorErrorFactoryInterface in the cloudcreativity/json-api package rather than extending that interface in laravel-json-api.
  • Ideally stop injecting HttpServiceInterface and instead inject the ApiInterface and RequestInterface 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.

@lindyhopchris
Copy link
Member Author

v0.9 released with:

  • Above internal changes
  • Blade template helpers
  • Broadcasting trait

@jstoone
Copy link
Contributor

jstoone commented Jun 7, 2017

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. 😄

@lindyhopchris
Copy link
Member Author

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.)

@lindyhopchris
Copy link
Member Author

@jstoone I've restructured the tests on develop. I'm heading off now so will be back online tomorrow morning (UK time)

@jstoone
Copy link
Contributor

jstoone commented Jun 7, 2017

@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! 🔥

@lindyhopchris
Copy link
Member Author

@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?

@jstoone
Copy link
Contributor

jstoone commented Jun 16, 2017

@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. 🙂

@lindyhopchris
Copy link
Member Author

@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 lindyhopchris added this to the 1.0.0 milestone Jun 16, 2017
@jstoone
Copy link
Contributor

jstoone commented Jun 23, 2017

@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?

@lindyhopchris
Copy link
Member Author

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.

@jstoone
Copy link
Contributor

jstoone commented Jun 23, 2017

@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. 😁

@lindyhopchris
Copy link
Member Author

@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?

@lindyhopchris
Copy link
Member Author

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

@JeanLucEsser
Copy link
Contributor

@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.

@lindyhopchris
Copy link
Member Author

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.

@JeanLucEsser
Copy link
Contributor

@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.

@lindyhopchris
Copy link
Member Author

@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!

@JeanLucEsser
Copy link
Contributor

JeanLucEsser commented Feb 27, 2018

@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

@lindyhopchris
Copy link
Member Author

@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.

@dimitvassilev
Copy link

@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)?

@lindyhopchris
Copy link
Member Author

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);
    });
}

@lindyhopchris
Copy link
Member Author

Have moved the caching of API config to the 1.1.0 milestone as it is not blocking the path to 1.0.

@JeanLucEsser
Copy link
Contributor

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.

@lindyhopchris
Copy link
Member Author

lindyhopchris commented May 3, 2018

@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.

@JeanLucEsser
Copy link
Contributor

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 ^^

@lindyhopchris
Copy link
Member Author

@JeanLucEsser the Authorization thing is mainly:

  1. Change the interfaces to take the Laravel request object rather than the JSON API objects that it currently does... this is because authorization typically takes place before validation, so those JSON API objects shouldn't exist at that point. As I'm no longer using the generic cloudcreativity/json-api packages I can type-hint Laravel classes so that'll be an improvement.
  2. At the moment the package assumes one authorizer per resource type. but actually in a lot of cases you're going to have an authorizer that could be used against multiple resource types. So I want to support reusable Authorizers not just resource-specific ones (i.e. it will support both).
  3. Add an Authorizer generator (that can generate a generic or a specific one).
  4. Add tests as none of the Authorizer stuff is tested in the current code base.
  5. Add to the demo app as it's not demonstrated at the moment.

@lindyhopchris
Copy link
Member Author

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.

@JeanLucEsser
Copy link
Contributor

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?

@lindyhopchris
Copy link
Member Author

Require the tag:

$ composer require cloudcreativity/laravel-json-api:1.0.0-alpha.2

@lindyhopchris
Copy link
Member Author

lindyhopchris commented Aug 22, 2018

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.

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

No branches or pull requests

4 participants