Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST query pagination #3962

Merged
merged 42 commits into from Jan 13, 2022
Merged

REST query pagination #3962

merged 42 commits into from Jan 13, 2022

Conversation

aptkingston
Copy link
Member

Description

This PR adds pagination to REST queries.
It also includes a full rewrite of data fetching inside client apps, and significant performance improvements.

REST query pagination

Pagination has a few different configuration options:

  • the location of pagination params in the request (either in the query string or request body)
  • the name of the pagination params in the request
  • the type of pagination (either page number based or cursor based)
  • the name of the param in the query response denoting the next cursor (for cursor based pagination only)

Pagination is configured in a new tab for REST queries:
image

When pagination is configured, a pagination control will appear as normal for REST query data providers which have pagination enabled. The client experience is identical to pagination for internal tables or datasource plus tables.

When pagination parameters are configured to be sent in the request body, it will use the body type configured in the Body tab. Not all body types are supported - for example it is not possible to add pagination parameters to a raw text body. The supported body types are:

  • form-data
  • x-www-form-encoded
  • json

If you set up a query to be a POST request with a body type other than the specified ones, and then also configure pagination parameters to be send in the request body, then pagination will be ignored and will not be applied. It will also be ignored if you configure a GET request but then specify that pagination should be included in the request body. Just be sensible basically.

Client data fetching rewrite

As part of this work, the entire process of fetching data in client apps needed to be written. Previously pagination was a concept that was entirely scoped to internal tables or datasource plus tables, and was handled in an opionated manner by the search endpoint. This work introduced the requirement of queries needing to support pagination, and for that pagination to be flexible and configurable.

The client library now has a new base class implementation for fetching data from a datasource called DataFetch. This class is extrended and customised to handle the different types of core datasource:

  • Tables via TableFetch
  • Views via ViewFetch
  • Relationships via RelationshipFetch
  • Queries via QueryFetch

There are also implementations for the more client focused "fake" datasources:

  • Nested data providers via NestedProviderFetch
  • Array-like fields (attachments and multi-array fields) via FieldFetch
  • JSON arrays via JSONArrayFetch

These new models do not consider the previous binary concept of "plus" and "non plus" datasource - they only consider what features any given datasource supports. Those features are searching, sorting and pagination. Each implementation defines which features are supported as well as how to fetch the datasources data and schema. If a certain datasource does not support searching or sorting then a client side implementation will be used, transparently ensuring that all sources support these features. Currently there is not a client side implementation for pagination, but this could potentially be added if required.

Eventualy this new implementation of data fetching will become the core implementation used in the builder as well.

Client library optimisations (in depth dev notes for reference)

While implementing the data fetching rewrite it became apparent that reactive statements in client components were triggering more often than they should. I did a very deep dive into svelte internals to work out why this is happening. It turns out that due to how we pass in props to components (we store all props inside an object and spread them in { ...props } which we have to do because everything is generic), any non-primitive props (such as objects) trigger a svelte invalidation whenever any other prop is changed.

Consider the following props:

let props = {
  count: 1,
  foo: {
    bar: "abc"
  }
}
...
<Component {...props} />

and consider the following component:

export let count
export let foo

$: console.log("foo changed", foo)

If the count prop changes, even by direct mutation of the object and not touching foo in any away:

props.count = 2

Then our reative statement inside our component will fire. This is terrible for performance of our components, as any prop change will trigger an invalidation of any other non-primitive props. Unfortunately the solution to this is non-trivial because this is just how svelte works.

I did find a solution to this problem by using the underlying svelte $$set function which exists on component refs. Using this function allows you to directly mutate a certain prop without invalidating other props. There are other considerations with using this approach, such as how to handle the initial render and how to avoid stale props, which have all been addressed. The new implementation for managing client component props can be seen in Component.svelte.

The result of this work is that whenever any props change, only the actual prop that changed is invalidated (as if we were a real app, and not a generic app created at runtime). This means that we immediately benefit from the following:

  • Far fewer duplicate API calls. We only call to the API whenever we actually need new data
  • Better performance as we skip many unnecessary renders
  • Avoidance of flashing loading spinners and a smoother experience in general

Misc fixes

  • Update REST query verbs to have a fixed width, which looks better
    image
  • Fix a crash in KeyValueBuilder that happened when object was explicity set to null

…trol props in order to avoid triggering additional reactive statements and improve performance
…n initial props without triggering svelte invalidation
@joebudi
Copy link
Contributor

joebudi commented Jan 7, 2022

Incredible write-up @aptkingston and awesome work. I look forward to trying the new Rest connector. Perf improvements are starting to become the norm with your PRs!

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2022

Codecov Report

Merging #3962 (0bb8ceb) into develop (4378a3a) will increase coverage by 0.02%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3962      +/-   ##
===========================================
+ Coverage    68.89%   68.91%   +0.02%     
===========================================
  Files          139      139              
  Lines         4630     4633       +3     
  Branches       685      685              
===========================================
+ Hits          3190     3193       +3     
- Misses        1017     1018       +1     
+ Partials       423      422       -1     
Impacted Files Coverage Δ
packages/server/src/threads/utils.js 81.08% <ø> (+6.94%) ⬆️
packages/server/src/api/controllers/query/index.js 76.13% <50.00%> (ø)
packages/server/src/threads/query.js 80.35% <70.37%> (-3.74%) ⬇️
packages/server/src/api/controllers/dev.js 28.57% <100.00%> (ø)
packages/server/src/api/controllers/hosting.js 93.33% <100.00%> (ø)
packages/server/src/api/index.js 93.10% <100.00%> (ø)
packages/server/src/constants/index.js 100.00% <100.00%> (ø)
packages/server/src/middleware/currentapp.js 69.81% <100.00%> (ø)
packages/server/src/threads/automation.js 81.57% <100.00%> (ø)
packages/server/src/utilities/budibaseDir.js 85.71% <100.00%> (ø)
... and 1 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 e48e21f...0bb8ceb. Read the comment docs.

Copy link
Contributor

@Rory-Powell Rory-Powell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Some really nice refactoring in here and great to see the server tests as well

Copy link
Member

@shogunpurple shogunpurple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - great job. The datadfetch abstraction is super clean - should make it so much easier going forward. I pulled it down and had a play around with an app I was trying to build that needed pagination. Works flawlessly 👌

@aptkingston aptkingston merged commit 8921436 into develop Jan 13, 2022
@aptkingston aptkingston deleted the rest-pagination branch January 13, 2022 10:13
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants