ppetzold/nestjs-paginate

This package does not adhere fully to JSON:API ๐Ÿฅฒ

Opened this issue ยท 16 comments

Helveg commented

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

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

Sorting

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

Filtering

  • Change filter.field to filter[field]

Searching

  • Change search to @search[query]
  • Change searchBy to @search[fields]
  • 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 commented

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

@Helveg looks like a good idea

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

Helveg commented

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.

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

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 ๐Ÿš€

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.

Helveg commented

Sounds good! Is the PR/release workflow documented somewhere? I'll get started on this ๐Ÿ˜Š

@ppetzold I'm ok as well

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

Helveg commented

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

Helveg commented

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

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

Yes, I'm working on it on the json-api branch, and have an open PR #883 that addresses some of the points here already.

@Helveg @ppetzold Just wanted to check if there is any movement on this and getting this released?

I currently dont have that much open source volunteer time, and am more focused on triaging existing issues. My schedule should open up more in a month or 2