From cb8a942d672fdd52db635525cf7f647aa908bc25 Mon Sep 17 00:00:00 2001 From: Pranav C Date: Fri, 9 Aug 2024 11:52:40 +0530 Subject: [PATCH] fix: Integration delete related bugs (#9189) * fix: use knex directly to get source list since it requires custom condition Signed-off-by: Pranav C * fix: add missing root scope for MetaService Signed-off-by: Pranav C * fix: invalid integration id handling Signed-off-by: Pranav C * test: bring integration unit tests for OSS Signed-off-by: Pranav C * fix: if NcError throw as it is Signed-off-by: Pranav C * test: use read api instead of list Signed-off-by: Pranav C --------- Signed-off-by: Pranav C --- packages/nocodb-sdk/src/lib/globals.ts | 4 +- packages/nocodb/src/meta/meta.service.ts | 13 +- packages/nocodb/src/models/Integration.ts | 12 +- .../src/services/integrations.service.ts | 56 ++-- packages/nocodb/src/utils/globals.ts | 2 + packages/nocodb/tests/unit/rest/index.test.ts | 2 +- .../tests/unit/rest/tests/integration.test.ts | 243 ++++++++++++++++++ 7 files changed, 290 insertions(+), 42 deletions(-) create mode 100644 packages/nocodb/tests/unit/rest/tests/integration.test.ts diff --git a/packages/nocodb-sdk/src/lib/globals.ts b/packages/nocodb-sdk/src/lib/globals.ts index 6420c8fc62..1e248ea059 100644 --- a/packages/nocodb-sdk/src/lib/globals.ts +++ b/packages/nocodb-sdk/src/lib/globals.ts @@ -192,8 +192,8 @@ export enum NcErrorType { INVALID_PK_VALUE = 'INVALID_PK_VALUE', COLUMN_ASSOCIATED_WITH_LINK = 'COLUMN_ASSOCIATED_WITH_LINK', TABLE_ASSOCIATED_WITH_LINK = 'TABLE_ASSOCIATED_WITH_LINK', - INTEGRATION_NOT_FOUND= 'INTEGRATION_NOT_FOUND', - INTEGRATION_LINKED_WITH_BASES= 'INTEGRATION_LINKED_WITH_BASES', + INTEGRATION_NOT_FOUND = 'INTEGRATION_NOT_FOUND', + INTEGRATION_LINKED_WITH_BASES = 'INTEGRATION_LINKED_WITH_BASES', } type Roles = OrgUserRoles | ProjectRoles | WorkspaceUserRoles; diff --git a/packages/nocodb/src/meta/meta.service.ts b/packages/nocodb/src/meta/meta.service.ts index 5e5c4bd3ba..0c4b666fa1 100644 --- a/packages/nocodb/src/meta/meta.service.ts +++ b/packages/nocodb/src/meta/meta.service.ts @@ -185,7 +185,7 @@ export class MetaService { }); } } else { - if (!base_id) { + if (!base_id && base_id !== RootScopes.WORKSPACE) { NcError.metaError({ message: 'Base ID is required', sql: '', @@ -251,6 +251,7 @@ export class MetaService { [MetaTable.COMMENTS_REACTIONS]: 'cre', [MetaTable.USER_COMMENTS_NOTIFICATIONS_PREFERENCE]: 'cnp', [MetaTable.JOBS]: 'job', + [MetaTable.INTEGRATIONS]: 'int', }; const prefix = prefixMap[target] || 'nc'; @@ -297,7 +298,7 @@ export class MetaService { }); } } else { - if (!base_id) { + if (!base_id && base_id !== RootScopes.WORKSPACE) { NcError.metaError({ message: 'Base ID is required', sql: '', @@ -370,7 +371,7 @@ export class MetaService { }); } } else { - if (!base_id) { + if (!base_id && base_id !== RootScopes.WORKSPACE) { NcError.metaError({ message: 'Base ID is required', sql: '', @@ -453,7 +454,7 @@ export class MetaService { }); } } else { - if (!base_id) { + if (!base_id && base_id !== RootScopes.WORKSPACE) { NcError.metaError({ message: 'Base ID is required', sql: '', @@ -527,7 +528,7 @@ export class MetaService { }); } } else { - if (!base_id) { + if (!base_id && base_id !== RootScopes.WORKSPACE) { NcError.metaError({ message: 'Base ID is required', sql: '', @@ -588,7 +589,7 @@ export class MetaService { }); } } else { - if (!base_id) { + if (!base_id && base_id !== RootScopes.WORKSPACE) { NcError.metaError({ message: 'Base ID is required', sql: '', diff --git a/packages/nocodb/src/models/Integration.ts b/packages/nocodb/src/models/Integration.ts index dca0f3f148..74e8257fd9 100644 --- a/packages/nocodb/src/models/Integration.ts +++ b/packages/nocodb/src/models/Integration.ts @@ -237,8 +237,8 @@ export default class Integration implements IntegrationType { integration.meta = parseMetaProp(integration, 'meta'); } - const integrations = integrationList?.map((baseData) => { - return this.castType(baseData); + const integrations = integrationList?.map((integrationData) => { + return this.castType(integrationData); }); // if includeDatabaseInfo is true then get the database info for each integration @@ -280,7 +280,7 @@ export default class Integration implements IntegrationType { force = false, ncMeta = Noco.ncMeta, ): Promise { - const baseData = await ncMeta.metaGet2( + const integrationData = await ncMeta.metaGet2( context.workspace_id, context.workspace_id === RootScopes.BYPASS ? RootScopes.BYPASS @@ -308,11 +308,11 @@ export default class Integration implements IntegrationType { }, ); - if (baseData) { - baseData.meta = parseMetaProp(baseData, 'meta'); + if (integrationData) { + integrationData.meta = parseMetaProp(integrationData, 'meta'); } - return this.castType(baseData); + return this.castType(integrationData); } public async getConnectionConfig(): Promise { diff --git a/packages/nocodb/src/services/integrations.service.ts b/packages/nocodb/src/services/integrations.service.ts index 7a19259e06..8c3a1daa90 100644 --- a/packages/nocodb/src/services/integrations.service.ts +++ b/packages/nocodb/src/services/integrations.service.ts @@ -5,7 +5,7 @@ import type { NcContext, NcRequest } from '~/interface/config'; import { AppHooksService } from '~/services/app-hooks/app-hooks.service'; import { validatePayload } from '~/helpers'; import { Base, Integration } from '~/models'; -import { NcError } from '~/helpers/catchError'; +import { NcBaseError, NcError } from '~/helpers/catchError' import { Source } from '~/models'; import { CacheScope, MetaTable, RootScopes } from '~/utils/globals'; import Noco from '~/Noco'; @@ -28,6 +28,10 @@ export class IntegrationsService { ) { const integration = await Integration.get(context, param.integrationId); + if (!integration) { + NcError.integrationNotFound(param.integrationId); + } + integration.config = await integration.getConnectionConfig(); if (param.includeSources) { @@ -108,32 +112,26 @@ export class IntegrationsService { ncMeta, ); - // check linked sources - const sources = await ncMeta.metaList2( - RootScopes.WORKSPACE, - RootScopes.WORKSPACE, - MetaTable.BASES, - { - condition: { - fk_workspace_id: integration.fk_workspace_id, - fk_integration_id: integration.id, - }, - xcCondition: { - _or: [ - { - deleted: { - eq: false, - }, - }, - { - deleted: { - eq: null, - }, - }, - ], - }, - }, - ); + if (!integration) { + NcError.integrationNotFound(param.integrationId); + } + + // get linked sources + const sourceListQb = ncMeta + .knex(MetaTable.BASES) + .where({ + fk_integration_id: integration.id, + }) + .where((qb) => { + qb.where('deleted', false).orWhere('deleted', null); + }); + + if (integration.fk_workspace_id) { + sourceListQb.where('fk_workspace_id', integration.fk_workspace_id); + } + + const sources: Pick[] = + await sourceListQb.select('id', 'base_id'); if (sources.length > 0 && !param.force) { const bases = await Promise.all( @@ -176,6 +174,7 @@ export class IntegrationsService { await ncMeta.commit(); } catch (e) { await ncMeta.rollback(e); + if (e instanceof NcError || e instanceof NcBaseError) throw e; NcError.badRequest(e); } return true; @@ -187,6 +186,9 @@ export class IntegrationsService { ) { try { const integration = await Integration.get(context, param.integrationId); + if (!integration) { + NcError.integrationNotFound(param.integrationId); + } await integration.softDelete(); } catch (e) { NcError.badRequest(e); diff --git a/packages/nocodb/src/utils/globals.ts b/packages/nocodb/src/utils/globals.ts index 031c2d52ce..1d46aaa37d 100644 --- a/packages/nocodb/src/utils/globals.ts +++ b/packages/nocodb/src/utils/globals.ts @@ -290,4 +290,6 @@ export const RootScopeTables = { MetaTable.AUDIT, ], [RootScopes.BASE]: [MetaTable.PROJECT], + // It's a special case and Workspace is equivalent to org in oss + [RootScopes.WORKSPACE]: [MetaTable.INTEGRATIONS], }; diff --git a/packages/nocodb/tests/unit/rest/index.test.ts b/packages/nocodb/tests/unit/rest/index.test.ts index 679d8612d6..da5b6335bf 100644 --- a/packages/nocodb/tests/unit/rest/index.test.ts +++ b/packages/nocodb/tests/unit/rest/index.test.ts @@ -20,7 +20,7 @@ let ssoTest = () => {}; let cloudOrgTest = () => {}; let bulkAggregationTest = () => {}; let columnTest = () => {}; -let integrationTest = () => {}; +let integrationTest = require('./tests/integration.test').default; if (process.env.EE === 'true') { workspaceTest = require('./tests/ee/workspace.test').default; ssoTest = require('./tests/ee/sso.test').default; diff --git a/packages/nocodb/tests/unit/rest/tests/integration.test.ts b/packages/nocodb/tests/unit/rest/tests/integration.test.ts new file mode 100644 index 0000000000..a404bddbd5 --- /dev/null +++ b/packages/nocodb/tests/unit/rest/tests/integration.test.ts @@ -0,0 +1,243 @@ +import { expect } from 'chai' +import 'mocha' +import request from 'supertest' +import { IntegrationsType } from 'nocodb-sdk' +import { createProject } from '../../factory/base' +import init from '../../init' + +function integrationTests() { + let context + + beforeEach(async function() { + console.time('#### integrationTests') + context = await init() + await createProject(context) + console.timeEnd('#### integrationTests') + }) + + it('Create Integration', async () => { + const title = 'Sakila01' + const color = '#4351E8' + + const response = await request(context.app) + .post(`/api/v2/meta/integrations`) + .set('xc-auth', context.token) + .send({ + title, + meta: { + color, + }, + config: {}, + type: IntegrationsType.Database, + }) + .expect(201) + + expect(response.body).to.have.property('id') + expect(response.body).to.have.property('title') + expect(response.body).to.have.property('meta') + expect(response.body.meta).to.have.property('color') + + if (response.body.title !== title || response.body.meta.color !== color) { + throw new Error('Integration creation failed') + } + }) + + it('List Integrations', async () => { + const title = 'Sakila01' + const color = '#4351E8' + + await request(context.app) + .post(`/api/v2/meta/integrations`) + .set('xc-auth', context.token) + .send({ + title, + meta: { + color, + }, + config: {}, + type: IntegrationsType.Database, + }) + .expect(201) + + const response = await request(context.app) + .get(`/api/v2/meta/integrations`) + .set('xc-auth', context.token) + .expect(200) + + if ( + response.body.list[0].title !== 'Sakila01' || + response.body.list[0].meta.color !== '#4351E8' + ) { + throw new Error('Integration listing failed') + } + }) + + it('Delete Integration', async () => { + const title = 'Sakila01' + const color = '#4351E8' + + const integration = await request(context.app) + .post(`/api/v2/meta/integrations`) + .set('xc-auth', context.token) + .send({ + title, + meta: { + color, + }, + config: {}, + type: IntegrationsType.Database, + }) + .expect(201) + await request(context.app) + .delete(`/api/v2/meta/integrations/${integration.body.id}`) + .set('xc-auth', context.token) + .expect(200) + }) + + it('Update Integration', async () => { + const title = 'Sakila01' + const color = '#4351E8' + + const integration = await request(context.app) + .post(`/api/v2/meta/integrations`) + .set('xc-auth', context.token) + .send({ + title, + meta: { + color, + }, + config: {}, + type: IntegrationsType.Database, + }) + .expect(201) + + await request(context.app) + .patch(`/api/v2/meta/integrations/${integration.body.id}`) + .set('xc-auth', context.token) + .send({ title: 'Sakila02', config: {}, type: IntegrationsType.Database }) + .expect(200) + + const response = await request(context.app) + .get(`/api/v2/meta/integrations/${integration.body.id}`) + .set('xc-auth', context.token) + .expect(200) + + if (response.body.title !== 'Sakila02') { + throw new Error('Integration update failed') + } + }) + + it('Update Integration Error Test', async () => { + await request(context.app) + .patch(`/api/v2/meta/integrations/xxxxxxxxx`) + .set('xc-auth', context.token) + .send({ title: 'Sakila02', config: {}, type: IntegrationsType.Database }) + .expect(404) + }) + + it('Delete Integration Error Test', async () => { + await request(context.app) + .delete(`/api/v2/meta/integrations/xxxxxxxxx`) + .set('xc-auth', context.token) + .expect(404) + }) + + it('Create Integration Error Test', async () => { + await request(context.app) + .post(`/api/v2/meta/integrations`) + .set('xc-auth', context.token) + .send() + .expect(400) + }) + + it('Create Integration Unauthorized User Test', async () => { + const title = 'Sakila01' + const color = '#4351E8' + + await request(context.app) + .post(`/api/v2/meta/integrations`) + .send({ + title, + meta: { + color, + }, + }) + .expect(401) + }) + + it('List Integration Unauthorized User Test', async () => { + await request(context.app) + .get(`/api/v2/meta/integrations`) + .expect(401) + }) + + it('Update Integration - where multiple sources are using the integration', async () => { + const title = 'Sakila01' + const color = '#4351E8' + + const integration = await request(context.app) + .post(`/api/v2/meta/integrations`) + .set('xc-auth', context.token) + .send({ + title, + meta: { + color, + }, + config: {}, + type: IntegrationsType.Database, + }) + .expect(201) + + await request(context.app) + .patch(`/api/v2/meta/integrations/${integration.body.id}`) + .set('xc-auth', context.token) + .send({ title: 'Sakila02', config: {}, type: IntegrationsType.Database }) + .expect(200) + + const response = await request(context.app) + .get(`/api/v2/meta/integrations/${integration.body.id}`) + .set('xc-auth', context.token) + .expect(200) + + if (response.body.title !== 'Sakila02') { + throw new Error('Integration update failed') + } + }) + + it('Integration list based on user roles under workspace and base', async () => { + const title = 'Sakila01' + const color = '#4351E8' + + const integration = await request(context.app) + .post(`/api/v2/meta/integrations`) + .set('xc-auth', context.token) + .send({ + title, + meta: { + color, + }, + config: {}, + type: IntegrationsType.Database, + }) + .expect(201) + + await request(context.app) + .patch(`/api/v2/meta/integrations/${integration.body.id}`) + .set('xc-auth', context.token) + .send({ title: 'Sakila02', config: {}, type: IntegrationsType.Database }) + .expect(200) + + const response = await request(context.app) + .get(`/api/v2/meta/integrations/${integration.body.id}`) + .set('xc-auth', context.token) + .expect(200) + + if (response.body.title !== 'Sakila02') { + throw new Error('Integration update failed') + } + }) +} + +export default function() { + describe('Integration', integrationTests) +}