From 5bddbedf2eb5b6363e24e20dccfea267b80001fb Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Thu, 9 Oct 2025 15:33:26 +0200 Subject: [PATCH] feat(API): Add project and projectId fields to get and update a variable project (#20544) --- .../variables/variables.service.ee.ts | 7 +- .../variables/spec/paths/variables.id.yml | 2 +- .../variables/spec/paths/variables.yml | 16 ++- .../spec/schemas/variable.create.yml | 21 ++++ .../variables/spec/schemas/variable.yml | 2 + .../handlers/variables/variables.handler.ts | 17 +-- packages/cli/src/requests.ts | 14 ++- .../integration/public-api/variables.test.ts | 115 ++++++++++++++++-- .../test/integration/shared/db/variables.ts | 5 +- .../cli/test/integration/variables.test.ts | 20 ++- 10 files changed, 195 insertions(+), 24 deletions(-) create mode 100644 packages/cli/src/public-api/v1/handlers/variables/spec/schemas/variable.create.yml diff --git a/packages/cli/src/environments.ee/variables/variables.service.ee.ts b/packages/cli/src/environments.ee/variables/variables.service.ee.ts index 0961f15800d..051a73455ca 100644 --- a/packages/cli/src/environments.ee/variables/variables.service.ee.ts +++ b/packages/cli/src/environments.ee/variables/variables.service.ee.ts @@ -261,7 +261,12 @@ export class VariablesService { await this.variablesRepository.update(id, { key: variable.key, value: variable.value, - project: variable.projectId ? { id: variable.projectId } : null, + // Only update the project if it was explicitly set in the update + // If project id is undefined, keep the existing + // If project id is null, move to global (no project) + ...(typeof variable.projectId !== 'undefined' + ? { project: variable.projectId ? { id: variable.projectId } : null } + : {}), }); await this.updateCache(); return (await this.getCached(id))!; diff --git a/packages/cli/src/public-api/v1/handlers/variables/spec/paths/variables.id.yml b/packages/cli/src/public-api/v1/handlers/variables/spec/paths/variables.id.yml index bbc26094d6b..46edf8ee25a 100644 --- a/packages/cli/src/public-api/v1/handlers/variables/spec/paths/variables.id.yml +++ b/packages/cli/src/public-api/v1/handlers/variables/spec/paths/variables.id.yml @@ -29,7 +29,7 @@ put: content: application/json: schema: - $ref: '../schemas/variable.yml' + $ref: '../schemas/variable.create.yml' required: true responses: '204': diff --git a/packages/cli/src/public-api/v1/handlers/variables/spec/paths/variables.yml b/packages/cli/src/public-api/v1/handlers/variables/spec/paths/variables.yml index 7418c2fe057..1d75ee5211f 100644 --- a/packages/cli/src/public-api/v1/handlers/variables/spec/paths/variables.yml +++ b/packages/cli/src/public-api/v1/handlers/variables/spec/paths/variables.yml @@ -10,7 +10,7 @@ post: content: application/json: schema: - $ref: '../schemas/variable.yml' + $ref: '../schemas/variable.create.yml' required: true responses: '201': @@ -29,6 +29,20 @@ get: parameters: - $ref: '../../../../shared/spec/parameters/limit.yml' - $ref: '../../../../shared/spec/parameters/cursor.yml' + - name: projectId + in: query + required: false + explode: false + allowReserved: true + schema: + type: string + example: VmwOO9HeTEj20kxM + - name: state + in: query + required: false + schema: + type: string + enum: [empty] responses: '200': description: Operation successful. diff --git a/packages/cli/src/public-api/v1/handlers/variables/spec/schemas/variable.create.yml b/packages/cli/src/public-api/v1/handlers/variables/spec/schemas/variable.create.yml new file mode 100644 index 00000000000..05c37437dfb --- /dev/null +++ b/packages/cli/src/public-api/v1/handlers/variables/spec/schemas/variable.create.yml @@ -0,0 +1,21 @@ +type: object +additionalProperties: false +required: + - key + - value +properties: + id: + type: string + readOnly: true + key: + type: string + value: + type: string + example: test + type: + type: string + readOnly: true + projectId: + type: string + example: VmwOO9HeTEj20kxM + nullable: true diff --git a/packages/cli/src/public-api/v1/handlers/variables/spec/schemas/variable.yml b/packages/cli/src/public-api/v1/handlers/variables/spec/schemas/variable.yml index 319ad8d4403..283f62ce534 100644 --- a/packages/cli/src/public-api/v1/handlers/variables/spec/schemas/variable.yml +++ b/packages/cli/src/public-api/v1/handlers/variables/spec/schemas/variable.yml @@ -15,3 +15,5 @@ properties: type: type: string readOnly: true + project: + $ref: '../../../projects/spec/schemas/project.yml' diff --git a/packages/cli/src/public-api/v1/handlers/variables/variables.handler.ts b/packages/cli/src/public-api/v1/handlers/variables/variables.handler.ts index 08be7fe72a9..d5fdc681d3e 100644 --- a/packages/cli/src/public-api/v1/handlers/variables/variables.handler.ts +++ b/packages/cli/src/public-api/v1/handlers/variables/variables.handler.ts @@ -2,6 +2,8 @@ import { CreateVariableRequestDto } from '@n8n/api-types'; import type { AuthenticatedRequest } from '@n8n/db'; import { VariablesRepository } from '@n8n/db'; import { Container } from '@n8n/di'; +// eslint-disable-next-line n8n-local-rules/misplaced-n8n-typeorm-import +import { IsNull } from '@n8n/typeorm'; import type { Response } from 'express'; import { @@ -12,12 +14,8 @@ import { import { encodeNextCursor } from '../../shared/services/pagination.service'; import { VariablesController } from '@/environments.ee/variables/variables.controller.ee'; -import type { PaginatedRequest } from '@/public-api/types'; import type { VariablesRequest } from '@/requests'; -type Delete = VariablesRequest.Delete; -type GetAll = PaginatedRequest; - export = { createVariable: [ isLicensed('feat:variables'), @@ -48,7 +46,7 @@ export = { deleteVariable: [ isLicensed('feat:variables'), apiKeyHasScopeWithGlobalScopeFallback({ scope: 'variable:delete' }), - async (req: Delete, res: Response) => { + async (req: AuthenticatedRequest<{ id: string }>, res: Response) => { await Container.get(VariablesController).deleteVariable(req); return res.status(204).send(); @@ -58,12 +56,17 @@ export = { isLicensed('feat:variables'), apiKeyHasScopeWithGlobalScopeFallback({ scope: 'variable:list' }), validCursor, - async (req: GetAll, res: Response) => { - const { offset = 0, limit = 100 } = req.query; + async (req: VariablesRequest.GetAll, res: Response) => { + const { offset = 0, limit = 100, projectId, state } = req.query; const [variables, count] = await Container.get(VariablesRepository).findAndCount({ skip: offset, take: limit, + where: { + project: projectId === 'null' ? IsNull() : { id: projectId }, + value: state === 'empty' ? '' : undefined, + }, + relations: ['project'], }); return res.json({ diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index dc35e87898a..275ded8d284 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -235,7 +235,19 @@ export declare namespace LicenseRequest { export declare namespace VariablesRequest { type CreateUpdatePayload = Omit & { id?: unknown }; - type GetAll = AuthenticatedRequest; + type GetAll = AuthenticatedRequest< + {}, + {}, + {}, + { + limit?: number; + cursor?: string; + offset?: number; + lastId?: string; + projectId?: string; + state?: 'empty'; + } + >; type Get = AuthenticatedRequest<{ id: string }, {}, {}, {}>; type Create = AuthenticatedRequest<{}, {}, CreateUpdatePayload, {}>; type Update = AuthenticatedRequest<{ id: string }, {}, CreateUpdatePayload, {}>; diff --git a/packages/cli/test/integration/public-api/variables.test.ts b/packages/cli/test/integration/public-api/variables.test.ts index fa9317b53b8..a62360af72b 100644 --- a/packages/cli/test/integration/public-api/variables.test.ts +++ b/packages/cli/test/integration/public-api/variables.test.ts @@ -1,13 +1,18 @@ -import { testDb } from '@n8n/backend-test-utils'; -import type { User, Variables } from '@n8n/db'; +import { createTeamProject, testDb } from '@n8n/backend-test-utils'; +import type { Project, User, Variables } from '@n8n/db'; +import { createOwnerWithApiKey } from '@test-integration/db/users'; +import { + createProjectVariable, + createVariable, + getVariableByIdOrFail, +} from '@test-integration/db/variables'; +import { setupTestServer } from '@test-integration/utils'; import { FeatureNotLicensedError } from '@/errors/feature-not-licensed.error'; -import { createOwnerWithApiKey } from '@test-integration/db/users'; -import { createVariable, getVariableByIdOrFail } from '@test-integration/db/variables'; -import { setupTestServer } from '@test-integration/utils'; describe('Variables in Public API', () => { let owner: User; + let project: Project; const testServer = setupTestServer({ endpointGroups: ['publicApi'] }); const licenseErrorMessage = new FeatureNotLicensedError('feat:variables').message; @@ -19,6 +24,7 @@ describe('Variables in Public API', () => { await testDb.truncate(['Variables', 'User']); owner = await createOwnerWithApiKey(); + project = await createTeamProject(); }); describe('GET /variables', () => { @@ -27,7 +33,12 @@ describe('Variables in Public API', () => { * Arrange */ testServer.license.enable('feat:variables'); - const variables = await Promise.all([createVariable(), createVariable(), createVariable()]); + const variables = await Promise.all([ + createVariable(), + createVariable(), + createVariable(), + createProjectVariable('projectKey', 'projectValue', project), + ]); /** * Act @@ -43,11 +54,55 @@ describe('Variables in Public API', () => { expect(Array.isArray(response.body.data)).toBe(true); expect(response.body.data.length).toBe(variables.length); - variables.forEach(({ id, key, value }) => { + variables.forEach(({ id, key, value, project }) => { expect(response.body.data).toContainEqual(expect.objectContaining({ id, key, value })); + if (project) { + const projectResponse = response.body.data.find((v: Variables) => v.id === id).project; + expect(projectResponse).toBeDefined(); + expect(projectResponse).toEqual( + expect.objectContaining({ id: project.id, name: project.name }), + ); + } }); }); + it('if licensed, should be able to filter variables by projectId and state', async () => { + /** + * Arrange + */ + testServer.license.enable('feat:variables'); + await Promise.all([ + createVariable(), + createProjectVariable('projectKey', 'projectValue', project), + createProjectVariable('emptyVar', '', project), + createVariable('emptyVar', ''), + ]); + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .get('/variables') + .query({ projectId: project.id, state: 'empty' }); + + /** + * Assert + */ + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('data'); + expect(response.body).toHaveProperty('nextCursor'); + expect(Array.isArray(response.body.data)).toBe(true); + expect(response.body.data.length).toBe(1); + expect(response.body.data[0]).toEqual( + expect.objectContaining({ + key: 'emptyVar', + value: '', + project: expect.objectContaining({ id: project.id }), + }), + ); + }); + it('if not licensed, should reject', async () => { /** * Act @@ -87,6 +142,34 @@ describe('Variables in Public API', () => { ); }); + it('if licensed, should create a variable linked to a project', async () => { + /** + * Arrange + */ + testServer.license.enable('feat:variables'); + const variablePayload = { key: 'key', value: 'value', projectId: project.id }; + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .post('/variables') + .send(variablePayload); + + /** + * Assert + */ + expect(response.status).toBe(201); + await expect(getVariableByIdOrFail(response.body.id)).resolves.toEqual( + expect.objectContaining({ + key: 'key', + value: 'value', + project: expect.objectContaining({ id: project.id }), + }), + ); + }); + it('if not licensed, should reject', async () => { /** * Arrange @@ -129,6 +212,24 @@ describe('Variables in Public API', () => { expect(updatedVariable).toEqual(expect.objectContaining(variablePayload)); }); + it('if licensed, should update a variable to link it to a project', async () => { + testServer.license.enable('feat:variables'); + + const response = await testServer + .publicApiAgentFor(owner) + .put(`/variables/${variable.id}`) + .send({ ...variablePayload, projectId: project.id }); + + expect(response.status).toBe(204); + const updatedVariable = await getVariableByIdOrFail(variable.id); + expect(updatedVariable).toEqual( + expect.objectContaining({ + ...variablePayload, + project: expect.objectContaining({ id: project.id }), + }), + ); + }); + it('if not licensed, should reject', async () => { const response = await testServer .publicApiAgentFor(owner) diff --git a/packages/cli/test/integration/shared/db/variables.ts b/packages/cli/test/integration/shared/db/variables.ts index ed0fc1b50b6..aaaa96beb24 100644 --- a/packages/cli/test/integration/shared/db/variables.ts +++ b/packages/cli/test/integration/shared/db/variables.ts @@ -32,7 +32,10 @@ export async function createProjectVariable( } export async function getVariableByIdOrFail(id: string) { - return await Container.get(VariablesRepository).findOneOrFail({ where: { id } }); + return await Container.get(VariablesRepository).findOneOrFail({ + where: { id }, + relations: ['project'], + }); } export async function getVariableByKey(key: string) { diff --git a/packages/cli/test/integration/variables.test.ts b/packages/cli/test/integration/variables.test.ts index 6c4b54942f5..28bed4552e9 100644 --- a/packages/cli/test/integration/variables.test.ts +++ b/packages/cli/test/integration/variables.test.ts @@ -1,9 +1,14 @@ -import { testDb } from '@n8n/backend-test-utils'; -import type { Variables } from '@n8n/db'; +import { createTeamProject, linkUserToProject, testDb } from '@n8n/backend-test-utils'; +import type { Project, Variables } from '@n8n/db'; import { Container } from '@n8n/di'; import { CacheService } from '@/services/cache/cache.service'; -import { createVariable, getVariableById, getVariableByKey } from '@test-integration/db/variables'; +import { + createProjectVariable, + createVariable, + getVariableById, + getVariableByKey, +} from '@test-integration/db/variables'; import { createOwner, createUser } from './shared/db/users'; import type { SuperAgentTest } from './shared/types'; @@ -11,6 +16,7 @@ import * as utils from './shared/utils/'; let authOwnerAgent: SuperAgentTest; let authMemberAgent: SuperAgentTest; +let project: Project; const testServer = utils.setupTestServer({ endpointGroups: ['variables'] }); const license = testServer.license; @@ -21,6 +27,9 @@ beforeAll(async () => { const member = await createUser(); authMemberAgent = testServer.authAgentFor(member); + project = await createTeamProject(); + await linkUserToProject(member, project, 'project:editor'); + license.setDefaults({ features: ['feat:variables'], // quota: { @@ -42,6 +51,7 @@ describe('GET /variables', () => { createVariable('test1', 'value1'), createVariable('test2', 'value2'), createVariable('empty', ''), + createProjectVariable('testProject1', 'projectValue1', project), ]); }); @@ -57,13 +67,13 @@ describe('GET /variables', () => { test('should return all variables for an owner', async () => { const response = await authOwnerAgent.get('/variables'); expect(response.statusCode).toBe(200); - expect(response.body.data.length).toBe(3); + expect(response.body.data.length).toBe(4); }); test('should return all variables for a member', async () => { const response = await authMemberAgent.get('/variables'); expect(response.statusCode).toBe(200); - expect(response.body.data.length).toBe(3); + expect(response.body.data.length).toBe(4); }); describe('state:empty', () => {