Merge pull request #14468 from Budibase/BUDI-8562/fix-trimming-views

Trimming views

packages/server/src/api/routes/tests/viewV2.spec.ts

@@ -15,7 +15,6 @@ import {
   Table,
   TableSourceType,
   UpdateViewRequest,
-  ViewUIFieldMetadata,
   ViewV2,
   SearchResponse,
   BasicOperator,
@@ -125,18 +124,7 @@ describe.each([
     mocks.licenses.useCloudFree()
   })
 
-  const getRowUsage = async () => {
+  describe("view crud", () => {
-    const { total } = await config.doInContext(undefined, () =>
-      quotas.getCurrentUsageValues(QuotaUsageType.STATIC, StaticQuotaName.ROWS)
-    )
-    return total
-  }
-
-  const assertRowUsage = async (expected: number) => {
-    const usage = await getRowUsage()
-    expect(usage).toBe(expected)
-  }
-
     describe("create", () => {
       it("persist the view when the view is successfully created", async () => {
         const newView: CreateViewRequest = {
@@ -584,7 +572,9 @@ describe.each([
         expect((await config.api.table.get(tableId)).views).toEqual({
           [view.name]: {
             ...view,
-          query: [{ operator: "equal", field: "newField", value: "thatValue" }],
+            query: [
+              { operator: "equal", field: "newField", value: "thatValue" },
+            ],
             schema: expect.anything(),
           },
         })
@@ -886,7 +876,8 @@ describe.each([
 
           // Update the view to an invalid state
           const tableToUpdate = await config.api.table.get(table._id!)
-        ;(tableToUpdate.views![view.name] as ViewV2).schema!.id.visible = false
+          ;(tableToUpdate.views![view.name] as ViewV2).schema!.id.visible =
+            false
           await db.getDB(config.appId!).put(tableToUpdate)
 
           view = await config.api.viewV2.get(view.id)
@@ -937,26 +928,114 @@ describe.each([
       })
     })
 
-  describe("fetch view (through table)", () => {
+    describe.each([
-    it("should be able to fetch a view V2", async () => {
+      ["from view api", (view: ViewV2) => config.api.viewV2.get(view.id)],
-      const res = await config.api.viewV2.create({
+      [
-        name: generator.name(),
+        "from table",
-        tableId: table._id!,
+        async (view: ViewV2) => {
+          const table = await config.api.table.get(view.tableId)
+          return table.views![view.name] as ViewV2
+        },
+      ],
+    ])("read (%s)", (_, getDelegate) => {
+      let table: Table
+      let tableId: string
+
+      beforeEach(async () => {
+        table = await config.api.table.save(
+          saveTableRequest({
+            schema: {
+              one: {
+                type: FieldType.STRING,
+                name: "one",
+              },
+              two: {
+                type: FieldType.STRING,
+                name: "two",
+              },
+              three: {
+                type: FieldType.STRING,
+                name: "three",
+              },
+            },
+          })
+        )
+        tableId = table._id!
+      })
+
+      it("retrieves the view data with the enriched schema", async () => {
+        const view = await config.api.viewV2.create({
+          tableId,
+          name: generator.guid(),
           schema: {
             id: { visible: true },
-          Price: { visible: false },
+            one: { visible: true },
-          Category: { visible: true },
+            two: { visible: true },
           },
         })
 
-      const view = await config.api.viewV2.get(res.id)
+        expect(await getDelegate(view)).toEqual({
-      const updatedTable = await config.api.table.get(table._id!)
+          ...view,
-      const viewSchema = updatedTable.views![view!.name!].schema as Record<
+          schema: {
-        string,
+            id: { ...table.schema["id"], visible: true },
-        ViewUIFieldMetadata
+            one: { ...table.schema["one"], visible: true },
-      >
+            two: { ...table.schema["two"], visible: true },
-      expect(viewSchema.Price?.visible).toEqual(false)
+            three: { ...table.schema["three"], visible: false },
-      expect(viewSchema.Category?.visible).toEqual(true)
+          },
+        })
+      })
+
+      it("does not include columns removed from the table", async () => {
+        const view = await config.api.viewV2.create({
+          tableId,
+          name: generator.guid(),
+          schema: {
+            id: { visible: true },
+            one: { visible: true },
+            two: { visible: true },
+          },
+        })
+        const table = await config.api.table.get(tableId)
+        const { one: _, ...newSchema } = table.schema
+        await config.api.table.save({ ...table, schema: newSchema })
+
+        expect(await getDelegate(view)).toEqual({
+          ...view,
+          schema: {
+            id: { ...table.schema["id"], visible: true },
+            two: { ...table.schema["two"], visible: true },
+            three: { ...table.schema["three"], visible: false },
+          },
+        })
+      })
+
+      it("does not include columns hidden from the table", async () => {
+        const view = await config.api.viewV2.create({
+          tableId,
+          name: generator.guid(),
+          schema: {
+            id: { visible: true },
+            one: { visible: true },
+            two: { visible: true },
+          },
+        })
+        const table = await config.api.table.get(tableId)
+        await config.api.table.save({
+          ...table,
+          schema: {
+            ...table.schema,
+            two: { ...table.schema["two"], visible: false },
+          },
+        })
+
+        expect(await getDelegate(view)).toEqual({
+          ...view,
+          schema: {
+            id: { ...table.schema["id"], visible: true },
+            one: { ...table.schema["one"], visible: true },
+            three: { ...table.schema["three"], visible: false },
+          },
+        })
       })
 
       it("should be able to fetch readonly config after downgrades", async () => {
@@ -966,58 +1045,135 @@ describe.each([
           tableId: table._id!,
           schema: {
             id: { visible: true },
-          Price: { visible: true, readonly: true },
+            one: { visible: true, readonly: true },
           },
         })
 
         mocks.licenses.useCloudFree()
-      const view = await config.api.viewV2.get(res.id)
+        const view = await getDelegate(res)
-      expect(view.schema?.Price).toEqual(
+        expect(view.schema?.one).toEqual(
           expect.objectContaining({ visible: true, readonly: true })
         )
       })
     })
 
-  describe("read", () => {
+    describe("updating table schema", () => {
-    let view: ViewV2
+      describe("existing columns changed to required", () => {
-
+        beforeEach(async () => {
-    beforeAll(async () => {
           table = await config.api.table.save(
             saveTableRequest({
               schema: {
-            Country: {
+                id: {
-              type: FieldType.STRING,
+                  name: "id",
-              name: "Country",
+                  type: FieldType.NUMBER,
+                  autocolumn: true,
                 },
-            Story: {
+                name: {
+                  name: "name",
                   type: FieldType.STRING,
-              name: "Story",
                 },
               },
             })
           )
+        })
 
-      view = await config.api.viewV2.create({
+        it("allows updating when no views constrains the field", async () => {
+          await config.api.viewV2.create({
+            name: "view a",
             tableId: table._id!,
-        name: generator.guid(),
             schema: {
               id: { visible: true },
-          Country: {
+              name: { visible: true },
+            },
+          })
+
+          table = await config.api.table.get(table._id!)
+          await config.api.table.save(
+            {
+              ...table,
+              schema: {
+                ...table.schema,
+                name: {
+                  name: "name",
+                  type: FieldType.STRING,
+                  constraints: { presence: { allowEmpty: false } },
+                },
+              },
+            },
+            { status: 200 }
+          )
+        })
+
+        it("rejects if field is readonly in any view", async () => {
+          mocks.licenses.useViewReadonlyColumns()
+
+          await config.api.viewV2.create({
+            name: "view a",
+            tableId: table._id!,
+            schema: {
+              id: { visible: true },
+              name: {
                 visible: true,
+                readonly: true,
               },
             },
           })
+
+          table = await config.api.table.get(table._id!)
+          await config.api.table.save(
+            {
+              ...table,
+              schema: {
+                ...table.schema,
+                name: {
+                  name: "name",
+                  type: FieldType.STRING,
+                  constraints: { presence: true },
+                },
+              },
+            },
+            {
+              status: 400,
+              body: {
+                status: 400,
+                message:
+                  'To make field "name" required, this field must be present and writable in views: view a.',
+              },
+            }
+          )
         })
 
-    it("views have extra data trimmed", async () => {
+        it("rejects if field is hidden in any view", async () => {
-      let row = await config.api.row.save(view.id, {
+          await config.api.viewV2.create({
-        Country: "Aussy",
+            name: "view a",
-        Story: "aaaaa",
+            tableId: table._id!,
+            schema: { id: { visible: true } },
           })
 
-      row = await config.api.row.get(table._id!, row._id!)
+          table = await config.api.table.get(table._id!)
-      expect(row.Story).toBeUndefined()
+          await config.api.table.save(
-      expect(row.Country).toEqual("Aussy")
+            {
+              ...table,
+              schema: {
+                ...table.schema,
+                name: {
+                  name: "name",
+                  type: FieldType.STRING,
+                  constraints: { presence: true },
+                },
+              },
+            },
+            {
+              status: 400,
+              body: {
+                status: 400,
+                message:
+                  'To make field "name" required, this field must be present and writable in views: view a.',
+              },
+            }
+          )
+        })
+      })
     })
   })
 
@@ -1084,9 +1240,36 @@ describe.each([
         expect(row.one).toBeUndefined()
         expect(row.two).toEqual("bar")
       })
+
+      it("should not return non-view view fields for a row", async () => {
+        const newRow = await config.api.row.save(view.id, {
+          one: "foo",
+          two: "bar",
+        })
+
+        expect(newRow.one).toBeUndefined()
+        expect(newRow.two).toEqual("bar")
+      })
     })
 
     describe("patch", () => {
+      it("should not return non-view view fields for a row", async () => {
+        const newRow = await config.api.row.save(table._id!, {
+          one: "foo",
+          two: "bar",
+        })
+        const row = await config.api.row.patch(view.id, {
+          tableId: table._id!,
+          _id: newRow._id!,
+          _rev: newRow._rev!,
+          one: "newFoo",
+          two: "newBar",
+        })
+
+        expect(row.one).toBeUndefined()
+        expect(row.two).toEqual("newBar")
+      })
+
       it("should update only the view fields for a row", async () => {
         const newRow = await config.api.row.save(table._id!, {
           one: "foo",
@@ -1135,6 +1318,21 @@ describe.each([
     })
 
     describe("destroy", () => {
+      const getRowUsage = async () => {
+        const { total } = await config.doInContext(undefined, () =>
+          quotas.getCurrentUsageValues(
+            QuotaUsageType.STATIC,
+            StaticQuotaName.ROWS
+          )
+        )
+        return total
+      }
+
+      const assertRowUsage = async (expected: number) => {
+        const usage = await getRowUsage()
+        expect(usage).toBe(expected)
+      }
+
       it("should be able to delete a row", async () => {
         const createdRow = await config.api.row.save(table._id!, {})
         const rowUsage = await getRowUsage()
@@ -1167,6 +1365,50 @@ describe.each([
       })
     })
 
+    describe("read", () => {
+      let view: ViewV2
+      let table: Table
+
+      beforeAll(async () => {
+        table = await config.api.table.save(
+          saveTableRequest({
+            schema: {
+              Country: {
+                type: FieldType.STRING,
+                name: "Country",
+              },
+              Story: {
+                type: FieldType.STRING,
+                name: "Story",
+              },
+            },
+          })
+        )
+
+        view = await config.api.viewV2.create({
+          tableId: table._id!,
+          name: generator.guid(),
+          schema: {
+            id: { visible: true },
+            Country: {
+              visible: true,
+            },
+          },
+        })
+      })
+
+      it("views have extra data trimmed", async () => {
+        let row = await config.api.row.save(view.id, {
+          Country: "Aussy",
+          Story: "aaaaa",
+        })
+
+        row = await config.api.row.get(table._id!, row._id!)
+        expect(row.Story).toBeUndefined()
+        expect(row.Country).toEqual("Aussy")
+      })
+    })
+
     describe("search", () => {
       it("returns empty rows from view when no schema is passed", async () => {
         const rows = await Promise.all(
@@ -1795,123 +2037,4 @@ describe.each([
       })
     })
   })
-
-  describe("updating table schema", () => {
-    describe("existing columns changed to required", () => {
-      beforeEach(async () => {
-        table = await config.api.table.save(
-          saveTableRequest({
-            schema: {
-              id: {
-                name: "id",
-                type: FieldType.NUMBER,
-                autocolumn: true,
-              },
-              name: {
-                name: "name",
-                type: FieldType.STRING,
-              },
-            },
-          })
-        )
-      })
-
-      it("allows updating when no views constrains the field", async () => {
-        await config.api.viewV2.create({
-          name: "view a",
-          tableId: table._id!,
-          schema: {
-            id: { visible: true },
-            name: { visible: true },
-          },
-        })
-
-        table = await config.api.table.get(table._id!)
-        await config.api.table.save(
-          {
-            ...table,
-            schema: {
-              ...table.schema,
-              name: {
-                name: "name",
-                type: FieldType.STRING,
-                constraints: { presence: { allowEmpty: false } },
-              },
-            },
-          },
-          { status: 200 }
-        )
-      })
-
-      it("rejects if field is readonly in any view", async () => {
-        mocks.licenses.useViewReadonlyColumns()
-
-        await config.api.viewV2.create({
-          name: "view a",
-          tableId: table._id!,
-          schema: {
-            id: { visible: true },
-            name: {
-              visible: true,
-              readonly: true,
-            },
-          },
-        })
-
-        table = await config.api.table.get(table._id!)
-        await config.api.table.save(
-          {
-            ...table,
-            schema: {
-              ...table.schema,
-              name: {
-                name: "name",
-                type: FieldType.STRING,
-                constraints: { presence: true },
-              },
-            },
-          },
-          {
-            status: 400,
-            body: {
-              status: 400,
-              message:
-                'To make field "name" required, this field must be present and writable in views: view a.',
-            },
-          }
-        )
-      })
-
-      it("rejects if field is hidden in any view", async () => {
-        await config.api.viewV2.create({
-          name: "view a",
-          tableId: table._id!,
-          schema: { id: { visible: true } },
-        })
-
-        table = await config.api.table.get(table._id!)
-        await config.api.table.save(
-          {
-            ...table,
-            schema: {
-              ...table.schema,
-              name: {
-                name: "name",
-                type: FieldType.STRING,
-                constraints: { presence: true },
-              },
-            },
-          },
-          {
-            status: 400,
-            body: {
-              status: 400,
-              message:
-                'To make field "name" required, this field must be present and writable in views: view a.',
-            },
-          }
-        )
-      })
-    })
-  })
 })

packages/server/src/middleware/trimViewRowInfo.ts

@@ -1,10 +1,10 @@
-import { Ctx, Row } from "@budibase/types"
+import { Ctx, Row, ViewV2 } from "@budibase/types"
 
 import sdk from "../sdk"
 import { Next } from "koa"
 import { getSourceId } from "../api/controllers/row/utils"
 
-export default async (ctx: Ctx<Row>, next: Next) => {
+export default async (ctx: Ctx<Row, Row>, next: Next) => {
   const { body } = ctx.request
   const viewId = getSourceId(ctx).viewId ?? body._viewId
 
@@ -14,22 +14,31 @@ export default async (ctx: Ctx<Row>, next: Next) => {
   }
 
   // don't need to trim delete requests
-  if (ctx?.method?.toLowerCase() !== "delete") {
+  const trimFields = ctx?.method?.toLowerCase() !== "delete"
-    await trimViewFields(ctx.request.body, viewId)
+  if (!trimFields) {
-  }
-
     return next()
   }
 
-// have to mutate the koa context, can't return
-export async function trimViewFields(body: Row, viewId: string): Promise<void> {
   const view = await sdk.views.get(viewId)
-  const allowedKeys = sdk.views.allowedFields(view)
+  ctx.request.body = await trimNonViewFields(ctx.request.body, view, "WRITE")
+
+  await next()
+
+  ctx.body = await trimNonViewFields(ctx.body, view, "READ")
+}
+
+// have to mutate the koa context, can't return
+export async function trimNonViewFields(
+  row: Row,
+  view: ViewV2,
+  permission: "WRITE" | "READ"
+): Promise<Row> {
+  row = { ...row }
+  const allowedKeys = sdk.views.allowedFields(view, permission)
   // have to mutate the context, can't update reference
-  const toBeRemoved = Object.keys(body).filter(
+  const toBeRemoved = Object.keys(row).filter(key => !allowedKeys.includes(key))
-    key => !allowedKeys.includes(key)
-  )
   for (let removeKey of toBeRemoved) {
-    delete body[removeKey]
+    delete row[removeKey]
   }
+  return row
 }

packages/server/src/sdk/app/views/index.ts

@@ -13,7 +13,6 @@ import {
   PROTECTED_EXTERNAL_COLUMNS,
   PROTECTED_INTERNAL_COLUMNS,
 } from "@budibase/shared-core"
-import { cloneDeep } from "lodash/fp"
 
 import * as utils from "../../../db/utils"
 import { isExternalTableID } from "../../../integrations/utils"
@@ -139,14 +138,20 @@ export async function remove(viewId: string): Promise<ViewV2> {
   return pickApi(tableId).remove(viewId)
 }
 
-export function allowedFields(view: View | ViewV2) {
+export function allowedFields(
+  view: View | ViewV2,
+  permission: "WRITE" | "READ"
+) {
   return [
     ...Object.keys(view?.schema || {}).filter(key => {
       if (!isV2(view)) {
         return true
       }
       const fieldSchema = view.schema![key]
+      if (permission === "WRITE") {
         return fieldSchema.visible && !fieldSchema.readonly
+      }
+      return fieldSchema.visible
     }),
     ...PROTECTED_EXTERNAL_COLUMNS,
     ...PROTECTED_INTERNAL_COLUMNS,
@@ -157,17 +162,19 @@ export function enrichSchema(
   view: ViewV2,
   tableSchema: TableSchema
 ): ViewV2Enriched {
-  let schema = cloneDeep(tableSchema)
+  let schema: TableSchema = {}
   const anyViewOrder = Object.values(view.schema || {}).some(
     ui => ui.order != null
   )
-  for (const key of Object.keys(schema)) {
+  for (const key of Object.keys(tableSchema).filter(
+    key => tableSchema[key].visible !== false
+  )) {
     // if nothing specified in view, then it is not visible
     const ui = view.schema?.[key] || { visible: false }
     schema[key] = {
-      ...schema[key],
+      ...tableSchema[key],
       ...ui,
-      order: anyViewOrder ? ui?.order ?? undefined : schema[key].order,
+      order: anyViewOrder ? ui?.order ?? undefined : tableSchema[key].order,
     }
   }
 

packages/server/src/sdk/app/views/tests/views.spec.ts

@@ -101,14 +101,6 @@ describe("table sdk", () => {
               type: "number",
             },
           },
-          hiddenField: {
-            type: "string",
-            name: "hiddenField",
-            visible: false,
-            constraints: {
-              type: "string",
-            },
-          },
         },
       })
     })
@@ -143,10 +135,6 @@ describe("table sdk", () => {
             ...basicTable.schema.id,
             visible: true,
           },
-          hiddenField: {
-            ...basicTable.schema.hiddenField,
-            visible: false,
-          },
         },
       })
     })
@@ -181,10 +169,6 @@ describe("table sdk", () => {
             ...basicTable.schema.id,
             visible: false,
           },
-          hiddenField: {
-            ...basicTable.schema.hiddenField,
-            visible: false,
-          },
         },
       })
     })
@@ -209,7 +193,6 @@ describe("table sdk", () => {
         expect.objectContaining({
           ...view,
           schema: {
-            ...basicTable.schema,
             name: {
               type: "string",
               name: "name",
@@ -264,7 +247,6 @@ describe("table sdk", () => {
         expect.objectContaining({
           ...view,
           schema: {
-            ...basicTable.schema,
             name: {
               type: "string",
               name: "name",