JSON:API Layer Refactor about framework

[tobyzerner]

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:

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.

Fair enough - a stable 1.0 version isn't far off though.

Once we pass control of the /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.

I would suggest adding /api/* as a catch-all route after custom API routes are added in your router. This way your custom routes will match first, otherwise the request will fall through to the catch-all json-api-server handler if not. I don't think it's necessary to selectively register routes for each of your resources to invoke the library as you've suggested. Also, it is possible to define your own custom Endpoint classes in json-api-server to create bespoke routes for resources. See here.

Flarum's flow of saving is:
authorization -> fill data -> saving hook(event) -> validation -> save -> post-save-hooks(events).
The library's is:
authorization -> validation -> fill data -> saving hook -> save -> post-save-hooks.
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.

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.

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.

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

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.

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

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

I would suggest changing the frontend to make two requests here rather than trying to wrangle the API layer into this non-standard behaviour.

Eager loading & the serialization process

In my projects, I manually eager-load internally required relationships in the scope method ($query->with('relation')). If you do this, the relation will be available during visibility checking and won't need to be loaded again during serialisation. I also enable Eloquent's strict mode to catch any n+1 queries that slip through.

Default included relationships

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.

Custom Endpoints

I think I've addressed this above.

Custom request params extraction

You could do this by subclassing the Index endpoint. You could also probably subclass OffsetPagination to achieve the same thing.

Endpoint Visibility

Again, could be achieved with a subclass, though I would argue that passing helpers functions into the visible method is just as good.

Happy to discuss anything in more detail - let me know what you think.

[tobyzerner]

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.

Unless I'm missing something, this could easily exist as a class in Flarum (implementing json-api-server's Endpoint interface) rather than requiring a fork? I do think a separate class for each custom endpoint – in the same way that Controllers are classes in MVC – is a better architectural pattern (reusability, testability, etc). And there shouldn't need to be too many of them if you're building a RESTful API.

I can see the need for a beforeSerialization hook, and happy to implement this in some form. Can you please make an issue for it?

[SychO9]

Hi Toby 👋🏼, thanks for taking the time to look over this.

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.

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.

I would suggest adding /api/* as a catch-all route after custom API routes are added in your router. This way your custom routes will match first, otherwise the request will fall through to the catch-all json-api-server handler if not. I don't think it's necessary to selectively register routes for each of your resources to invoke the library as you've suggested. Also, it is possible to define your own custom Endpoint classes in json-api-server to create bespoke routes for resources. See here.

This part turned out not to require changes to the package itself, Flarum now has its own child JsonApi class that can support this behavior, so this does not require forking. Though I like the idea as right now to add to router requires early resolution of resource endpoints which is likely to cause issues for extension devs. We would unfortunately lose the nice ability to link to the api routes by name but that may not be such a loss.

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.

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.

There is some discussion about this tobyzerner/json-api-server#81 (comment). 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 EloquentResource.

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 assertDataValid method for overriding or similarly to other spots, check if the resource provides a validation method/validator class (or smth of the sort) then we can have the added laravel rules trait in Flarum itself.

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:

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

I would suggest changing the frontend to make two requests here rather than trying to wrangle the API layer into this non-standard behaviour.

That would much better, need to look at this again.

In my projects, I manually eager-load internally required relationships in the scope method ($query->with('relation')). If you do this, the relation will be available during visibility checking and won't need to be loaded again during serialisation. I also enable Eloquent's strict mode to catch any n+1 queries that slip through.

It gets a little more complicated with extensibility. scope is only called on listing and on including (eager loading is also very relevant on other endpoints as we've found). We could allow extending the scope method like we do with fields and endpoints, but it would be a downgrade from https://github.com/flarum/json-api-server/blob/main/src/Endpoint/Concerns/HasEagerLoading.php

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 inverse relationship setting, so when an included relation B is serialized for model A and the relation field defines what the inverse relation is, the EloquentBuffer auto sets it back on the loaded relation. I will try to contribute this in the next few days.

---

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 beforeSerialization hook and the Endpoint changes to support that along with custom inline endpoint actions. But also resolving endpoints, fields, sorts and filters then caching them, through additional resolveEndpoints ..etc so that there is a layer to allow adding to resources.

I'll send some PRs to allow simple custom behavior be used (like for the assertDataValid method). But for the endpoint beforeSerialization i'll leave it up to what you want to see done how you want it.

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.