nestjs-paginate icon indicating copy to clipboard operation
nestjs-paginate copied to clipboard

This package does not adhere fully to JSON:API 🥲

Open Helveg opened this issue 1 year ago • 16 comments

Here's a write up of the discrepancies with the spec, and my proposed solutions.

Violations

Pagination

JSON:API does not allow the use of non-family query parameters for implementation semantics, and on top of that we SHOULD use the page query parameter family.

So we have to change to page[number] and page[limit] or similar.

Sorting

Here we don't do much of what we MUST do:

  • MUST use the sort query parameter
  • MAY support multiple sorts by the comma ,, applying the sorts in order
  • MUST be ascending, unless the field is prefixed by the minus -, which then MUST be descending

Filtering

JSON:API is strategy agnostic, but it does specify the use of the filter query parameter family, and that entails that the filter query should be structured like this:

?filter[column]=<this piece is already compliant>

Since JSON:API does not allow the use of non-family query parameters for implementation semantics, we have to change the search query parameter to perhaps filter[@search] (@ is the only allowed symbol at the start of a member name that can avoid collision with field names).

Sparse fieldsets

Currently we use select, but this should be changed to fields[TYPE] query parameter family, eg /articles?fields[articles]=title,body

Include relationships

I didn't see from the docs whether we support any syntax for this? But we could support the "Inclusion of Related Resources" as specified:

/articles?include=comments,author

Proposal

  • We can create backwards-compatible additions to the syntax, and post a deprecation warning when a query contains old syntax.
  • We let users opt-in to JSON:API through a new decorator JsonApiPaginate that constructs a standard PaginateQuery.
  • We add a new overload signature to the existing Paginate decorator: @Paginate(format: 'legacy' | 'json:api') with default value 'legacy' and can swap the default value after the deprecation period.

The package doesn't have a deprecation policy, and deprecating the existing syntax isn't necesarily an end goal here, just a personal preference to promote and transition to the standard.

Pagination

  • [x] Change page to page[number]
  • [x] Change limit to page[size]

Sorting

  • [x] Change sortBy to sort
  • [x] Change current syntax (field:ASC/DESC) to a comma separated ordered list of the sort fields with minus as desc operator (field1,-field2)

Filtering

  • [x] Change filter.field to filter[field]

Searching

  • [x] Change search to @search[query]
  • [x] Change searchBy to @search[fields]
  • [x] Allow @search as a shorthand for @search[query] (without the possibility to then specify fields)

Sparse fieldsets

  • [ ] Change select=field1,field2 to fields[TYPE]=field1,field2
  • [ ] Expand paginate logic to allow field selection for relations as well (if any fields are specified for the sub-TYPE)

We can try to guess the types of relations by using their key in the object and using pluralize to derive the singular form. e.g., post.comments would have TYPEs post and comment. For more advanced cases we can accept a documentTypes: Record<string, string> map in the PaginateConfig that map any appearing relationship keys to their type:

{
  documentTypes: {
    'post.deeper.weirdKeyForComment': 'comment'
  }
}

so that fields[comment] would affect post.deeper.weirdKeyForComment

Include relationships

  • [ ] When a includes query parameter is given, we should only load the intersect of the configured relations and the given includes. Use dot notation for nested relations eg ?includes=post.comments

Helveg avatar Sep 15 '23 15:09 Helveg

Let me know what you think @ppetzold and I can get started on a branch for this.

Helveg avatar Sep 18 '23 08:09 Helveg

@Helveg looks like a good idea

Do you use some tool that is required to comply with this format?

vsamofal avatar Sep 22 '23 22:09 vsamofal

No, but I like that JSON:API provides a full "HTTP-URL-compliant" spec to query REST API's including nested relationships and field selects! And in general I enjoy adhering to standards because it comes with many benefits :)

(Also, we already claim in the README to be JSON:API compliant, so this may even be considered a bugfix! 😄 )

I will get started on a PR. Which backwards compatibility / deprecation / opt-in mechanism should I add? My favorites:

Module configuration

Several idioms/flavors:

  imports: [
    PaginateModule.forRoot({syntax: 'json:api'})
  ]
  imports: [
    JsonApiPaginateModule
  ]
  providers: [
    JsonApiPagination
  ]
  • + Configure in one central location
  • + Backwards compatible
  • + Opt-in

New decorator or decorator param

@Get()
controllerMethod(@JsonApiPaginate() query: PaginateQuery) {}

@Get()
controllerMethod(@Paginate('json:api') query: PaginateQuery) {}
  • + Backwards compatible
  • + Opt-in
  • - Need to adjust all occurences of Paginate

Dual parsing

We just allow both syntaxes by default 🎉

  • + Backwards compatible.
  • + No changes required to user code.
  • - Not opt-in, might mask some dev errors, change edge case behaviours, or overlap with existing application semantics of the user.

Helveg avatar Sep 24 '23 09:09 Helveg

we also have swagger now, it need to be adjusted as well, once you finish I can help with it

vsamofal avatar Sep 24 '23 10:09 vsamofal

I don't really have a strong opinion on it; as I am currently not having this package in any production use-case. But I see your point, it claims compliance - so it better be compliant lol.

Jsonapi spec seems to be quite active and reasonably popular, so I think I would be okay with taking the leap into full compliance with a breaking change release.

Don't think the headache is worthwhile to maintain backwards compatibility. In case of any security patches we can still patch the current major.

Let's go for a new major with full jsonapi compliance 🚀

ppetzold avatar Sep 25 '23 12:09 ppetzold

btw @Helveg @vsamofal both of you had been around for a while and seem to be caring; if you are up for it, I'd like to grant you maintainer status for this repo. would add 2 out of 3 rule to approve PRs and merge to master / release stuff.

ppetzold avatar Sep 25 '23 13:09 ppetzold

Sounds good! Is the PR/release workflow documented somewhere? I'll get started on this 😊

Helveg avatar Sep 25 '23 14:09 Helveg

@ppetzold I'm ok as well

vsamofal avatar Sep 25 '23 15:09 vsamofal

Cool, this repo uses semantic release. so every commit (if prefixed) triggers a release.

for larger major releases, we could add a beta or next branch. but likely overkill for such a small package.

invited both of you to the repo :)

ppetzold avatar Sep 25 '23 15:09 ppetzold

Just one thing, I misinterpreted the query parameter section, and as long as a query parameter contains at least one non a-z character it's a valid implementation query parameter:

A “query parameter family” is the set of all query parameters whose name starts with a “base name”, followed by zero or more instances of empty square brackets (i.e. []) or square-bracketed legal member names. The family is referred to by its base name.

For example, the filter query parameter family includes parameters named: filter, filter[x], filter[], filter[x][], filter[][], filter[x][y], etc. However, filter[_] is not a valid parameter name in the family, because _ is not a valid member name.

Note the zero square brackets, I missed that, so no sub-member is required. And:

Implementation-Specific Query Parameters

Implementations MAY support custom query parameters. However, the names of these query parameters MUST come from a family whose base name is a legal member name and also contains at least one non a-z character (i.e., outside U+0061 to U+007A).

It is RECOMMENDED that a capital letter (e.g. camelCasing) be used to satisfy the above requirement.

This means that filter[@search] might be needlessly complicated, and we can use a camelCased name instead, like searchText, or simply @search, but I can't really find a satisfying, intuitive replacement. So if you prefer anything over filter[@search] let me know.

Some ChatGPT generated camelCased possibilities: "searchQuery", "searchColumns", "searchAll", "queryAll", "fullSearch"

For now I'm going with @search=hello, and I'm including an expanded form with @search[query]=hello&@search[fields]=description,name to succeed the searchBy; I can also do searchQuery and searchBy? I chose @search for similarity to the existing implementation, and the extended form @search[query] and @search[fields] pleased my eyes the most. I chose it over searchQuery/searchBy because then we'll be using strictly single words and square brackets everywhere, and @-members are designated implementation semantics.

Helveg avatar Sep 26 '23 08:09 Helveg

@Helveg just a note, it will make sense to release those fixes one by one, having it all in one will be hard to review properly

vsamofal avatar Sep 26 '23 12:09 vsamofal

Ok, I'll PR them separately but targetting a new feature branch, so that they land on master as 1 breaking change!

Helveg avatar Sep 26 '23 13:09 Helveg

Any update or eta on this work and the complaint status? :)

orecus avatar Mar 28 '24 09:03 orecus

Yes, I'm working on it on the json-api branch, and have an open PR https://github.com/ppetzold/nestjs-paginate/pull/883 that addresses some of the points here already.

Helveg avatar Mar 28 '24 12:03 Helveg