This package does not adhere fully to JSON:API 🥲

[Helveg]

This package does not adhere fully to JSON:API 🥲

[Helveg]

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

[Helveg]

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.

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

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]

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.

[vsamofal]

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

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]

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

[vsamofal]

@Helveg looks like a good idea

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

[vsamofal]

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

[ppetzold]

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 🚀

[Helveg]

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

[vsamofal]

@ppetzold I'm ok as well

[orecus]

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