Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 21, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e5d5a74
fix(core): Fix 431 for large dynamic node parameters
ivov May 14, 2024
0127322
Merge branch 'master' into pay-997-bug-restnode-parameter-options-sen…
ivov May 17, 2024
3174770
Tests
ivov May 17, 2024
e6d3049
No need to check for `methodName` in `/options`
ivov May 21, 2024
4236530
Merge master
ivov May 21, 2024
2e65fab
Fix e2e
ivov May 21, 2024
5e2e01a
Standardize (except spy)
ivov May 21, 2024
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
85 changes: 35 additions & 50 deletions packages/cli/src/controllers/dynamicNodeParameters.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,54 +1,29 @@
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,
};
@Post('/options')
async getOptions(req: DynamicNodeParametersRequest.Options): Promise<INodePropertyOptions[]> {
Copy link
Contributor

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
Member Author

Choose a reason for hiding this comment

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

I'd love this - Val is planning to address this more generally: https://n8nio.slack.com/archives/C069HS026UF/p1715177726506449?thread_ts=1715164850.683949&cid=C069HS026UF

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

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

/** Returns parameter values which normally get loaded from an external API or get generated dynamically */
@Get('/options')
async getOptions(req: DynamicNodeParametersRequest.Options): Promise<INodePropertyOptions[]> {
const { path, methodName, loadOptions } = req.query;
const { credentials, currentNodeParameters, nodeTypeAndVersion } = req.params;
const additionalData = await getBase(req.user.id, currentNodeParameters);

if (methodName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this check still needed now that we are also checking methodName on line 25?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, please see e6d3049

Expand All @@ -75,13 +50,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 +78,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,21 +367,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
6 changes: 3 additions & 3 deletions packages/editor-ui/src/api/nodeTypes.ts
View file
Open in desktop
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