From a879b5814ab3191eb224bb49c3795d08b6eb7846 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 7 Jun 2024 16:57:33 +0100 Subject: [PATCH 1/6] Making sure that columns get updated to allow nulls/disallow correctly, as well as making sure enums can be updated and autocolumn state can change. --- packages/server/src/integrations/utils/utils.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/server/src/integrations/utils/utils.ts b/packages/server/src/integrations/utils/utils.ts index 157bdba3bd..41e62f8975 100644 --- a/packages/server/src/integrations/utils/utils.ts +++ b/packages/server/src/integrations/utils/utils.ts @@ -280,12 +280,29 @@ function copyExistingPropsOver( utils.unreachable(existingColumnType) } + // copy the BB schema in case of special props if (shouldKeepSchema) { + const fetchedColumnDefinition: FieldSchema | undefined = + table.schema[key] table.schema[key] = { ...existingTableSchema[key], externalType: existingTableSchema[key].externalType || table.schema[key]?.externalType, + autocolumn: !!fetchedColumnDefinition?.autocolumn, + } as FieldSchema + // check constraints which can be fetched from the DB (they could be updated) + if (fetchedColumnDefinition?.constraints) { + // inclusions are the enum values (select/options) + const fetchedInclusion = fetchedColumnDefinition.constraints.inclusion + const oldInclusion = table.schema[key].constraints?.inclusion + table.schema[key].constraints = { + ...table.schema[key].constraints, + presence: !!fetchedColumnDefinition.constraints?.presence, + inclusion: fetchedInclusion?.length + ? fetchedInclusion + : oldInclusion, + } } } } From 9cd7c144f4d06f4d415f3effafa72f4df7681183 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 7 Jun 2024 16:57:46 +0100 Subject: [PATCH 2/6] Adding test case. --- .../src/integration-test/postgres.spec.ts | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/packages/server/src/integration-test/postgres.spec.ts b/packages/server/src/integration-test/postgres.spec.ts index ccf63d0820..9da56034d1 100644 --- a/packages/server/src/integration-test/postgres.spec.ts +++ b/packages/server/src/integration-test/postgres.spec.ts @@ -1265,4 +1265,48 @@ describe("postgres integrations", () => { expect(JSON.parse(decoded)[0]).toBe("1111") }) }) + + describe("check fetching null/not null table", () => { + beforeAll(async () => { + await rawQuery( + rawDatasource, + `CREATE TABLE nullableTable ( + order_id SERIAL PRIMARY KEY, + order_number INT NOT NULL + ); + ` + ) + }) + + it("should be able to change the table to allow nullable and refetch this", async () => { + const response = await makeRequest( + "post", + `/api/datasources/${datasource._id}/schema` + ) + const entities = response.body.datasource.entities + expect(entities).toBeDefined() + const nullableTable = entities["nullabletable"] + expect(nullableTable).toBeDefined() + expect(nullableTable.schema["order_number"].constraints.presence).toEqual( + true + ) + await rawQuery( + rawDatasource, + `ALTER TABLE nullableTable + ALTER COLUMN order_number DROP NOT NULL; + ` + ) + const responseAfter = await makeRequest( + "post", + `/api/datasources/${datasource._id}/schema` + ) + const entitiesAfter = responseAfter.body.datasource.entities + expect(entitiesAfter).toBeDefined() + const nullableTableAfter = entitiesAfter["nullabletable"] + expect(nullableTableAfter).toBeDefined() + expect( + nullableTableAfter.schema["order_number"].constraints.presence + ).toEqual(false) + }) + }) }) From 5de2dc838222c36fe2946b9c77e8867204eb2206 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 7 Jun 2024 17:13:06 +0100 Subject: [PATCH 3/6] Some test updates to make the fetchSchema tableFilter prop usable. --- .../src/api/routes/tests/datasource.spec.ts | 2 +- .../src/integration-test/postgres.spec.ts | 95 ++++++++----------- .../src/tests/utilities/api/datasource.ts | 15 ++- 3 files changed, 55 insertions(+), 57 deletions(-) diff --git a/packages/server/src/api/routes/tests/datasource.spec.ts b/packages/server/src/api/routes/tests/datasource.spec.ts index f2cea90675..9aa494eb14 100644 --- a/packages/server/src/api/routes/tests/datasource.spec.ts +++ b/packages/server/src/api/routes/tests/datasource.spec.ts @@ -341,7 +341,7 @@ describe("/datasources", () => { ) const persisted = await config.api.datasource.get(datasourceId) - await config.api.datasource.fetchSchema(datasourceId) + await config.api.datasource.fetchSchema({ datasourceId }) const updated = await config.api.datasource.get(datasourceId) const expected: Datasource = { diff --git a/packages/server/src/integration-test/postgres.spec.ts b/packages/server/src/integration-test/postgres.spec.ts index 9da56034d1..99369202ec 100644 --- a/packages/server/src/integration-test/postgres.spec.ts +++ b/packages/server/src/integration-test/postgres.spec.ts @@ -1097,12 +1097,11 @@ describe("postgres integrations", () => { it("recognises when a table has no primary key", async () => { await rawQuery(rawDatasource, `CREATE TABLE "${tableName}" (id SERIAL)`) - const response = await makeRequest( - "post", - `/api/datasources/${datasource._id}/schema` - ) + const response = await config.api.datasource.fetchSchema({ + datasourceId: datasource._id!, + }) - expect(response.body.errors).toEqual({ + expect(response.errors).toEqual({ [tableName]: "Table must have a primary key.", }) }) @@ -1113,12 +1112,11 @@ describe("postgres integrations", () => { `CREATE TABLE "${tableName}" (_id SERIAL PRIMARY KEY) ` ) - const response = await makeRequest( - "post", - `/api/datasources/${datasource._id}/schema` - ) + const response = await config.api.datasource.fetchSchema({ + datasourceId: datasource._id!, + }) - expect(response.body.errors).toEqual({ + expect(response.errors).toEqual({ [tableName]: "Table contains invalid columns.", }) }) @@ -1143,15 +1141,14 @@ describe("postgres integrations", () => { ` ) - const response = await makeRequest( - "post", - `/api/datasources/${datasource._id}/schema` - ) + const response = await config.api.datasource.fetchSchema({ + datasourceId: datasource._id!, + }) - const table = response.body.datasource.entities[tableName] + const table = response.datasource.entities?.[tableName] expect(table).toBeDefined() - expect(table.schema[enumColumnName].type).toEqual(FieldType.OPTIONS) + expect(table?.schema[enumColumnName].type).toEqual(FieldType.OPTIONS) }) }) @@ -1215,20 +1212,16 @@ describe("postgres integrations", () => { rawDatasource, `CREATE TABLE "${schema2}".${repeated_table_name} (id2 SERIAL PRIMARY KEY, val2 TEXT);` ) - const response = await makeRequest( - "post", - `/api/datasources/${datasource._id}/schema`, - { - tablesFilter: [repeated_table_name], - } - ) - expect(response.status).toBe(200) + + const response = await config.api.datasource.fetchSchema({ + datasourceId: datasource._id!, + tablesFilter: [repeated_table_name], + }) expect( - response.body.datasource.entities[repeated_table_name].schema + response.datasource.entities?.[repeated_table_name].schema ).toBeDefined() - const schema = - response.body.datasource.entities[repeated_table_name].schema - expect(Object.keys(schema).sort()).toEqual(["id", "val1"]) + const schema = response.datasource.entities?.[repeated_table_name].schema + expect(Object.keys(schema || {}).sort()).toEqual(["id", "val1"]) }) }) @@ -1246,16 +1239,14 @@ describe("postgres integrations", () => { }) it("should handle binary columns", async () => { - const response = await makeRequest( - "post", - `/api/datasources/${datasource._id}/schema` - ) - expect(response.body).toBeDefined() - expect(response.body.datasource.entities).toBeDefined() - const table = response.body.datasource.entities["binarytable"] + const response = await config.api.datasource.fetchSchema({ + datasourceId: datasource._id!, + }) + expect(response.datasource.entities).toBeDefined() + const table = response.datasource.entities?.["binarytable"] expect(table).toBeDefined() - expect(table.schema.id.externalType).toBe("bytea") - const row = await config.api.row.save(table._id, { + expect(table?.schema.id.externalType).toBe("bytea") + const row = await config.api.row.save(table?._id!, { id: "1111", column1: "hello", column2: 222, @@ -1279,33 +1270,31 @@ describe("postgres integrations", () => { }) it("should be able to change the table to allow nullable and refetch this", async () => { - const response = await makeRequest( - "post", - `/api/datasources/${datasource._id}/schema` - ) - const entities = response.body.datasource.entities + const response = await config.api.datasource.fetchSchema({ + datasourceId: datasource._id!, + }) + const entities = response.datasource.entities expect(entities).toBeDefined() - const nullableTable = entities["nullabletable"] + const nullableTable = entities?.["nullabletable"] expect(nullableTable).toBeDefined() - expect(nullableTable.schema["order_number"].constraints.presence).toEqual( - true - ) + expect( + nullableTable?.schema["order_number"].constraints?.presence + ).toEqual(true) await rawQuery( rawDatasource, `ALTER TABLE nullableTable ALTER COLUMN order_number DROP NOT NULL; ` ) - const responseAfter = await makeRequest( - "post", - `/api/datasources/${datasource._id}/schema` - ) - const entitiesAfter = responseAfter.body.datasource.entities + const responseAfter = await config.api.datasource.fetchSchema({ + datasourceId: datasource._id!, + }) + const entitiesAfter = responseAfter.datasource.entities expect(entitiesAfter).toBeDefined() - const nullableTableAfter = entitiesAfter["nullabletable"] + const nullableTableAfter = entitiesAfter?.["nullabletable"] expect(nullableTableAfter).toBeDefined() expect( - nullableTableAfter.schema["order_number"].constraints.presence + nullableTableAfter?.schema["order_number"].constraints?.presence ).toEqual(false) }) }) diff --git a/packages/server/src/tests/utilities/api/datasource.ts b/packages/server/src/tests/utilities/api/datasource.ts index bb4c74093c..0dd50562f9 100644 --- a/packages/server/src/tests/utilities/api/datasource.ts +++ b/packages/server/src/tests/utilities/api/datasource.ts @@ -71,11 +71,20 @@ export class DatasourceAPI extends TestAPI { }) } - fetchSchema = async (id: string, expectations?: Expectations) => { + fetchSchema = async ( + { + datasourceId, + tablesFilter, + }: { datasourceId: string; tablesFilter?: string[] }, + expectations?: Expectations + ) => { return await this._post( - `/api/datasources/${id}/schema`, + `/api/datasources/${datasourceId}/schema`, { - expectations, + expectations: expectations, + body: { + tablesFilter: tablesFilter, + }, } ) } From 14266be4e431115114d160a8956f567de4f91ee7 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 7 Jun 2024 17:26:45 +0100 Subject: [PATCH 4/6] Commenting why it does stuff. --- packages/server/src/integration-test/postgres.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/server/src/integration-test/postgres.spec.ts b/packages/server/src/integration-test/postgres.spec.ts index 99369202ec..32d74e5c45 100644 --- a/packages/server/src/integration-test/postgres.spec.ts +++ b/packages/server/src/integration-test/postgres.spec.ts @@ -1280,6 +1280,8 @@ describe("postgres integrations", () => { expect( nullableTable?.schema["order_number"].constraints?.presence ).toEqual(true) + // need to perform these calls raw to the DB so that the external state of the DB differs to what Budibase + // is aware of - therefore we can try to fetch and make sure BB updates correctly await rawQuery( rawDatasource, `ALTER TABLE nullableTable From 3cc4b71561b0d93e8417edc5e6127f346ddae9ec Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 7 Jun 2024 17:59:18 +0100 Subject: [PATCH 5/6] Fixing some issues highlighted by test case. --- .../src/api/routes/tests/datasource.spec.ts | 7 ++++++- .../server/src/integrations/utils/utils.ts | 20 ++++++++++++------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/packages/server/src/api/routes/tests/datasource.spec.ts b/packages/server/src/api/routes/tests/datasource.spec.ts index f7838b1d39..8fa84ac05d 100644 --- a/packages/server/src/api/routes/tests/datasource.spec.ts +++ b/packages/server/src/api/routes/tests/datasource.spec.ts @@ -258,11 +258,12 @@ describe("/datasources", () => { }) ) + const stringName = "string" const fullSchema: { [type in SupportedSqlTypes]: FieldSchema & { type: type } } = { [FieldType.STRING]: { - name: "string", + name: stringName, type: FieldType.STRING, constraints: { presence: true, @@ -353,6 +354,10 @@ describe("/datasources", () => { ), schema: Object.entries(table.schema).reduce( (acc, [fieldName, field]) => { + // the constraint will be unset - as the DB doesn't recognise it as not null + if (fieldName === stringName) { + field.constraints = {} + } acc[fieldName] = expect.objectContaining({ ...field, }) diff --git a/packages/server/src/integrations/utils/utils.ts b/packages/server/src/integrations/utils/utils.ts index 41e62f8975..b97782ce7e 100644 --- a/packages/server/src/integrations/utils/utils.ts +++ b/packages/server/src/integrations/utils/utils.ts @@ -289,19 +289,25 @@ function copyExistingPropsOver( externalType: existingTableSchema[key].externalType || table.schema[key]?.externalType, - autocolumn: !!fetchedColumnDefinition?.autocolumn, + autocolumn: fetchedColumnDefinition?.autocolumn, } as FieldSchema // check constraints which can be fetched from the DB (they could be updated) if (fetchedColumnDefinition?.constraints) { // inclusions are the enum values (select/options) - const fetchedInclusion = fetchedColumnDefinition.constraints.inclusion - const oldInclusion = table.schema[key].constraints?.inclusion + const fetchedConstraints = fetchedColumnDefinition.constraints + const oldConstraints = table.schema[key].constraints table.schema[key].constraints = { ...table.schema[key].constraints, - presence: !!fetchedColumnDefinition.constraints?.presence, - inclusion: fetchedInclusion?.length - ? fetchedInclusion - : oldInclusion, + inclusion: fetchedConstraints.inclusion?.length + ? fetchedConstraints.inclusion + : oldConstraints?.inclusion, + } + // true or undefined - consistent with old API + if (fetchedConstraints.presence) { + table.schema[key].constraints!.presence = + fetchedConstraints.presence + } else if (oldConstraints?.presence === true) { + delete table.schema[key].constraints?.presence } } } From 138f1d02b694ce6498967246a32c501d68698abd Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 7 Jun 2024 18:02:26 +0100 Subject: [PATCH 6/6] Hopefully final fix to test. --- packages/server/src/integration-test/postgres.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/integration-test/postgres.spec.ts b/packages/server/src/integration-test/postgres.spec.ts index 32d74e5c45..ddd3447d0d 100644 --- a/packages/server/src/integration-test/postgres.spec.ts +++ b/packages/server/src/integration-test/postgres.spec.ts @@ -1297,7 +1297,7 @@ describe("postgres integrations", () => { expect(nullableTableAfter).toBeDefined() expect( nullableTableAfter?.schema["order_number"].constraints?.presence - ).toEqual(false) + ).toBeUndefined() }) }) })