From 05666445089187e46bf8f9d32dfd49ddfae9cc16 Mon Sep 17 00:00:00 2001
From: Andrew Kingston <andrew@kingston.dev>
Date: Mon, 21 Aug 2023 11:56:58 +0100
Subject: [PATCH] Allow view searching and sorting. Refactor grid logic to fix
 multiple issuies

---
 packages/client/manifest.json                 |   5 +-
 packages/frontend-core/src/api/viewsV2.js     |   3 +
 .../grid/controls/SortButton.svelte           |  46 ++-----
 .../src/components/grid/stores/pagination.js  |   7 +-
 .../src/components/grid/stores/sort.js        |  27 +++-
 .../src/components/grid/stores/table.js       |  82 ++++++------
 .../src/components/grid/stores/viewV2.js      | 123 +++++++++++-------
 packages/frontend-core/src/fetch/DataFetch.js |  71 +++++-----
 .../frontend-core/src/fetch/QueryFetch.js     |   4 +
 .../frontend-core/src/fetch/ViewV2Fetch.js    |  12 +-
 .../server/src/api/controllers/row/views.ts   |  30 ++++-
 packages/types/src/api/web/app/rows.ts        |   8 +-
 12 files changed, 250 insertions(+), 168 deletions(-)

diff --git a/packages/client/manifest.json b/packages/client/manifest.json
index 795cc17097..c8d0506553 100644
--- a/packages/client/manifest.json
+++ b/packages/client/manifest.json
@@ -4615,14 +4615,15 @@
             "type": "field/sortable",
             "label": "Sort by",
             "key": "sortColumn",
-            "placeholder": "None"
+            "placeholder": "Default"
           },
           {
             "type": "select",
             "label": "Sort order",
             "key": "sortOrder",
             "options": ["Ascending", "Descending"],
-            "defaultValue": "Ascending"
+            "defaultValue": "Ascending",
+            "dependsOn": "sortColumn"
           },
           {
             "type": "select",
diff --git a/packages/frontend-core/src/api/viewsV2.js b/packages/frontend-core/src/api/viewsV2.js
index c765214c3b..38bacac012 100644
--- a/packages/frontend-core/src/api/viewsV2.js
+++ b/packages/frontend-core/src/api/viewsV2.js
@@ -31,6 +31,7 @@ export const buildViewV2Endpoints = API => ({
   /**
    * Fetches all rows in a view
    * @param viewId the id of the view
+   * @param query the search query
    * @param paginate whether to paginate or not
    * @param limit page size
    * @param bookmark pagination cursor
@@ -40,6 +41,7 @@ export const buildViewV2Endpoints = API => ({
    */
   fetch: async ({
     viewId,
+    query,
     paginate,
     limit,
     bookmark,
@@ -50,6 +52,7 @@ export const buildViewV2Endpoints = API => ({
     return await API.post({
       url: `/api/v2/views/${viewId}/search`,
       body: {
+        query,
         paginate,
         limit,
         bookmark,
diff --git a/packages/frontend-core/src/components/grid/controls/SortButton.svelte b/packages/frontend-core/src/components/grid/controls/SortButton.svelte
index bd75249216..60e3c8e514 100644
--- a/packages/frontend-core/src/components/grid/controls/SortButton.svelte
+++ b/packages/frontend-core/src/components/grid/controls/SortButton.svelte
@@ -8,7 +8,6 @@
   let anchor
 
   $: columnOptions = getColumnOptions($stickyColumn, $columns)
-  $: checkValidSortColumn($sort.column, $stickyColumn, $columns)
   $: orderOptions = getOrderOptions($sort.column, columnOptions)
 
   const getColumnOptions = (stickyColumn, columns) => {
@@ -46,8 +45,8 @@
 
   const updateSortColumn = e => {
     sort.update(state => ({
-      ...state,
       column: e.detail,
+      order: e.detail ? state.order : "ascending",
     }))
   }
 
@@ -57,29 +56,6 @@
       order: e.detail,
     }))
   }
-
-  // Ensure we never have a sort column selected that is not visible
-  const checkValidSortColumn = (sortColumn, stickyColumn, columns) => {
-    if (!sortColumn) {
-      return
-    }
-    if (
-      sortColumn !== stickyColumn?.name &&
-      !columns.some(col => col.name === sortColumn)
-    ) {
-      if (stickyColumn) {
-        sort.update(state => ({
-          ...state,
-          column: stickyColumn.name,
-        }))
-      } else {
-        sort.update(state => ({
-          ...state,
-          column: columns[0]?.name,
-        }))
-      }
-    }
-  }
 </script>
 
 <div bind:this={anchor}>
@@ -98,21 +74,23 @@
 <Popover bind:open {anchor} align="left">
   <div class="content">
     <Select
-      placeholder={null}
+      placeholder="Default"
       value={$sort.column}
       options={columnOptions}
       autoWidth
       on:change={updateSortColumn}
       label="Column"
     />
-    <Select
-      placeholder={null}
-      value={$sort.order}
-      options={orderOptions}
-      autoWidth
-      on:change={updateSortOrder}
-      label="Order"
-    />
+    {#if $sort.column}
+      <Select
+        placeholder={null}
+        value={$sort.order || "ascending"}
+        options={orderOptions}
+        autoWidth
+        on:change={updateSortOrder}
+        label="Order"
+      />
+    {/if}
   </div>
 </Popover>
 
diff --git a/packages/frontend-core/src/components/grid/stores/pagination.js b/packages/frontend-core/src/components/grid/stores/pagination.js
index c6a856e229..1dbea6e0d2 100644
--- a/packages/frontend-core/src/components/grid/stores/pagination.js
+++ b/packages/frontend-core/src/components/grid/stores/pagination.js
@@ -1,4 +1,4 @@
-import { derived } from "svelte/store"
+import { derived, get } from "svelte/store"
 
 export const initialise = context => {
   const { scrolledRowCount, rows, visualRowCapacity } = context
@@ -11,13 +11,12 @@ export const initialise = context => {
     [scrolledRowCount, rowCount, visualRowCapacity],
     ([$scrolledRowCount, $rowCount, $visualRowCapacity]) => {
       return Math.max(0, $rowCount - $scrolledRowCount - $visualRowCapacity)
-    },
-    100
+    }
   )
 
   // Fetch next page when fewer than 25 remaining rows to scroll
   remainingRows.subscribe(remaining => {
-    if (remaining < 25) {
+    if (remaining < 25 && get(rowCount)) {
       rows.actions.loadNextPage()
     }
   })
diff --git a/packages/frontend-core/src/components/grid/stores/sort.js b/packages/frontend-core/src/components/grid/stores/sort.js
index 689b278874..9b2dbfb8df 100644
--- a/packages/frontend-core/src/components/grid/stores/sort.js
+++ b/packages/frontend-core/src/components/grid/stores/sort.js
@@ -1,4 +1,4 @@
-import { get } from "svelte/store"
+import { derived, get } from "svelte/store"
 import { memo } from "../../../utils"
 
 export const createStores = context => {
@@ -17,13 +17,34 @@ export const createStores = context => {
 }
 
 export const initialise = context => {
-  const { sort, initialSortColumn, initialSortOrder } = context
+  const { sort, initialSortColumn, initialSortOrder, definition } = context
 
   // Reset sort when initial sort props change
   initialSortColumn.subscribe(newSortColumn => {
     sort.update(state => ({ ...state, column: newSortColumn }))
   })
   initialSortOrder.subscribe(newSortOrder => {
-    sort.update(state => ({ ...state, order: newSortOrder }))
+    sort.update(state => ({ ...state, order: newSortOrder || "ascending" }))
+  })
+
+  // Derive if the current sort column exists in the schema
+  const sortColumnExists = derived(
+    [sort, definition],
+    ([$sort, $definition]) => {
+      if (!$sort?.column) {
+        return true
+      }
+      return $definition?.schema?.[$sort.column] != null
+    }
+  )
+
+  // Clear sort state if our sort column does not exist
+  sortColumnExists.subscribe(exists => {
+    if (!exists) {
+      sort.set({
+        column: null,
+        order: "ascending",
+      })
+    }
   })
 }
diff --git a/packages/frontend-core/src/components/grid/stores/table.js b/packages/frontend-core/src/components/grid/stores/table.js
index 5c33e8171d..6c877e353a 100644
--- a/packages/frontend-core/src/components/grid/stores/table.js
+++ b/packages/frontend-core/src/components/grid/stores/table.js
@@ -59,51 +59,55 @@ export const createActions = context => {
 }
 
 export const initialise = context => {
-  const { datasource, fetch, filter, sort, definition } = context
+  const { datasource, fetch, filter, sort, table } = context
 
-  // Wipe filter whenever table ID changes to avoid using stale filters
+  // Keep a list of subscriptions so that we can clear them when the datasource
+  // config changes
+  let unsubscribers = []
+
+  // Observe datasource changes and apply logic for table datasources
   datasource.subscribe($datasource => {
-    if ($datasource?.type !== "table") {
+    // Clear previous subscriptions
+    unsubscribers?.forEach(unsubscribe => unsubscribe())
+    unsubscribers = []
+    if (!table.actions.isDatasourceValid($datasource)) {
       return
     }
+
+    // Wipe state
     filter.set([])
-  })
-
-  // Update fetch when filter changes
-  filter.subscribe($filter => {
-    if (get(datasource)?.type !== "table") {
-      return
-    }
-    get(fetch)?.update({
-      filter: $filter,
-    })
-  })
-
-  // Update fetch when sorting changes
-  sort.subscribe($sort => {
-    if (get(datasource)?.type !== "table") {
-      return
-    }
-    get(fetch)?.update({
-      sortOrder: $sort.order,
-      sortColumn: $sort.column,
-    })
-  })
-
-  // Ensure sorting UI reflects the fetch state whenever we reset the fetch,
-  // which triggers a new definition
-  definition.subscribe(() => {
-    if (get(datasource)?.type !== "table") {
-      return
-    }
-    const $fetch = get(fetch)
-    if (!$fetch) {
-      return
-    }
-    const { sortColumn, sortOrder } = get($fetch)
     sort.set({
-      column: sortColumn,
-      order: sortOrder,
+      column: null,
+      order: "ascending",
     })
+
+    // Update fetch when filter changes
+    unsubscribers.push(
+      filter.subscribe($filter => {
+        // Ensure we're updating the correct fetch
+        const $fetch = get(fetch)
+        if ($fetch?.options?.datasource?.tableId !== $datasource.tableId) {
+          return
+        }
+        $fetch.update({
+          filter: $filter,
+        })
+      })
+    )
+
+    // Update fetch when sorting changes
+    unsubscribers.push(
+      sort.subscribe($sort => {
+        // Ensure we're updating the correct fetch
+        const $fetch = get(fetch)
+        if ($fetch?.options?.datasource?.tableId !== $datasource.tableId) {
+          return
+        }
+        $fetch.update({
+          sortOrder: $sort.order || "ascending",
+          sortColumn: $sort.column,
+        })
+      })
+    )
   })
 }
diff --git a/packages/frontend-core/src/components/grid/stores/viewV2.js b/packages/frontend-core/src/components/grid/stores/viewV2.js
index b24bf56c98..f93dc57b69 100644
--- a/packages/frontend-core/src/components/grid/stores/viewV2.js
+++ b/packages/frontend-core/src/components/grid/stores/viewV2.js
@@ -69,62 +69,91 @@ export const createActions = context => {
 }
 
 export const initialise = context => {
-  const { definition, datasource, sort, rows, filter, subscribe } = context
+  const { definition, datasource, sort, rows, filter, subscribe, viewV2 } =
+    context
 
-  // Keep sort and filter state in line with the view definition
-  definition.subscribe($definition => {
-    if (!$definition || get(datasource)?.type !== "viewV2") {
+  // Keep a list of subscriptions so that we can clear them when the datasource
+  // config changes
+  let unsubscribers = []
+
+  // Observe datasource changes and apply logic for view V2 datasources
+  datasource.subscribe($datasource => {
+    // Clear previous subscriptions
+    unsubscribers?.forEach(unsubscribe => unsubscribe())
+    unsubscribers = []
+    if (!viewV2.actions.isDatasourceValid($datasource)) {
       return
     }
+
+    // Reset state for new view
+    filter.set([])
     sort.set({
-      column: $definition.sort?.field,
-      order: $definition.sort?.order,
+      column: null,
+      order: "ascending",
     })
-    filter.set($definition.query || [])
-  })
 
-  // When sorting changes, ensure view definition is kept up to date
-  sort.subscribe(async $sort => {
-    const $view = get(definition)
-    if (!$view || get(datasource)?.type !== "viewV2") {
-      return
-    }
-    if (
-      $sort?.column !== $view.sort?.field ||
-      $sort?.order !== $view.sort?.order
-    ) {
-      await datasource.actions.saveDefinition({
-        ...$view,
-        sort: {
-          field: $sort.column,
-          order: $sort.order,
-        },
+    // Keep sort and filter state in line with the view definition
+    unsubscribers.push(
+      definition.subscribe($definition => {
+        if ($definition?.id !== $datasource.id) {
+          return
+        }
+        sort.set({
+          column: $definition.sort?.field,
+          order: $definition.sort?.order || "ascending",
+        })
+        filter.set($definition.query || [])
       })
-      await rows.actions.refreshData()
-    }
-  })
+    )
 
-  // When filters change, ensure view definition is kept up to date
-  filter.subscribe(async $filter => {
-    const $view = get(definition)
-    if (!$view || get(datasource)?.type !== "viewV2") {
-      return
-    }
-    if (JSON.stringify($filter) !== JSON.stringify($view.query)) {
-      await datasource.actions.saveDefinition({
-        ...$view,
-        query: $filter,
+    // When sorting changes, ensure view definition is kept up to date
+    unsubscribers.push(
+      sort.subscribe(async $sort => {
+        // Ensure we're updating the correct view
+        const $view = get(definition)
+        if ($view?.id !== $datasource.id) {
+          return
+        }
+        if (
+          $sort?.column !== $view.sort?.field ||
+          $sort?.order !== $view.sort?.order
+        ) {
+          await datasource.actions.saveDefinition({
+            ...$view,
+            sort: {
+              field: $sort.column,
+              order: $sort.order || "ascending",
+            },
+          })
+          await rows.actions.refreshData()
+        }
       })
-      await rows.actions.refreshData()
-    }
-  })
+    )
 
-  // When hidden we show columns, we need to refresh data in order to fetch
-  // values for those columns
-  subscribe("show-column", async () => {
-    if (get(datasource)?.type !== "viewV2") {
-      return
-    }
-    await rows.actions.refreshData()
+    // When filters change, ensure view definition is kept up to date
+    unsubscribers?.push(
+      filter.subscribe(async $filter => {
+        // Ensure we're updating the correct view
+        const $view = get(definition)
+        if ($view?.id !== $datasource.id) {
+          return
+        }
+        if (JSON.stringify($filter) !== JSON.stringify($view.query)) {
+          await datasource.actions.saveDefinition({
+            ...$view,
+            query: $filter,
+          })
+          await rows.actions.refreshData()
+        }
+      })
+    )
+
+    // When hidden we show columns, we need to refresh data in order to fetch
+    // values for those columns
+    unsubscribers.push(
+      subscribe("show-column", async () => {
+        await rows.actions.refreshData()
+      })
+    )
   })
 }
diff --git a/packages/frontend-core/src/fetch/DataFetch.js b/packages/frontend-core/src/fetch/DataFetch.js
index 28fa68afe9..cd12535ddc 100644
--- a/packages/frontend-core/src/fetch/DataFetch.js
+++ b/packages/frontend-core/src/fetch/DataFetch.js
@@ -110,6 +110,17 @@ export default class DataFetch {
     return this.derivedStore.subscribe
   }
 
+  /**
+   * Gets the default sort column for this datasource
+   */
+  getDefaultSortColumn(definition, schema) {
+    if (definition?.primaryDisplay && schema[definition.primaryDisplay]) {
+      return definition.primaryDisplay
+    } else {
+      return Object.keys(schema)[0]
+    }
+  }
+
   /**
    * Fetches a fresh set of data from the server, resetting pagination
    */
@@ -118,12 +129,6 @@ export default class DataFetch {
 
     // Fetch datasource definition and extract sort properties if configured
     const definition = await this.getDefinition(datasource)
-    if (definition?.sort?.field) {
-      this.options.sortColumn = definition.sort.field
-    }
-    if (definition?.sort?.order) {
-      this.options.sortOrder = definition.sort.order
-    }
 
     // Determine feature flags
     const features = this.determineFeatureFlags(definition)
@@ -140,32 +145,32 @@ export default class DataFetch {
       return
     }
 
-    // If no sort order, default to descending
-    if (!this.options.sortOrder) {
+    // If an invalid sort column is specified, delete it
+    if (this.options.sortColumn && !schema[this.options.sortColumn]) {
+      this.options.sortColumn = null
+    }
+
+    // If no sort column, get the default column for this datasource
+    if (!this.options.sortColumn) {
+      this.options.sortColumn = this.getDefaultSortColumn(definition, schema)
+    }
+
+    // If we don't have a sort column specified then just ensure we don't set
+    // any sorting params
+    if (!this.options.sortColumn) {
       this.options.sortOrder = "ascending"
-    }
+      this.options.sortType = null
+    } else {
+      // Otherwise determine what sort type to use base on sort column
+      const type = schema?.[this.options.sortColumn]?.type
+      this.options.sortType =
+        type === "number" || type === "bigint" ? "number" : "string"
 
-    // If no sort column, or an invalid sort column is provided, use the primary
-    // display and fallback to first column
-    const sortValid = this.options.sortColumn && schema[this.options.sortColumn]
-    if (!sortValid) {
-      let newSortColumn
-      if (definition?.primaryDisplay && schema[definition.primaryDisplay]) {
-        newSortColumn = definition.primaryDisplay
-      } else {
-        newSortColumn = Object.keys(schema)[0]
+      // If no sort order, default to ascending
+      if (!this.options.sortOrder) {
+        this.options.sortOrder = "ascending"
       }
-      this.options.sortColumn = newSortColumn
     }
-    const { sortOrder, sortColumn } = this.options
-
-    // Determine what sort type to use
-    let sortType = "string"
-    if (sortColumn) {
-      const type = schema?.[sortColumn]?.type
-      sortType = type === "number" || type === "bigint" ? "number" : "string"
-    }
-    this.options.sortType = sortType
 
     // Build the lucene query
     let query = this.options.query
@@ -182,8 +187,6 @@ export default class DataFetch {
       loading: true,
       cursors: [],
       cursor: null,
-      sortOrder,
-      sortColumn,
     }))
 
     // Actually fetch data
@@ -351,6 +354,14 @@ export default class DataFetch {
     const entries = Object.entries(newOptions || {})
     for (let [key, value] of entries) {
       if (JSON.stringify(value) !== JSON.stringify(this.options[key])) {
+        console.log(
+          key,
+          "is different",
+          "new",
+          value,
+          "vs old",
+          this.options[key]
+        )
         refresh = true
         break
       }
diff --git a/packages/frontend-core/src/fetch/QueryFetch.js b/packages/frontend-core/src/fetch/QueryFetch.js
index 456abaec79..6420893515 100644
--- a/packages/frontend-core/src/fetch/QueryFetch.js
+++ b/packages/frontend-core/src/fetch/QueryFetch.js
@@ -29,6 +29,10 @@ export default class QueryFetch extends DataFetch {
     }
   }
 
+  getDefaultSortColumn() {
+    return null
+  }
+
   async getData() {
     const { datasource, limit, paginate } = this.options
     const { supportsPagination } = this.features
diff --git a/packages/frontend-core/src/fetch/ViewV2Fetch.js b/packages/frontend-core/src/fetch/ViewV2Fetch.js
index d8b96df7ae..b9eaf4bdf7 100644
--- a/packages/frontend-core/src/fetch/ViewV2Fetch.js
+++ b/packages/frontend-core/src/fetch/ViewV2Fetch.js
@@ -4,9 +4,6 @@ import { get } from "svelte/store"
 export default class ViewV2Fetch extends DataFetch {
   determineFeatureFlags() {
     return {
-      // The API does not actually support dynamic filtering, but since views
-      // have filters built in we don't want to perform client side filtering
-      // which would happen if we marked this as false
       supportsSearch: true,
       supportsSort: true,
       supportsPagination: true,
@@ -33,18 +30,23 @@ export default class ViewV2Fetch extends DataFetch {
     }
   }
 
+  getDefaultSortColumn() {
+    return null
+  }
+
   async getData() {
     const { datasource, limit, sortColumn, sortOrder, sortType, paginate } =
       this.options
-    const { cursor } = get(this.store)
+    const { cursor, query } = get(this.store)
     try {
       const res = await this.API.viewV2.fetch({
         viewId: datasource.id,
+        query,
         paginate,
         limit,
         bookmark: cursor,
         sort: sortColumn,
-        sortOrder,
+        sortOrder: sortOrder?.toLowerCase(),
         sortType,
       })
       return {
diff --git a/packages/server/src/api/controllers/row/views.ts b/packages/server/src/api/controllers/row/views.ts
index 455a4c0aa2..578c775a55 100644
--- a/packages/server/src/api/controllers/row/views.ts
+++ b/packages/server/src/api/controllers/row/views.ts
@@ -6,9 +6,11 @@ import {
   SearchViewRowRequest,
   RequiredKeys,
   SearchParams,
+  SearchFilters,
 } from "@budibase/types"
 import { dataFilters } from "@budibase/shared-core"
 import sdk from "../../../sdk"
+import { db } from "@budibase/backend-core"
 
 export async function searchView(
   ctx: UserCtx<SearchViewRowRequest, SearchRowResponse>
@@ -19,15 +21,37 @@ export async function searchView(
   if (!view) {
     ctx.throw(404, `View ${viewId} not found`)
   }
-
   if (view.version !== 2) {
     ctx.throw(400, `This method only supports viewsV2`)
   }
 
   const viewFields = Object.keys(view.schema || {})
-
   const { body } = ctx.request
-  const query = dataFilters.buildLuceneQuery(view.query || [])
+
+  // Enrich saved query with ephemeral query params.
+  // We prevent searching on any fields that are saved as part of the query, as
+  // that could let users find rows they should not be allowed to access.
+  let query = dataFilters.buildLuceneQuery(view.query || [])
+  if (body.query) {
+    // Extract existing fields
+    const existingFields =
+      view.query
+        ?.filter(filter => filter.field)
+        .map(filter => db.removeKeyNumbering(filter.field)) || []
+
+    // Prevent using an "OR" search
+    delete body.query.allOr
+
+    // Carry over filters for unused fields
+    Object.keys(body.query).forEach(key => {
+      const operator = key as keyof Omit<SearchFilters, "allOr">
+      Object.keys(body.query[operator] || {}).forEach(field => {
+        if (!existingFields.includes(db.removeKeyNumbering(field))) {
+          query[operator]![field] = body.query[operator]![field]
+        }
+      })
+    })
+  }
 
   const searchOptions: RequiredKeys<SearchViewRowRequest> &
     RequiredKeys<Pick<SearchParams, "tableId" | "query" | "fields">> = {
diff --git a/packages/types/src/api/web/app/rows.ts b/packages/types/src/api/web/app/rows.ts
index 2b51c7b203..a99ef0e837 100644
--- a/packages/types/src/api/web/app/rows.ts
+++ b/packages/types/src/api/web/app/rows.ts
@@ -16,7 +16,13 @@ export interface SearchRowRequest extends Omit<SearchParams, "tableId"> {}
 export interface SearchViewRowRequest
   extends Pick<
     SearchRowRequest,
-    "sort" | "sortOrder" | "sortType" | "limit" | "bookmark" | "paginate"
+    | "sort"
+    | "sortOrder"
+    | "sortType"
+    | "limit"
+    | "bookmark"
+    | "paginate"
+    | "query"
   > {}
 
 export interface SearchRowResponse {