Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Fix 431 for large dynamic node parameters #9384

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cypress/e2e/5-ndv.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,11 @@ describe('NDV', () => {
});

it('should not retrieve remote options when a parameter value changes', () => {
cy.intercept('/rest/dynamic-node-parameters/options?**', cy.spy().as('fetchParameterOptions'));
cy.intercept(
'POST',
'/rest/dynamic-node-parameters/options',
cy.spy().as('fetchParameterOptions'),
);
workflowPage.actions.addInitialNodeToCanvas('E2e Test', { action: 'Remote Options' });
// Type something into the field
ndv.actions.typeIntoParameterInput('otherField', 'test');
Expand Down
85 changes: 34 additions & 51 deletions packages/cli/src/controllers/dynamicNodeParameters.controller.ts
Original file line number Diff line number Diff line change
@@ -1,54 +1,27 @@
import type { RequestHandler } from 'express';
import { NextFunction, Response } from 'express';
import type {
INodeListSearchResult,
INodePropertyOptions,
ResourceMapperFields,
} from 'n8n-workflow';
import type { INodePropertyOptions } from 'n8n-workflow';
import { jsonParse } from 'n8n-workflow';

import { Get, Middleware, RestController } from '@/decorators';
import { Post, RestController } from '@/decorators';
import { getBase } from '@/WorkflowExecuteAdditionalData';
import { DynamicNodeParametersService } from '@/services/dynamicNodeParameters.service';
import { DynamicNodeParametersRequest } from '@/requests';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';

const assertMethodName: RequestHandler = (req, res, next) => {
const { methodName } = req.query as DynamicNodeParametersRequest.BaseRequest['query'];
if (!methodName) {
throw new BadRequestError('Parameter methodName is required.');
}
next();
};

@RestController('/dynamic-node-parameters')
export class DynamicNodeParametersController {
constructor(private readonly service: DynamicNodeParametersService) {}

@Middleware()
parseQueryParams(req: DynamicNodeParametersRequest.BaseRequest, _: Response, next: NextFunction) {
const { credentials, currentNodeParameters, nodeTypeAndVersion } = req.query;
if (!nodeTypeAndVersion) {
throw new BadRequestError('Parameter nodeTypeAndVersion is required.');
}
if (!currentNodeParameters) {
throw new BadRequestError('Parameter currentNodeParameters is required.');
}

req.params = {
nodeTypeAndVersion: jsonParse(nodeTypeAndVersion),
currentNodeParameters: jsonParse(currentNodeParameters),
credentials: credentials ? jsonParse(credentials) : undefined,
};

next();
}

/** Returns parameter values which normally get loaded from an external API or get generated dynamically */
@Get('/options')
@Post('/options')
async getOptions(req: DynamicNodeParametersRequest.Options): Promise<INodePropertyOptions[]> {
const { path, methodName, loadOptions } = req.query;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we consider converting DynamicNodeParametersRequest.Options (and other request types in this controller) to a validatable class, to ensure that properties like nodeTypeAndVersion and currentNodeParameters are actually sent and valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const { credentials, currentNodeParameters, nodeTypeAndVersion } = req.params;
const {
credentials,
currentNodeParameters,
nodeTypeAndVersion,
path,
methodName,
loadOptions,
} = req.body;

const additionalData = await getBase(req.user.id, currentNodeParameters);

if (methodName) {
Expand All @@ -75,13 +48,22 @@ export class DynamicNodeParametersController {
return [];
}

@Get('/resource-locator-results', { middlewares: [assertMethodName] })
async getResourceLocatorResults(
req: DynamicNodeParametersRequest.ResourceLocatorResults,
): Promise<INodeListSearchResult | undefined> {
const { path, methodName, filter, paginationToken } = req.query;
const { credentials, currentNodeParameters, nodeTypeAndVersion } = req.params;
@Post('/resource-locator-results')
async getResourceLocatorResults(req: DynamicNodeParametersRequest.ResourceLocatorResults) {
const {
path,
methodName,
filter,
paginationToken,
credentials,
currentNodeParameters,
nodeTypeAndVersion,
} = req.body;

if (!methodName) throw new BadRequestError('Missing `methodName` in request body');

const additionalData = await getBase(req.user.id, currentNodeParameters);

return await this.service.getResourceLocatorResults(
methodName,
path,
Expand All @@ -94,13 +76,14 @@ export class DynamicNodeParametersController {
);
}

@Get('/resource-mapper-fields', { middlewares: [assertMethodName] })
async getResourceMappingFields(
req: DynamicNodeParametersRequest.ResourceMapperFields,
): Promise<ResourceMapperFields | undefined> {
const { path, methodName } = req.query;
const { credentials, currentNodeParameters, nodeTypeAndVersion } = req.params;
@Post('/resource-mapper-fields')
async getResourceMappingFields(req: DynamicNodeParametersRequest.ResourceMapperFields) {
const { path, methodName, credentials, currentNodeParameters, nodeTypeAndVersion } = req.body;

if (!methodName) throw new BadRequestError('Missing `methodName` in request body');

const additionalData = await getBase(req.user.id, currentNodeParameters);

return await this.service.getResourceMappingFields(
methodName,
path,
Expand Down
16 changes: 6 additions & 10 deletions packages/cli/src/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,21 +393,17 @@ export declare namespace OAuthRequest {
// /dynamic-node-parameters
// ----------------------------------
export declare namespace DynamicNodeParametersRequest {
type BaseRequest<QueryParams = {}> = AuthenticatedRequest<
{
nodeTypeAndVersion: INodeTypeNameVersion;
currentNodeParameters: INodeParameters;
credentials?: INodeCredentials;
},
type BaseRequest<RequestBody = {}> = AuthenticatedRequest<
{},
{},
{
path: string;
nodeTypeAndVersion: string;
currentNodeParameters: string;
nodeTypeAndVersion: INodeTypeNameVersion;
currentNodeParameters: INodeParameters;
methodName?: string;
credentials?: string;
} & QueryParams
credentials?: INodeCredentials;
} & RequestBody,
{}
>;

/** GET /dynamic-node-parameters/options */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import type { SuperTest, Test } from 'supertest';
import { createOwner } from '../shared/db/users';
import { setupTestServer } from '../shared/utils';
import * as AdditionalData from '@/WorkflowExecuteAdditionalData';
import type {
INodeListSearchResult,
IWorkflowExecuteAdditionalData,
ResourceMapperFields,
} from 'n8n-workflow';
import { mock } from 'jest-mock-extended';
import { DynamicNodeParametersService } from '@/services/dynamicNodeParameters.service';

describe('DynamicNodeParametersController', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about adding unit tests instead, since there are plenty of logical checks in the controller itself.
but, I can help with that bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this fix is about switching to POST and body hence I thought of integration tests.

const testServer = setupTestServer({ endpointGroups: ['dynamic-node-parameters'] });
let ownerAgent: SuperTest<Test>;

beforeAll(async () => {
const owner = await createOwner();
ownerAgent = testServer.authAgentFor(owner);
});

const commonRequestParams = {
credentials: {},
currentNodeParameters: {},
nodeTypeAndVersion: {},
path: 'path',
methodName: 'methodName',
};

describe('POST /dynamic-node-parameters/options', () => {
jest.spyOn(AdditionalData, 'getBase').mockResolvedValue(mock<IWorkflowExecuteAdditionalData>());

it('should take params via body', async () => {
jest
.spyOn(DynamicNodeParametersService.prototype, 'getOptionsViaMethodName')
.mockResolvedValue([]);

await ownerAgent
.post('/dynamic-node-parameters/options')
.send({
...commonRequestParams,
loadOptions: 'loadOptions',
})
.expect(200);
});
});

describe('POST /dynamic-node-parameters/resource-locator-results', () => {
it('should take params via body', async () => {
jest
.spyOn(DynamicNodeParametersService.prototype, 'getResourceLocatorResults')
.mockResolvedValue(mock<INodeListSearchResult>());

await ownerAgent
.post('/dynamic-node-parameters/resource-locator-results')
.send({
...commonRequestParams,
filter: 'filter',
paginationToken: 'paginationToken',
})
.expect(200);
});
});

describe('POST /dynamic-node-parameters/resource-mapper-fields', () => {
it('should take params via body', async () => {
jest
.spyOn(DynamicNodeParametersService.prototype, 'getResourceMappingFields')
.mockResolvedValue(mock<ResourceMapperFields>());

await ownerAgent
.post('/dynamic-node-parameters/resource-mapper-fields')
.send({
...commonRequestParams,
loadOptions: 'loadOptions',
})
.expect(200);
});
});
});
3 changes: 2 additions & 1 deletion packages/cli/test/integration/shared/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ type EndpointGroup =
| 'invitations'
| 'debug'
| 'project'
| 'role';
| 'role'
| 'dynamic-node-parameters';

export interface SetupProps {
endpointGroups?: EndpointGroup[];
Expand Down
7 changes: 7 additions & 0 deletions packages/cli/test/integration/shared/utils/testServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,13 @@ export const setupTestServer = ({
const { RoleController } = await import('@/controllers/role.controller');
registerController(app, RoleController);
break;

case 'dynamic-node-parameters':
const { DynamicNodeParametersController } = await import(
'@/controllers/dynamicNodeParameters.controller'
);
registerController(app, DynamicNodeParametersController);
break;
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions packages/editor-ui/src/api/nodeTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export async function getNodeParameterOptions(
context: IRestApiContext,
sendData: DynamicNodeParameters.OptionsRequest,
): Promise<INodePropertyOptions[]> {
return await makeRestApiRequest(context, 'GET', '/dynamic-node-parameters/options', sendData);
return await makeRestApiRequest(context, 'POST', '/dynamic-node-parameters/options', sendData);
}

export async function getResourceLocatorResults(
Expand All @@ -40,7 +40,7 @@ export async function getResourceLocatorResults(
): Promise<INodeListSearchResult> {
return await makeRestApiRequest(
context,
'GET',
'POST',
'/dynamic-node-parameters/resource-locator-results',
sendData,
);
Expand All @@ -52,7 +52,7 @@ export async function getResourceMapperFields(
): Promise<ResourceMapperFields> {
return await makeRestApiRequest(
context,
'GET',
'POST',
'/dynamic-node-parameters/resource-mapper-fields',
sendData,
);
Expand Down
Loading