framework icon indicating copy to clipboard operation
framework copied to clipboard

refactor: JSON:API

Open SychO9 opened this issue 11 months ago • 5 comments

Closes #3964

Changes proposed in this pull request: This is a difficult review, I couldn't really find a way to split this since it can only be all done together, I would recommend looking at some specific resources, for example, comparing the main branch Discussion model JSON API with this one. The controllers and serializer class and command handlers vs the DiscussionResource class.

Reading the issue itself will help you a lot in getting context about the changes here. Other than that, some key things to think about:

  • Do we want to add out fork of the library into the monorepo? I'm leaning towards yes.
  • Any specific concerns about the new API? everything that an extension dev could do before should be able to do now as well, only differently. Once we start updating FoF extensions we will be able to see and make any necessary changes.

Some notable changes

  • The validation.yml file has been updated.
  • The discussions sortmap is no longer bound to the container, instead the SortColumn on the DiscussionResource can receive an ascendingAlias and descendngAlias which can them easily be accessed in the content classes, this also means extensions can add to the sortmap by just the sorts they add themselves in the same manner.

Todo

  • [x] Tag a 1.0 of the forked library: https://github.com/flarum/json-api-server
  • [x] Extensively document the changes from 1.0 to extension devs. (https://github.com/flarum/docs/pull/472)

SychO9 avatar Mar 08 '24 13:03 SychO9

still a couple test failures to fix as well, and phpstan..

SychO9 avatar Mar 08 '24 13:03 SychO9

serializers have been replaced with resources, and resources contain not just endpoint instruction, but also field instructions

Yes, and columns. Filters have been disabled since we have our own driver-based filter implementation (which we can always improve in terms of boilerplate as well).

many of the controllers have been simplified or removed entirely because of this refactor, which is also nice. I do think it will need some getting used to, to easily find where what is declared when looking things up.

Keep in mind, you can still create a separate controller the old fashion way, only there are non of the previous parent serializer classes. So the result of it cannot be the serialization of a resource. You can also just use a custom endpoint (see user resource delete avatar).

Can we somehow get rid of all the Command/Handlers and move them to the respective controllers where possible or would that better be done in a follow up PR?

They are still useful reusable domain logic, for example check the delete avatar custom endpoint in the user resource class. Now that most controllers have been removed, command handlers aren't actually bad to have in terms of boilerplate. An additional improvement to them would be to make them one class rather than requiring two classes imo.

SychO9 avatar Mar 13 '24 09:03 SychO9

documentation added: https://github.com/flarum/docs/pull/472

SychO9 avatar Mar 14 '24 08:03 SychO9

There's two things I'd love to be able to do:

  • allow custom paths that aren't prefixed with /api, eg /.well-known/webfinger
  • allow a callback to be executed before payload is send to the user, so I can flatten the response or filter some values. I considered a middleware, but I'm not sure this feature could be applied for other purposes than mine (flattening the json payload by pushing attributes one level up). And yes this is a hack, but extensibility is about flexibility too.

Overall using this is very nice, it reminds me a lot of the filament resources logic. I have had no issue declaring the resources, their endpoints and their schema. So 👍

luceos avatar Mar 20 '24 08:03 luceos

allow custom paths that aren't prefixed with /api, eg /.well-known/webfinger

That would mean adding a route to the forum rather than the API, it's quite messy and would require quite a lot of changes.

This is an edge case where I would rather you use a normal controller. Does this endpoint require the api resourceat all? if it returns custom json structure that isn't actually of the JSON:API spec for resources, all the more reason to use a normal controller instead.

If it does return valid json for a resource, then checkout the ShowForumController controller and how you can call an api resource's endpoint.

allow a callback to be executed before payload is send to the user, so I can flatten the response or filter some values.

You can use the response method on the endpoint. Though again, if you are returning non json api spec json, what you might be needing to do instead is using custom controllers.

SychO9 avatar Mar 20 '24 15:03 SychO9

Will merge after the DB PRs are merged.

SychO9 avatar May 11 '24 11:05 SychO9

@SychO9 this one could/should be merged asap. Let's work off of this codebase moving forward before any other branches change dramatically.

luceos avatar Jun 16 '24 16:06 luceos