From cef0fdd3ea8475eeeb4cdf0e0f2419e3e2e91348 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 24 May 2024 14:03:48 +0200 Subject: [PATCH 01/21] Move ViewV2Enriched out of document types --- packages/types/src/api/web/app/view.ts | 3 ++- packages/types/src/documents/app/view.ts | 6 +----- packages/types/src/sdk/index.ts | 1 + packages/types/src/sdk/view.ts | 5 +++++ 4 files changed, 9 insertions(+), 6 deletions(-) create mode 100644 packages/types/src/sdk/view.ts diff --git a/packages/types/src/api/web/app/view.ts b/packages/types/src/api/web/app/view.ts index c00bc0e468..a6be5e2986 100644 --- a/packages/types/src/api/web/app/view.ts +++ b/packages/types/src/api/web/app/view.ts @@ -1,4 +1,5 @@ -import { ViewV2, ViewV2Enriched } from "../../../documents" +import { ViewV2 } from "../../../documents" +import { ViewV2Enriched } from "../../../sdk/view" export interface ViewResponse { data: ViewV2 diff --git a/packages/types/src/documents/app/view.ts b/packages/types/src/documents/app/view.ts index 8a36b96b4e..7b93d24f3d 100644 --- a/packages/types/src/documents/app/view.ts +++ b/packages/types/src/documents/app/view.ts @@ -1,5 +1,5 @@ import { SearchFilter, SortOrder, SortType } from "../../api" -import { TableSchema, UIFieldMetadata } from "./table" +import { UIFieldMetadata } from "./table" import { Document } from "../document" import { DBView } from "../../sdk" @@ -48,10 +48,6 @@ export interface ViewV2 { schema?: Record } -export interface ViewV2Enriched extends ViewV2 { - schema?: TableSchema -} - export type ViewSchema = ViewCountOrSumSchema | ViewStatisticsSchema export interface ViewCountOrSumSchema { diff --git a/packages/types/src/sdk/index.ts b/packages/types/src/sdk/index.ts index 36faaae9c3..d87ec58b0c 100644 --- a/packages/types/src/sdk/index.ts +++ b/packages/types/src/sdk/index.ts @@ -21,3 +21,4 @@ export * from "./websocket" export * from "./permissions" export * from "./row" export * from "./vm" +export * from "./view" diff --git a/packages/types/src/sdk/view.ts b/packages/types/src/sdk/view.ts new file mode 100644 index 0000000000..cb551dada9 --- /dev/null +++ b/packages/types/src/sdk/view.ts @@ -0,0 +1,5 @@ +import { TableSchema, ViewV2 } from "../documents" + +export interface ViewV2Enriched extends ViewV2 { + schema?: TableSchema +} From ee77e08b3cf126640f34407d7d4b1309fcca3e36 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 24 May 2024 14:04:50 +0200 Subject: [PATCH 02/21] Fix build --- packages/types/src/api/web/app/table.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/types/src/api/web/app/table.ts b/packages/types/src/api/web/app/table.ts index c9d63feaab..fc382643f1 100644 --- a/packages/types/src/api/web/app/table.ts +++ b/packages/types/src/api/web/app/table.ts @@ -1,10 +1,5 @@ -import { - Row, - Table, - TableRequest, - View, - ViewV2Enriched, -} from "../../../documents" +import { Row, Table, TableRequest, View } from "../../../documents" +import { ViewV2Enriched } from "../../../sdk" export type TableViewsResponse = { [key: string]: View | ViewV2Enriched } From a0c2843236442c96cd2ac10cef99a19b6a809c19 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 24 May 2024 14:28:04 +0200 Subject: [PATCH 03/21] Extend view metadata --- packages/server/src/api/controllers/view/viewsV2.ts | 9 +++++---- packages/types/src/documents/app/view.ts | 6 +++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/controllers/view/viewsV2.ts b/packages/server/src/api/controllers/view/viewsV2.ts index eb28883e15..0087d377ad 100644 --- a/packages/server/src/api/controllers/view/viewsV2.ts +++ b/packages/server/src/api/controllers/view/viewsV2.ts @@ -3,7 +3,7 @@ import { CreateViewRequest, Ctx, RequiredKeys, - UIFieldMetadata, + ViewUIFieldMetadata, UpdateViewRequest, ViewResponse, ViewResponseEnriched, @@ -18,20 +18,21 @@ async function parseSchema(view: CreateViewRequest) { const finalViewSchema = view.schema && Object.entries(view.schema).reduce((p, [fieldName, schemaValue]) => { - const fieldSchema: RequiredKeys = { + const fieldSchema: RequiredKeys = { order: schemaValue.order, width: schemaValue.width, visible: schemaValue.visible, + readonly: schemaValue.readonly, icon: schemaValue.icon, } Object.entries(fieldSchema) .filter(([, val]) => val === undefined) .forEach(([key]) => { - delete fieldSchema[key as keyof UIFieldMetadata] + delete fieldSchema[key as keyof ViewUIFieldMetadata] }) p[fieldName] = fieldSchema return p - }, {} as Record>) + }, {} as Record>) for (let [key, column] of Object.entries(finalViewSchema)) { if (!column.visible) { delete finalViewSchema[key] diff --git a/packages/types/src/documents/app/view.ts b/packages/types/src/documents/app/view.ts index 7b93d24f3d..2572ddba5d 100644 --- a/packages/types/src/documents/app/view.ts +++ b/packages/types/src/documents/app/view.ts @@ -33,6 +33,10 @@ export interface View { groupBy?: string } +export type ViewUIFieldMetadata = UIFieldMetadata & { + readonly?: boolean +} + export interface ViewV2 { version: 2 id: string @@ -45,7 +49,7 @@ export interface ViewV2 { order?: SortOrder type?: SortType } - schema?: Record + schema?: Record } export type ViewSchema = ViewCountOrSumSchema | ViewStatisticsSchema From 28137f95006005b7b1585e3890790b7f330d989f Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 24 May 2024 16:07:07 +0200 Subject: [PATCH 04/21] Validate view schema on upsert --- .../src/api/routes/tests/viewV2.spec.ts | 8 ++++-- packages/server/src/sdk/app/views/index.ts | 28 ++++++++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 2ad02a8082..61c8dbbe0f 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -141,7 +141,7 @@ describe.each([ type: SortType.STRING, }, schema: { - name: { + Price: { visible: true, }, }, @@ -150,7 +150,11 @@ describe.each([ expect(res).toEqual({ ...newView, - schema: newView.schema, + schema: { + Price: { + visible: true, + }, + }, id: expect.any(String), version: 2, }) diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 2edfd900c4..e62931b934 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -2,10 +2,11 @@ import { RenameColumn, TableSchema, View, + ViewUIFieldMetadata, ViewV2, ViewV2Enriched, } from "@budibase/types" -import { db as dbCore } from "@budibase/backend-core" +import { HTTPError, db as dbCore } from "@budibase/backend-core" import { cloneDeep } from "lodash" import * as utils from "../../../db/utils" @@ -13,6 +14,7 @@ import { isExternalTableID } from "../../../integrations/utils" import * as internal from "./internal" import * as external from "./external" +import sdk from "../../../sdk" function pickApi(tableId: any) { if (isExternalTableID(tableId)) { @@ -31,14 +33,38 @@ export async function getEnriched(viewId: string): Promise { return pickApi(tableId).getEnriched(viewId) } +async function guardViewSchema( + tableId: string, + schema?: Record +) { + if (!schema || !Object.keys(schema).length) { + return + } + const table = await sdk.tables.getTable(tableId) + if (schema) { + for (const field of Object.keys(schema)) { + if (!table.schema[field]) { + throw new HTTPError( + `Field "${field}" is not valid for the requested table`, + 400 + ) + } + } + } +} + export async function create( tableId: string, viewRequest: Omit ): Promise { + await guardViewSchema(tableId, viewRequest.schema) + return pickApi(tableId).create(tableId, viewRequest) } export async function update(tableId: string, view: ViewV2): Promise { + await guardViewSchema(tableId, view.schema) + return pickApi(tableId).update(tableId, view) } From 6acb3f666991c5423dae98c8b2ff8b707ec495cc Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Fri, 24 May 2024 16:15:24 +0200 Subject: [PATCH 05/21] Validation test --- .../src/api/routes/tests/viewV2.spec.ts | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 61c8dbbe0f..8469bc52f7 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -218,6 +218,48 @@ describe.each([ status: 201, }) }) + + it("does not persist non-visible fields", async () => { + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, + primaryDisplay: generator.word(), + schema: { + Price: { visible: true }, + Category: { visible: false }, + }, + } + const res = await config.api.viewV2.create(newView) + + expect(res).toEqual({ + ...newView, + schema: { + Price: { + visible: true, + }, + }, + id: expect.any(String), + version: 2, + }) + }) + + it("throws bad request when the schema fields are not valid", async () => { + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, + schema: { + nonExisting: { + visible: true, + }, + }, + } + await config.api.viewV2.create(newView, { + status: 400, + body: { + message: 'Field "nonExisting" is not valid for the requested table', + }, + }) + }) }) describe("update", () => { From 65d2aa50c677fcab6cb05ce57127d15396a36e54 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 13:39:43 +0200 Subject: [PATCH 06/21] Guard readonly fields --- .../src/api/controllers/view/viewsV2.ts | 2 +- .../src/api/routes/tests/viewV2.spec.ts | 47 +++++++++++++++++++ packages/server/src/sdk/app/views/index.ts | 22 +++++++-- packages/server/src/utilities/schema.ts | 23 ++++++--- 4 files changed, 81 insertions(+), 13 deletions(-) diff --git a/packages/server/src/api/controllers/view/viewsV2.ts b/packages/server/src/api/controllers/view/viewsV2.ts index 0087d377ad..76807b796a 100644 --- a/packages/server/src/api/controllers/view/viewsV2.ts +++ b/packages/server/src/api/controllers/view/viewsV2.ts @@ -34,7 +34,7 @@ async function parseSchema(view: CreateViewRequest) { return p }, {} as Record>) for (let [key, column] of Object.entries(finalViewSchema)) { - if (!column.visible) { + if (!column.visible && !column.readonly) { delete finalViewSchema[key] } } diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 8469bc52f7..aad8736a62 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -23,6 +23,14 @@ import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" import merge from "lodash/merge" import { quotas } from "@budibase/pro" import { roles } from "@budibase/backend-core" +import * as schemaUtils from "../../../utilities/schema" + +jest.mock("../../../utilities/schema", () => { + return { + __esModule: true, + ...jest.requireActual("../../../utilities/schema"), + } +}) describe.each([ ["internal", undefined], @@ -260,6 +268,45 @@ describe.each([ }, }) }) + + it("required fields cannot be marked as readonly", async () => { + const isRequiredSpy = jest.spyOn(schemaUtils, "isRequired") + + isRequiredSpy.mockReturnValue(true) + + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + description: { + name: "description", + type: FieldType.STRING, + }, + }, + }) + ) + + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, + schema: { + name: { + readonly: true, + }, + }, + } + + await config.api.viewV2.create(newView, { + status: 400, + body: { + message: 'Field "name" cannot be readonly as it is a required field', + status: 400, + }, + }) + }) }) describe("update", () => { diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index e62931b934..68a689fb78 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -15,6 +15,7 @@ import { isExternalTableID } from "../../../integrations/utils" import * as internal from "./internal" import * as external from "./external" import sdk from "../../../sdk" +import { isRequired } from "../../../utilities/schema" function pickApi(tableId: any) { if (isExternalTableID(tableId)) { @@ -35,20 +36,31 @@ export async function getEnriched(viewId: string): Promise { async function guardViewSchema( tableId: string, - schema?: Record + viewSchema?: Record ) { - if (!schema || !Object.keys(schema).length) { + if (!viewSchema || !Object.keys(viewSchema).length) { return } const table = await sdk.tables.getTable(tableId) - if (schema) { - for (const field of Object.keys(schema)) { - if (!table.schema[field]) { + if (viewSchema) { + for (const field of Object.keys(viewSchema)) { + const tableSchemaField = table.schema[field] + if (!tableSchemaField) { throw new HTTPError( `Field "${field}" is not valid for the requested table`, 400 ) } + + if ( + viewSchema[field].readonly && + isRequired(tableSchemaField.constraints) + ) { + throw new HTTPError( + `Field "${field}" cannot be readonly as it is a required field`, + 400 + ) + } } } } diff --git a/packages/server/src/utilities/schema.ts b/packages/server/src/utilities/schema.ts index f73701fdfd..421d3ef8f2 100644 --- a/packages/server/src/utilities/schema.ts +++ b/packages/server/src/utilities/schema.ts @@ -4,6 +4,7 @@ import { TableSchema, FieldSchema, Row, + FieldConstraints, } from "@budibase/types" import { ValidColumnNameRegex, utils } from "@budibase/shared-core" import { db } from "@budibase/backend-core" @@ -40,6 +41,15 @@ export function isRows(rows: any): rows is Rows { return Array.isArray(rows) && rows.every(row => typeof row === "object") } +export function isRequired(constraints: FieldConstraints | undefined) { + const isRequired = + !!constraints && + ((typeof constraints.presence !== "boolean" && + !constraints.presence?.allowEmpty) || + constraints.presence === true) + return isRequired +} + export function validate(rows: Rows, schema: TableSchema): ValidationResults { const results: ValidationResults = { schemaValidation: {}, @@ -62,12 +72,6 @@ export function validate(rows: Rows, schema: TableSchema): ValidationResults { return } - const isRequired = - !!constraints && - ((typeof constraints.presence !== "boolean" && - !constraints.presence?.allowEmpty) || - constraints.presence === true) - // If the columnType is not a string, then it's not present in the schema, and should be added to the invalid columns array if (typeof columnType !== "string") { results.invalidColumns.push(columnName) @@ -101,7 +105,12 @@ export function validate(rows: Rows, schema: TableSchema): ValidationResults { } else if ( (columnType === FieldType.BB_REFERENCE || columnType === FieldType.BB_REFERENCE_SINGLE) && - !isValidBBReference(columnData, columnType, columnSubtype, isRequired) + !isValidBBReference( + columnData, + columnType, + columnSubtype, + isRequired(constraints) + ) ) { results.schemaValidation[columnName] = false } else { From 3cf230e3af054e28c3de91da96fd36579b6ee5dd Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 13:59:39 +0200 Subject: [PATCH 07/21] Add tests --- .../src/api/routes/tests/viewV2.spec.ts | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index aad8736a62..7efa7ec84d 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -269,6 +269,48 @@ describe.each([ }) }) + it("readonly fields are persisted", async () => { + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + description: { + name: "description", + type: FieldType.STRING, + }, + }, + }) + ) + + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, + schema: { + name: { + visible: true, + readonly: true, + }, + description: { + readonly: true, + }, + }, + } + + const res = await config.api.viewV2.create(newView) + expect(res.schema).toEqual({ + name: { + visible: true, + readonly: true, + }, + description: { + readonly: true, + }, + }) + }) + it("required fields cannot be marked as readonly", async () => { const isRequiredSpy = jest.spyOn(schemaUtils, "isRequired") From ae36a79f8cbd7d7ddf1d1c32da97000d3874e98d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 14:10:20 +0200 Subject: [PATCH 08/21] Add tests --- packages/server/src/utilities/schema.ts | 2 +- .../server/src/utilities/tests/schema.spec.ts | 35 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 packages/server/src/utilities/tests/schema.spec.ts diff --git a/packages/server/src/utilities/schema.ts b/packages/server/src/utilities/schema.ts index 421d3ef8f2..7fea417d5a 100644 --- a/packages/server/src/utilities/schema.ts +++ b/packages/server/src/utilities/schema.ts @@ -45,7 +45,7 @@ export function isRequired(constraints: FieldConstraints | undefined) { const isRequired = !!constraints && ((typeof constraints.presence !== "boolean" && - !constraints.presence?.allowEmpty) || + constraints.presence?.allowEmpty === false) || constraints.presence === true) return isRequired } diff --git a/packages/server/src/utilities/tests/schema.spec.ts b/packages/server/src/utilities/tests/schema.spec.ts new file mode 100644 index 0000000000..664e95a42a --- /dev/null +++ b/packages/server/src/utilities/tests/schema.spec.ts @@ -0,0 +1,35 @@ +import { isRequired } from "../schema" + +describe("schema utilities", () => { + describe("isRequired", () => { + it("not required by default", () => { + const result = isRequired(undefined) + expect(result).toBe(false) + }) + + it("required when presence is true", () => { + const result = isRequired({ presence: true }) + expect(result).toBe(true) + }) + + it("not required when presence is false", () => { + const result = isRequired({ presence: false }) + expect(result).toBe(false) + }) + + it("not required when presence is an empty object", () => { + const result = isRequired({ presence: {} }) + expect(result).toBe(false) + }) + + it("not required when allowEmpty is true", () => { + const result = isRequired({ presence: { allowEmpty: true } }) + expect(result).toBe(false) + }) + + it("required when allowEmpty is false", () => { + const result = isRequired({ presence: { allowEmpty: false } }) + expect(result).toBe(true) + }) + }) +}) From 9bac192cf908d682461367d7b372db4c0890689f Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 14:11:50 +0200 Subject: [PATCH 09/21] Add tests --- packages/server/src/api/routes/tests/viewV2.spec.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 7efa7ec84d..9b50c28649 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -313,8 +313,7 @@ describe.each([ it("required fields cannot be marked as readonly", async () => { const isRequiredSpy = jest.spyOn(schemaUtils, "isRequired") - - isRequiredSpy.mockReturnValue(true) + isRequiredSpy.mockReturnValueOnce(true) const table = await config.api.table.save( saveTableRequest({ @@ -410,6 +409,10 @@ describe.each([ Category: { visible: false, }, + Price: { + visible: true, + readonly: true, + }, }, } await config.api.viewV2.update(updatedData) @@ -426,7 +429,8 @@ describe.each([ visible: false, }), Price: expect.objectContaining({ - visible: false, + visible: true, + readonly: true, }), }, }, From 041f85886cd99ebe5a1ba0dc7d8092e05f41d144 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 14:16:03 +0200 Subject: [PATCH 10/21] Ensure consistency --- .../src/api/routes/tests/viewV2.spec.ts | 38 +++++++++++++++++++ packages/server/src/sdk/app/views/index.ts | 22 +++++++---- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 9b50c28649..68f1259670 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -294,6 +294,7 @@ describe.each([ readonly: true, }, description: { + visible: true, readonly: true, }, }, @@ -306,6 +307,7 @@ describe.each([ readonly: true, }, description: { + visible: true, readonly: true, }, }) @@ -348,6 +350,42 @@ describe.each([ }, }) }) + + it("readonly fields must be visible", async () => { + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + description: { + name: "description", + type: FieldType.STRING, + }, + }, + }) + ) + + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, + schema: { + name: { + visible: false, + readonly: true, + }, + }, + } + + await config.api.viewV2.create(newView, { + status: 400, + body: { + message: 'Field "name" cannot be readonly and not visible', + status: 400, + }, + }) + }) }) describe("update", () => { diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 68a689fb78..42c6c9079f 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -52,14 +52,20 @@ async function guardViewSchema( ) } - if ( - viewSchema[field].readonly && - isRequired(tableSchemaField.constraints) - ) { - throw new HTTPError( - `Field "${field}" cannot be readonly as it is a required field`, - 400 - ) + if (viewSchema[field].readonly) { + if (isRequired(tableSchemaField.constraints)) { + throw new HTTPError( + `Field "${field}" cannot be readonly as it is a required field`, + 400 + ) + } + + if (!viewSchema[field].visible) { + throw new HTTPError( + `Field "${field}" cannot be readonly and not visible`, + 400 + ) + } } } } From 4329900a642639c93556d68ec2c28e1a230780ab Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 14:31:16 +0200 Subject: [PATCH 11/21] Add viewReadonlyColumns feature --- packages/types/src/sdk/licensing/feature.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/types/src/sdk/licensing/feature.ts b/packages/types/src/sdk/licensing/feature.ts index a3f149d6da..0d8db8258e 100644 --- a/packages/types/src/sdk/licensing/feature.ts +++ b/packages/types/src/sdk/licensing/feature.ts @@ -14,6 +14,7 @@ export enum Feature { OFFLINE = "offline", EXPANDED_PUBLIC_API = "expandedPublicApi", VIEW_PERMISSIONS = "viewPermissions", + VIEW_READONLY_COLUMNS = "viewReadonlyColumns", } export type PlanFeatures = { [key in PlanType]: Feature[] | undefined } From 1eb929736c66f85122891cbee42babb6afcf58c9 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 14:31:26 +0200 Subject: [PATCH 12/21] Guard --- packages/pro | 2 +- packages/server/src/sdk/app/views/index.ts | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/pro b/packages/pro index d3c3077011..d67e126044 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit d3c3077011a8e20ed3c48dcd6301caca4120b6ac +Subproject commit d67e126044bde383fb8c316c3c89a37fae74e349 diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 42c6c9079f..2d0c046b08 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -7,6 +7,7 @@ import { ViewV2Enriched, } from "@budibase/types" import { HTTPError, db as dbCore } from "@budibase/backend-core" +import { features } from "@budibase/pro" import { cloneDeep } from "lodash" import * as utils from "../../../db/utils" @@ -53,6 +54,13 @@ async function guardViewSchema( } if (viewSchema[field].readonly) { + if (!(await features.isViewReadonlyColumnsEnabled())) { + throw new HTTPError( + `Readonly fields are not enabled for your tenant`, + 400 + ) + } + if (isRequired(tableSchemaField.constraints)) { throw new HTTPError( `Field "${field}" cannot be readonly as it is a required field`, From 422b1e275312ae727ed91eb0e8a3b6b30784ea1d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 14:31:45 +0200 Subject: [PATCH 13/21] Fix tests --- .../tests/core/utilities/mocks/licenses.ts | 4 + .../src/api/routes/tests/viewV2.spec.ts | 181 ++++++++++-------- 2 files changed, 100 insertions(+), 85 deletions(-) diff --git a/packages/backend-core/tests/core/utilities/mocks/licenses.ts b/packages/backend-core/tests/core/utilities/mocks/licenses.ts index 1cbc282575..ec7d2af794 100644 --- a/packages/backend-core/tests/core/utilities/mocks/licenses.ts +++ b/packages/backend-core/tests/core/utilities/mocks/licenses.ts @@ -106,6 +106,10 @@ export const useViewPermissions = () => { return useFeature(Feature.VIEW_PERMISSIONS) } +export const useViewReadonlyColumns = () => { + return useFeature(Feature.VIEW_READONLY_COLUMNS) +} + // QUOTAS export const setAutomationLogsQuota = (value: number) => { diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 68f1259670..980e34c0bc 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -104,6 +104,10 @@ describe.each([ setup.afterAll() }) + beforeEach(() => { + mocks.licenses.useCloudFree() + }) + const getRowUsage = async () => { const { total } = await config.doInContext(undefined, () => quotas.getCurrentUsageValues(QuotaUsageType.STATIC, StaticQuotaName.ROWS) @@ -269,26 +273,43 @@ describe.each([ }) }) - it("readonly fields are persisted", async () => { - const table = await config.api.table.save( - saveTableRequest({ + describe("readonly fields", () => { + beforeEach(() => { + mocks.licenses.useViewReadonlyColumns() + }) + it("readonly fields are persisted", async () => { + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + description: { + name: "description", + type: FieldType.STRING, + }, + }, + }) + ) + + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, schema: { name: { - name: "name", - type: FieldType.STRING, + visible: true, + readonly: true, }, description: { - name: "description", - type: FieldType.STRING, + visible: true, + readonly: true, }, }, - }) - ) + } - const newView: CreateViewRequest = { - name: generator.name(), - tableId: table._id!, - schema: { + const res = await config.api.viewV2.create(newView) + expect(res.schema).toEqual({ name: { visible: true, readonly: true, @@ -297,93 +318,82 @@ describe.each([ visible: true, readonly: true, }, - }, - } - - const res = await config.api.viewV2.create(newView) - expect(res.schema).toEqual({ - name: { - visible: true, - readonly: true, - }, - description: { - visible: true, - readonly: true, - }, + }) }) - }) - it("required fields cannot be marked as readonly", async () => { - const isRequiredSpy = jest.spyOn(schemaUtils, "isRequired") - isRequiredSpy.mockReturnValueOnce(true) + it("required fields cannot be marked as readonly", async () => { + const isRequiredSpy = jest.spyOn(schemaUtils, "isRequired") + isRequiredSpy.mockReturnValueOnce(true) - const table = await config.api.table.save( - saveTableRequest({ + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + description: { + name: "description", + type: FieldType.STRING, + }, + }, + }) + ) + + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, schema: { name: { - name: "name", - type: FieldType.STRING, - }, - description: { - name: "description", - type: FieldType.STRING, + readonly: true, }, }, + } + + await config.api.viewV2.create(newView, { + status: 400, + body: { + message: + 'Field "name" cannot be readonly as it is a required field', + status: 400, + }, }) - ) - - const newView: CreateViewRequest = { - name: generator.name(), - tableId: table._id!, - schema: { - name: { - readonly: true, - }, - }, - } - - await config.api.viewV2.create(newView, { - status: 400, - body: { - message: 'Field "name" cannot be readonly as it is a required field', - status: 400, - }, }) - }) - it("readonly fields must be visible", async () => { - const table = await config.api.table.save( - saveTableRequest({ + it("readonly fields must be visible", async () => { + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + description: { + name: "description", + type: FieldType.STRING, + }, + }, + }) + ) + + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, schema: { name: { - name: "name", - type: FieldType.STRING, - }, - description: { - name: "description", - type: FieldType.STRING, + visible: false, + readonly: true, }, }, + } + + await config.api.viewV2.create(newView, { + status: 400, + body: { + message: 'Field "name" cannot be readonly and not visible', + status: 400, + }, }) - ) - - const newView: CreateViewRequest = { - name: generator.name(), - tableId: table._id!, - schema: { - name: { - visible: false, - readonly: true, - }, - }, - } - - await config.api.viewV2.create(newView, { - status: 400, - body: { - message: 'Field "name" cannot be readonly and not visible', - status: 400, - }, }) }) }) @@ -423,6 +433,7 @@ describe.each([ }) it("can update all fields", async () => { + mocks.licenses.useViewReadonlyColumns() const tableId = table._id! const updatedData: Required = { From 9e4c575d7987702c2ff593ff3f92caf682c558c1 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 14:44:00 +0200 Subject: [PATCH 14/21] Add license tests --- .../src/api/routes/tests/viewV2.spec.ts | 70 ++++++++++++++++++- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 980e34c0bc..fa6b0ca304 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -277,6 +277,7 @@ describe.each([ beforeEach(() => { mocks.licenses.useViewReadonlyColumns() }) + it("readonly fields are persisted", async () => { const table = await config.api.table.save( saveTableRequest({ @@ -395,6 +396,43 @@ describe.each([ }, }) }) + + it("readonly fields cannot be used on free license", async () => { + mocks.licenses.useCloudFree() + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + description: { + name: "description", + type: FieldType.STRING, + }, + }, + }) + ) + + const newView: CreateViewRequest = { + name: generator.name(), + tableId: table._id!, + schema: { + name: { + visible: true, + readonly: true, + }, + }, + } + + await config.api.viewV2.create(newView, { + status: 400, + body: { + message: "Readonly fields are not enabled for your tenant", + status: 400, + }, + }) + }) }) }) @@ -638,6 +676,28 @@ describe.each([ } ) }) + + it("cannot update views with readonly on on free license", async () => { + mocks.licenses.useViewReadonlyColumns() + + view = await config.api.viewV2.update({ + ...view, + schema: { + Price: { + visible: true, + readonly: true, + }, + }, + }) + + mocks.licenses.useCloudFree() + await config.api.viewV2.create(view, { + status: 400, + body: { + message: "Readonly fields are not enabled for your tenant", + }, + }) + }) }) describe("delete", () => { @@ -686,8 +746,10 @@ describe.each([ }) describe("read", () => { - it("views have extra data trimmed", async () => { - const table = await config.api.table.save( + let view: ViewV2 + + beforeAll(async () => { + table = await config.api.table.save( saveTableRequest({ schema: { Country: { @@ -702,7 +764,7 @@ describe.each([ }) ) - const view = await config.api.viewV2.create({ + view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), schema: { @@ -711,7 +773,9 @@ describe.each([ }, }, }) + }) + it("views have extra data trimmed", async () => { let row = await config.api.row.save(view.id, { Country: "Aussy", Story: "aaaaa", From ab0bac689bd59ba77582245e3ef36075ad5a7ed8 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 14:53:26 +0200 Subject: [PATCH 15/21] Add tests --- .../src/api/routes/tests/viewV2.spec.ts | 63 ++++++++++++++++++- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index fa6b0ca304..3160dae510 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -14,8 +14,8 @@ import { StaticQuotaName, Table, TableSourceType, - UIFieldMetadata, UpdateViewRequest, + ViewUIFieldMetadata, ViewV2, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" @@ -698,6 +698,45 @@ describe.each([ }, }) }) + + it("can remove readonly config after license downgrade", async () => { + mocks.licenses.useViewReadonlyColumns() + + view = await config.api.viewV2.update({ + ...view, + schema: { + Price: { + visible: true, + readonly: true, + }, + Category: { + visible: true, + readonly: true, + }, + }, + }) + mocks.licenses.useCloudFree() + const res = await config.api.viewV2.update({ + ...view, + schema: { + Price: { + visible: true, + readonly: false, + }, + }, + }) + expect(res).toEqual( + expect.objectContaining({ + ...view, + schema: { + Price: { + visible: true, + readonly: false, + }, + }, + }) + ) + }) }) describe("delete", () => { @@ -739,9 +778,27 @@ describe.each([ const updatedTable = await config.api.table.get(table._id!) const viewSchema = updatedTable.views![view!.name!].schema as Record< string, - UIFieldMetadata + ViewUIFieldMetadata > expect(viewSchema.Price?.visible).toEqual(false) + expect(viewSchema.Category?.visible).toEqual(true) + }) + + it("should be able to fetch readonly config after downgrades", async () => { + mocks.licenses.useViewReadonlyColumns() + const res = await config.api.viewV2.create({ + name: generator.name(), + tableId: table._id!, + schema: { + Price: { visible: true, readonly: true }, + }, + }) + + mocks.licenses.useCloudFree() + const view = await config.api.viewV2.get(res.id) + expect(view.schema?.Price).toEqual( + expect.objectContaining({ visible: true, readonly: true }) + ) }) }) @@ -773,7 +830,7 @@ describe.each([ }, }, }) - }) + }) it("views have extra data trimmed", async () => { let row = await config.api.row.save(view.id, { From 99659748beb3ef197aa5164dc6282c92607796bd Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 29 May 2024 15:40:32 +0200 Subject: [PATCH 16/21] Update submodule --- packages/pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pro b/packages/pro index d67e126044..8d20fe2fd3 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit d67e126044bde383fb8c316c3c89a37fae74e349 +Subproject commit 8d20fe2fd3ebb4b5bf6ce2f5b8f255b7e846a02e From 05544d30827221b9a7a73d1fd06b06ec575049a3 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 29 May 2024 15:45:33 +0200 Subject: [PATCH 17/21] Simplify mock --- packages/server/src/api/routes/tests/viewV2.spec.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 3160dae510..5b335542c0 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -25,12 +25,7 @@ import { quotas } from "@budibase/pro" import { roles } from "@budibase/backend-core" import * as schemaUtils from "../../../utilities/schema" -jest.mock("../../../utilities/schema", () => { - return { - __esModule: true, - ...jest.requireActual("../../../utilities/schema"), - } -}) +jest.mock("../../../utilities/schema") describe.each([ ["internal", undefined], From 2d5ecb6e3eda375e0be950e61fa37894eb77e28c Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 29 May 2024 15:50:10 +0200 Subject: [PATCH 18/21] PR comments --- .../src/api/routes/tests/viewV2.spec.ts | 5 +- packages/server/src/sdk/app/views/index.ts | 49 +++++++++---------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 5b335542c0..7b3619351a 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -386,7 +386,8 @@ describe.each([ await config.api.viewV2.create(newView, { status: 400, body: { - message: 'Field "name" cannot be readonly and not visible', + message: + 'Field "name" must be visible if you want to make it readonly', status: 400, }, }) @@ -686,7 +687,7 @@ describe.each([ }) mocks.licenses.useCloudFree() - await config.api.viewV2.create(view, { + await config.api.viewV2.update(view, { status: 400, body: { message: "Readonly fields are not enabled for your tenant", diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 2d0c046b08..da66e8ac18 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -43,37 +43,36 @@ async function guardViewSchema( return } const table = await sdk.tables.getTable(tableId) - if (viewSchema) { - for (const field of Object.keys(viewSchema)) { - const tableSchemaField = table.schema[field] - if (!tableSchemaField) { + + for (const field of Object.keys(viewSchema)) { + const tableSchemaField = table.schema[field] + if (!tableSchemaField) { + throw new HTTPError( + `Field "${field}" is not valid for the requested table`, + 400 + ) + } + + if (viewSchema[field].readonly) { + if (!(await features.isViewReadonlyColumnsEnabled())) { throw new HTTPError( - `Field "${field}" is not valid for the requested table`, + `Readonly fields are not enabled for your tenant`, 400 ) } - if (viewSchema[field].readonly) { - if (!(await features.isViewReadonlyColumnsEnabled())) { - throw new HTTPError( - `Readonly fields are not enabled for your tenant`, - 400 - ) - } + if (isRequired(tableSchemaField.constraints)) { + throw new HTTPError( + `Field "${field}" cannot be readonly as it is a required field`, + 400 + ) + } - if (isRequired(tableSchemaField.constraints)) { - throw new HTTPError( - `Field "${field}" cannot be readonly as it is a required field`, - 400 - ) - } - - if (!viewSchema[field].visible) { - throw new HTTPError( - `Field "${field}" cannot be readonly and not visible`, - 400 - ) - } + if (!viewSchema[field].visible) { + throw new HTTPError( + `Field "${field}" must be visible if you want to make it readonly`, + 400 + ) } } } From 4b0e433c450353ab4f373ab0355b7b2c6390dbd4 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 27 May 2024 16:36:45 +0200 Subject: [PATCH 19/21] Prevent write readonly column --- .../src/api/routes/tests/viewV2.spec.ts | 48 +++++++++++++++++++ packages/server/src/sdk/app/views/index.ts | 8 +++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 7b3619351a..8069fadf10 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -873,6 +873,27 @@ describe.each([ expect(row.one).toBeUndefined() expect(row.two).toEqual("bar") }) + + it("can't persist readonly columns", async () => { + mocks.licenses.useViewReadonlyColumns() + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + one: { visible: true, readonly: true }, + two: { visible: true }, + }, + }) + const row = await config.api.row.save(view.id, { + tableId: table!._id, + _viewId: view.id, + one: "foo", + two: "bar", + }) + + expect(row.one).toBeUndefined() + expect(row.two).toEqual("bar") + }) }) describe("patch", () => { @@ -893,6 +914,33 @@ describe.each([ expect(row.one).toEqual("foo") expect(row.two).toEqual("newBar") }) + + it("can't update readonly columns", async () => { + mocks.licenses.useViewReadonlyColumns() + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + one: { visible: true, readonly: true }, + two: { visible: true }, + }, + }) + const newRow = await config.api.row.save(table._id!, { + one: "foo", + two: "bar", + }) + await config.api.row.patch(view.id, { + tableId: table._id!, + _id: newRow._id!, + _rev: newRow._rev!, + one: "newFoo", + two: "newBar", + }) + + const row = await config.api.row.get(table._id!, newRow._id!) + expect(row.one).toEqual("foo") + expect(row.two).toEqual("newBar") + }) }) describe("destroy", () => { diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index da66e8ac18..ea05ecf512 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -104,7 +104,13 @@ export async function remove(viewId: string): Promise { export function allowedFields(view: View | ViewV2) { return [ - ...Object.keys(view?.schema || {}), + ...Object.keys(view?.schema || {}).filter(key => { + if (!isV2(view)) { + return true + } + const fieldSchema = view.schema![key] + return fieldSchema.visible && !fieldSchema.readonly + }), ...dbCore.CONSTANT_EXTERNAL_ROW_COLS, ...dbCore.CONSTANT_INTERNAL_ROW_COLS, ] From 2417cbd461f6c6a80029c487e7a4abab6bcc6654 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 29 May 2024 15:56:58 +0200 Subject: [PATCH 20/21] Styling --- packages/pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pro b/packages/pro index 8d20fe2fd3..5189b83bea 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit 8d20fe2fd3ebb4b5bf6ce2f5b8f255b7e846a02e +Subproject commit 5189b83bea1868574ff7f4c51fe5db38a11badb8 From 5b6242ae779c98a34688d908af9cc3c1f8a79f4e Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 29 May 2024 15:58:39 +0200 Subject: [PATCH 21/21] Fix test --- .../server/src/middleware/tests/trimViewRowInfo.spec.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/server/src/middleware/tests/trimViewRowInfo.spec.ts b/packages/server/src/middleware/tests/trimViewRowInfo.spec.ts index 243f9a9329..29d80759c7 100644 --- a/packages/server/src/middleware/tests/trimViewRowInfo.spec.ts +++ b/packages/server/src/middleware/tests/trimViewRowInfo.spec.ts @@ -144,8 +144,12 @@ describe("trimViewRowInfo middleware", () => { name: generator.guid(), tableId: table._id!, schema: { - name: {}, - address: {}, + name: { + visible: true, + }, + address: { + visible: true, + }, }, })