-
-
Notifications
You must be signed in to change notification settings - Fork 830
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
JSON:API Layer Refactor #3964
Comments
Hi @SychO9, Thanks for choosing json-api-server to power Flarum's new JSON:API layer! It's a great feeling to still be able to contribute to Flarum indirectly. Thank you also for the PRs you've sent to the json-api-server repo - I plan to look at these soon, and I don't foresee any issues getting them merged. Based on the detail you've included in this issue (and without having looked at your actual fork), I would like to propose that a fork is not necessary, and that all the problems you've mentioned can be solved without one. Of course you are free to fork, but I would argue it's better for both projects if you don't - more improvements go back into json-api-server, and reduces Flarum's maintenance and documentation burden. Here are some thoughts on everything you've mentioned:
Fair enough - a stable 1.0 version isn't far off though.
I would suggest adding
I agree that it's probably worth breaking BC for this change. However, one of the things I'd like to improve in json-api-server is the ability to have more control over the flow in the Create/Update endpoints, so this is something that could be looked at.
There is some discussion about this here. I would suggest using built-in (schema) validation where possible rather than Laravel's validation because then you get a more accurate/useful OpenAPI spec (automatic generation planned for the future). Definitely open to adding more built-in validation functionality to reflect what's supported in the spec. For edge cases where you do need to do some more complex validation, you can do this manually in one of the resource hooks (first solution in the issue linked previously). You could even build this functionality into a subclass of
Replacing the command bus with the API itself sounds like a sensible decision. The custom API you've suggested is fine, but this does not require a fork - you could easily build this as a wrapper class which translates into this under the hood: $request = (new ServerRequest('POST', '/api/groups'))->withParsedBody(['data' => ['attributes' => []]]);
$api->handle($request);
I would suggest changing the frontend to make two requests here rather than trying to wrangle the API layer into this non-standard behaviour.
In my projects, I manually eager-load internally required relationships in the
I'll be happy to merge the PR you've made for this as the JSON:API spec does allow default includes. However I agree that Flarum should probably move away from them.
I think I've addressed this above.
You could do this by subclassing the Index endpoint. You could also probably subclass OffsetPagination to achieve the same thing.
Again, could be achieved with a subclass, though I would argue that passing helpers functions into the Happy to discuss anything in more detail - let me know what you think. |
Hi Toby 👋🏼, thanks for taking the time to look over this.
That would be the best path forward. I've sent PRs for some changes that seemed would be easily desirable. The rest of the more Flarum-specific wanted behavior I wasn't certain would make sense in the original package, or at least I thought you'd probably want to implement those types of behaviors in your own way (was planning to create issues though, haven't gotten around to that yet). I think what will happen is we will probably keep the fork until the original package supports the behavior needed/is able to be more extended for some of the behavior needed to be added on Flarum's side (the fork adds a few hooks to allow that). That way there is also no immediate pressure on the original package itself.
This part turned out not to require changes to the package itself, Flarum now has its own child
This is also no longer an issue 👍🏼, the flow in the fork is the same. Extension devs will have to deal with the breaking change of the Saving event no longer dispatching before validation. It is the more correct behavior with the new changes.
Yes, while changing the fork to base all validation off of laravel rules I realized the OpenAPI spec would be in a way sacrificed, but in the context of our refactor, trying to just preserve current behavior above new additions it was ok (as that was already very difficult 😅). I have not looked much into the OpenAPI stuff, but it could probably also be generated from laravel rules 🤔 However, the Laravel validation behavior also isn't required on the package level, if the package can just open up the
Yes! though its nice to not have to apply unnecessary serialization in this case. Though that separation of endpoint result vs endpoint response was also made for the sake of the changes to support a custom endpoint in the following manner: Endpoint\Endpoint::make('deleteAvatar')
->route('DELETE', '/{id}/avatar')
->action(function (Context $context) {
// ... logic
return $context->model; // auto serialized
})
->response(function (Context $context, User $result) {
// optional, if not provided, result from action will be auto serialized.
return new Response(204);
}), This is very opinionated though, but easier than to create a class for each custom endpoint. Especially since we often have command handlers to dispatch from inside action anyway. Curious about your opinion on this (though i'm still planning to submit issues on the more opinionated changes, not all was documented here). This is probably the change that requires us a fork the most. https://github.com/flarum/json-api-server/blob/main/src/Endpoint/Endpoint.php
That would much better, need to look at this again.
It gets a little more complicated with extensibility. But that's also not detrimental, since we can have the trait inside Flarum as well, the only thing we really require of the package here is a hook before serialization: https://github.com/flarum/json-api-server/blob/main/src/Endpoint/Endpoint.php#L120-L122 which ties back to the changes we made for the Endpoint. We've also added To conclude, I believe most custom behavior can be applied from outside the package, with some changes to make the packages open to that. And perhaps the single blockers are the I'll send some PRs to allow simple custom behavior be used (like for the assertDataValid method). But for the endpoint We could override every single endpoint (that's what the first iterations did), but would be nice to avoid. Thanks again! whatever changes are accepted/made to allow removing the fork, I will test again as it happens and see what blockers might exist every time until we no longer need a fork. |
Unless I'm missing something, this could easily exist as a class in Flarum (implementing json-api-server's I can see the need for a |
We currently use tobyzerner/json-api-php for our JSON:API layer implementation. Our implementation which is based on this package requires quite a lot of boilerplate to read and write resources. The primary goal is to reduce the boilerplate needed.
Additionally the package has been abandoned for a long time, though that should not actually matter, it still does work and it is very likely that regardless of the new chosen library, we will need to have a fork of our own where we can implement our own custom changes and unique use cases unsupported by said library (and sync back to the original what seems valid).
Crucial aspects needed in the revamp:
Current API
Using the current implementation, a developer has to create the following boilerplate for full CRUD of a resource.
*The domain commands are optional, but still used a lot for re-usability, useful to at least keep in mind. as we do need to maintain that re-usability from within.
The implementations of most CRUD controllers are almost always identical. They follow the same usual steps. For example, the creation of a resource usually goes as:
Or listing:
Alternative JSON:API packages
Most of the implementations of the json:api spec (https://jsonapi.org/implementations/#server-libraries-php) have not been updated in some years. Of the rest, only a few are framework agnostic, where the rest are specific to Laravel or Symfony frameworks. The Laravel ones are tightly coupled to Laravel so they expect Laravel's
Request
andRouting
so those are not an option.I believe the most appropriate package for this to be https://github.com/tobyzerner/json-api-server. From its documentation, the package simplifies the boilerplate necessary tremendously, its API is very readable and well put (and the code behind it is simple).
If we create an abstraction layer on top, we can have this integrated into Flarum to follow our internal needs and deal with changes from the underlying package without breaking our own API.
That would also enable us to enforce using our own filtering and searching systems, and preserve the same similar behavior of event dispatching like outlined in the previous section.
We will not escape having to fork and adapt any package of choice however as they are bound to have certain behavior we need changed. For example:
required_with
will not work./api
endpoint we would no longer be able to create simple controller endpoints in/api
such as/api/notifications/readAll
. So we need to change how routes lead to each resource endpoints.Propositions
There are two paths forward here:
json-api
package and adopt it underflarum
to continue maintaining it ourselves. We would then built the abstraction layer we require on top of it, or refactor it as needed, this would be a lot more work of course.The decision between the two will come down to some experimentation, but I feel it likely that the second option is the best.
--> going with option 2
Changes needed
BC Layer
Instead of directly using classes from the library, which is currently in beta and will have breaking changes. We need a layer on top to facilitate updating or switching to a different library. So for example, instead of directly extending
\Tobyz\JsonApiServer\Resource\AbstractResource
we create our ownAbstractResource
to extend from.Routing
Instead of giving control of the entire
/api
endpoint over to the library, and therefore only resources. We need to selectively create the endpoint routes of each resource into our router (/api/resource
). And invoke the library on each individual resource route. This maintains the ability to create/api
routes other than resources.Saving Flow
Flarum's flow of saving is:
The library's is:
To maintain BC for the various model
Saving
events, we need it dispatched before validation, though we will very likely just break BC for this one.Validation
The way validation works in the library is that each field is given its value from the request, and the field applies whatever validation, in isolation from other request data. This will not work for us as we need to be able to validate for example, that a field is required when another was not provided.
The validation relies on manual callbacks to be provided with the ability of passing Laravel rules using a helper:
Since we entirely rely on
illuminate\validation
we need to change things to allow directly passing illuminate rules, then gather all of that, and in the validation process use a single validator with all the data.Which would also allow us to more easily create helper validation methods:
Command Bus (domain commands, not console)
Flarum was built with the concept of command bus (not the console) where a domain operation takes place without any strong coupling to the controller. This makes calling these operations in different other contexts easily possible:
We need to maintain the ability to re-use the endpoint logic. The endpoint itself is pretty well isolated. It only needs a context object with a request.
With custom changes, we will have something along the lines of:
Relationship linkage vs Inclusion
Flarum has the unusual case of the show discussion endpoint, which adds the linkage of all the discussion's posts, but only includes a subset (This behavior is the basis for the post stream scrubber feature on the frontend).
The library does not separate between the values used for linkage and inclusion. So at the moment we have to override its
Serializer
class to allow this distinction. Though preferably we need to look into a different solution for the post scrubber feature that doesn't require this behavior. I avoided that here to not go out of scope.Eager loading & the serialization process
Problem 1
The library handles eager loading of included relationships by deferring getting the value of each relation and using a buffer of model relations to eager load.
Technically, eager loading does not just apply to included relationships however, all it takes is for a field visibility callback or getter callback to access a relation for that relation to require eager loading, so we need a manual API to select which relations to eager load on X endpoint.
We can't just port the exact logic we already use in our current implementation however:
We need a hybrid process of:
This is still not perfect however.
Problem 2
The serialization flow of the library is:
addToPrimary
: Fields are added to a map after a visibility check and resolved immediately by default (unless a callback is used for the getter).serialize
: Deferred fields are resolved and added to the map, then the rest of the serialization process happens.Problem: N+1 Queries are easily produced in this flow, especially coming from the visibility checker, and regardless of the eager loading API we're adding from Problem 1.
Example: Resource has one attribute A and
ToOne
relationship B, attribute A is only visible when B->col is true.->visible(fn (object $a) => $a->B->col)
Even though the library does its own eager loading of included relationship B, the visibility callback is accessed earlier than that.Solution 1: Eager load relation B twice. The first one for internal use (visibility checking), the second one for inclusion in the response (scoped).
Solution 2: Change the serialization flow. Defer all the fields, then prioritize resolving the relationship fields first. By the time visibility checking and getters of the attributes is accessed, relationship B will have been loaded. However if internally a relation is accessed which is not meant to be visible publicly, it'll leads to some N+1 queries.
--> Going for solution 1 as it's the least complicated and ensures internally accessed relations are always eager loaded while response included relations are always scoped. While this means some duplicate queries, it is still an improvement over the n+1 issue.
Additions
The excellent implementation of the library allows us to add some Flarum specific features to it (in the goal of reducing boilerplate). This includes both features already existent in the current API implementation (like default includes) or new features.
Default included relationships
Unfortunately the current library has no way of specifying relationships that are by default included in the response document. This needs to be added as it is a thing in 1.x, we can do so by adding a
defaultInclude
method on endpoints:However, I would prefer we move away from including relations by default, and instead, specifying on the frontend side on each request, what relationships to include. That way endpoints that require more data than others, don't affect the response size elsewhere. And extensions have taken the habit of just always using default includes which contributes to bad performance.
Custom Endpoints
This is a necessary addition. We have routes such as
DeleteAvatarController
which return a serialized user model, so it would better to have the ability to add custom endpoints within the resources, rather than try to re-use the JsonApi inside each similar custom route.Custom request params extraction
Some of our endpoints -like the list posts endpoint- have cusom logic for extracting some of the request parameters -like the offset-. We need to support custom callbacks on at least the Index endpoint (potentially the Show endpoint as well) to allow customizing the logic for the extraction per resource.
Endpoint Visibility
The library allows specifying when an endpoint is visible:
We can add
authenticated
andcan
methods to simplify this:The text was updated successfully, but these errors were encountered: