Support per-type page size limits/defaults

[ethanresnick]

Starting to tackle #132. May be rebased.

[codecov[bot]]

Codecov Report

Merging #170 into master will increase coverage by 0.01%. The diff coverage is 92.3%.

@@            Coverage Diff            @@
##           master    #170      +/-   ##
=========================================
+ Coverage   90.89%   90.9%   +0.01%     
=========================================
  Files          56      57       +1     
  Lines        2208    2255      +47     
  Branches      497     504       +7     
=========================================
+ Hits         2007    2050      +43     
- Misses        201     205       +4

Impacted Files	Coverage Δ
src/db-adapters/Mongoose/MongooseAdapter.ts	94.67% <ø> (+0.06%)	:arrow_up:
src/ResourceTypeRegistry.ts	97.72% <100%> (-0.75%)	:arrow_down:
src/controllers/API.ts	94.85% <100%> (+0.49%)	:arrow_up:
src/steps/pre-query/validate-resource-data.ts	92.85% <100%> (ø)	:arrow_up:
src/types/Query/WithCriteriaQuery.ts	79.62% <100%> (+1.62%)	:arrow_up:
src/steps/make-query/make-get.ts	89.28% <100%> (+0.82%)	:arrow_up:
src/types/Query/FindQuery.ts	56.75% <71.42%> (+6.75%)	:arrow_up:
src/controllers/Documentation.ts	96.15% <80%> (-1.18%)	:arrow_down:
src/steps/run-query.ts	97.29% <97.29%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 512fa8c...480ca2f. Read the comment docs.

[ethanresnick]

@carlbennettnz I want to get your input on this before merging, as the design in this PR requires adapters to do a bit of new work (namely, add the setRegistryDerivedOptions method and validate the limit in find). I think this is the best approach — esp. considering that, as mentioned in the commit, the find validation can be factored out into a shared function long-term — but I wanted to check that you didn't find it too onerous or wrong architecturally. I'm happy to be building in this feature, though, as pretty much every API seems to need it.

[carlbennettnz]

This looks great! The user-facing API looks good to me. Being able to set the max/default per resource type is really nice.

I'm not entirely sure that the adapter needs to know about these limits though. Would this logic and error handling be more at home in doGET and FindQuery? How many records to return is surely a higher-level concern than what the adapters deal with.

[ethanresnick]

Would this logic and error handling be more at home in doGET and FindQuery?... How many records to return is surely a higher-level concern than what the adapters deal with.

I tried to get at this in my commit message. Basically, my thought was that putting the logic in makeGET would mean that, if the user creates a custom query (which doesn't go through makeGET), they could forget to apply the pagination limits when constructing that query. One ressponse to that is to say "who cares", but I kind of like the safety that would come from only having to write the check once, rather than for every custom query.

That said, I agree that the adapter isn't conceptually the right place for this logic. Maybe, instead, the right approach is a runQuery function, defined on the API controller and also passed to query/result factories, that: takes a query, performs adapter-agnostic validations on it (including the limit size check), looks up the appropriate adapter from the registry given the query's type, and returns adapter.doQuery(query).then(query.returning, query.catch)? That seems like it might be best, and then we'd just note in the docs that adapter.doQuery() is really an internal thing that users should never call directly (they should use runQuery instead).

[carlbennettnz]

Custom queries are really powerful. Obviously this is also a good thing, but having this level of exposure of internals means it's tricky to decide which likely pitfalls we should be shielding custom query users from.

This is actually something I've been thinking about a bit recently. With the latest API I've been working on, I've been finding that writing custom queries often requires a very good understand of this library and how it works. The final code I end up with is usually fairly straightforward, but it takes a long time to figure out what that code should be. I've been following this library's changes quite closely, so I think its fair to say that most users will struggle with this more than I have been.

I guess this should be in an issue of its own, but I've been wondering if this situation could be helped by adding a simpler, more constrained API intended to cover 95% of use cases, keeping custom queries around for if you really need to do something weird. This would then free up custom queries to do anything, and these limits can be comfortably ignored, as long as the need to reimplement them is documented.

Back to the main issue at hand though, I like the runQuery idea. Given this logic doesn't fit into either makeGET or doQuery, adding a layer between them seems like a sensible solution. The adapter interface could remain unchanged and custom queries would have limits applied by default, but have the ability to override if necessary.

I can make the change if you like. I've got a bit of time over the next few days.

[ethanresnick]

With the latest API I've been working on, I've been finding that writing custom queries often requires a very good understand of this library and how it works. The final code I end up with is usually fairly straightforward, but it takes a long time to figure out what that code should be.

I agree with this diagnosis.

I guess this should be in an issue of its own, but I've been wondering if this situation could be helped by adding a simpler, more constrained API intended to cover 95% of use cases, keeping custom queries around for if you really need to do something weird.

I would be open to this, but I'm not sure really what it would look like. If you have specific ideas, please share.

I can make the change if you like. I've got a bit of time over the next few days.

Thanks, that would be awesome. With that change, maybe we should also kill Adapter#doQuery, and instead do the query type => adapter method mapping in runQuery?

[carlbennettnz]

With that change, maybe we should also kill Adapter#doQuery, and instead do the query type => adapter method mapping in runQuery?

Sounds good to me!

[ethanresnick]

Cool beans. When you make the changes, please branch off of c340ee6; I killed the commit that previously came after it (releasing rc4) and will recreate that once the final solution's ready to go.

[carlbennettnz]

Change is pushed to carlbennettnz/json-api#page-limits. Just holding off on a PR for now until we decide what to do regarding the type checking issue.

[ethanresnick]

Awesome! See comment above on type checking :)

[ethanresnick]

Also, to get registry.type(name)! type checking properly, let's leave the type for ResourceTypeDescription as is, because the adapter might still be missing in the provided type description if it's given as a default instead. We just need to update OutputResourceTypeDescription to be something like:

export type OutputResourceTypeDescription =
  ResourceTypeDescription 
  & { urlTemplates: UrlTemplates }
  & { dbAdapter: Exclude<ResourceTypeDescription["dbAdapter"], undefined> }

and then have doGet reference OutputResourceTypeDescription in its return type.

[ethanresnick]

@carlbennettnz Ok, I've rebased + pushed a revised version of this, incorporating the changes from your branch. (Thanks for working that up, btw!)

Note: the semantics in the code I've pushed are slightly different from yours in that, for custom queries (i.e., those not generated by makeGET), the default page size isn't applied as the limit. I made that change because I'm pretty sure that, in your branch, it was impossible to create a custom query that was intentionally unlimited (assuming a default page size was set for the query's query.type).

That said, the semantics I came up with for custom queries—i.e., applying the max limit, assuming ignoreMaxLimit isn't set, but not the default page size—do feel a bit inconsistent. Idk if there's anything to be done about that.... I guess we could apply both max limit and default page size by default, but then add a symmetric ignoreDefaultLimit flag too. I'm not sure how I feel about that, but I'm open to ideas...

Please review when you have a chance.

[ethanresnick]

Ok, I'm gonna go ahead and merge this, as I need the feature in a project and want to finalize v3, and the potential inconsistency doesn't bother me that much.