fix(ERPNext Node): Return each row once and tolerate empty Get Many options (#36860)

packages/nodes-base/nodes/ERPNext/ERPNext.node.ts

@@ -117,7 +117,6 @@ export class ERPNext implements INodeType {
 		let responseData;
 
 		const body: IDataObject = {};
-		const qs: IDataObject = {};
 
 		const resource = this.getNodeParameter('resource', 0);
 		const operation = this.getNodeParameter('operation', 0);
@@ -158,16 +157,17 @@ export class ERPNext implements INodeType {
 
 					const docType = this.getNodeParameter('docType', i) as string;
 					const endpoint = `/api/resource/${docType}`;
+					const qs: IDataObject = {};
 
 					const { fields, filters } = this.getNodeParameter('options', i) as {
-						fields: string[];
-						filters: {
-							customProperty: Array<{ field: string; operator: string; value: string }>;
+						fields?: string[];
+						filters?: {
+							customProperty?: Array<{ field: string; operator: string; value: string }>;
 						};
 					};
 
 					// fields=["test", "example", "hi"]
-					if (fields) {
+					if (fields?.length) {
 						if (fields.includes('*')) {
 							qs.fields = JSON.stringify(['*']);
 						} else {
@@ -175,8 +175,7 @@ export class ERPNext implements INodeType {
 						}
 					}
 					// filters=[["Person","first_name","=","Jane"]]
-					// TODO: filters not working
-					if (filters) {
+					if (filters?.customProperty?.length) {
 						qs.filters = JSON.stringify(
 							filters.customProperty.map((filter) => {
 								return [docType, filter.field, toSQL(filter.operator), filter.value];

packages/nodes-base/nodes/ERPNext/GenericFunctions.ts

@@ -82,7 +82,7 @@ export async function erpNextApiRequestAllItems(
 	do {
 		responseData = await erpNextApiRequest.call(this, method, resource, body, query);
 		returnData.push.apply(returnData, responseData[propertyName] as IDataObject[]);
-		query.limit_start += query.limit_page_length - 1;
+		query.limit_start += query.limit_page_length;
 	} while (responseData.data && responseData.data.length > 0);
 
 	return returnData;

packages/nodes-base/nodes/ERPNext/test/ERPNext.node.test.ts

@@ -0,0 +1,137 @@
+import type { IDataObject, IExecuteFunctions, INodeExecutionData } from 'n8n-workflow';
+import type { Mock } from 'vitest';
+import { mockDeep } from 'vitest-mock-extended';
+
+import { ERPNext } from '../ERPNext.node';
+import * as GenericFunctions from '../GenericFunctions';
+
+describe('ERPNext > document: getAll', () => {
+	let node: ERPNext;
+	let mockExecuteFunctions: ReturnType<typeof mockDeep<IExecuteFunctions>>;
+
+	const mockParameters = (options: IDataObject | ((itemIndex: number) => IDataObject)) => {
+		mockExecuteFunctions.getNodeParameter.mockImplementation((parameter, itemIndex) => {
+			switch (parameter) {
+				case 'resource':
+					return 'document';
+				case 'operation':
+					return 'getAll';
+				case 'docType':
+					return 'Customer';
+				case 'options':
+					return typeof options === 'function' ? options(itemIndex) : options;
+				case 'returnAll':
+					return false;
+				case 'limit':
+					return 10;
+				default:
+					return undefined;
+			}
+		});
+	};
+
+	beforeEach(() => {
+		node = new ERPNext();
+		mockExecuteFunctions = mockDeep<IExecuteFunctions>();
+		mockExecuteFunctions.getInputData.mockReturnValue([{ json: {} }]);
+		(mockExecuteFunctions.helpers.returnJsonArray as Mock).mockImplementation(
+			(data: IDataObject | IDataObject[]) =>
+				(Array.isArray(data) ? data : [data]).map((json) => ({ json })),
+		);
+		(mockExecuteFunctions.helpers.constructExecutionMetaData as Mock).mockImplementation(
+			(data: INodeExecutionData[]) => data,
+		);
+		vi.spyOn(GenericFunctions, 'erpNextApiRequest').mockResolvedValue({
+			data: [{ name: 'Customer A' }],
+		});
+	});
+
+	afterEach(() => {
+		vi.restoreAllMocks();
+	});
+
+	it('should not send filters when the Filters collection is left empty', async () => {
+		mockParameters({ fields: ['name'], filters: {} });
+
+		const result = await node.execute.call(mockExecuteFunctions);
+
+		expect(result[0]).toEqual([{ json: { name: 'Customer A' } }]);
+		expect(GenericFunctions.erpNextApiRequest).toHaveBeenCalledWith(
+			'GET',
+			'/api/resource/Customer',
+			{},
+			{ fields: JSON.stringify(['name']), limit_page_length: 10, limit_start: 0 },
+		);
+	});
+
+	it('should not send fields or filters when both collections are left empty', async () => {
+		mockParameters({ fields: [], filters: { customProperty: [] } });
+
+		await node.execute.call(mockExecuteFunctions);
+
+		expect(GenericFunctions.erpNextApiRequest).toHaveBeenCalledWith(
+			'GET',
+			'/api/resource/Customer',
+			{},
+			{ limit_page_length: 10, limit_start: 0 },
+		);
+	});
+
+	it('should not carry query options over from a previous item', async () => {
+		mockExecuteFunctions.getInputData.mockReturnValue([{ json: {} }, { json: {} }]);
+		mockParameters((itemIndex) => (itemIndex === 0 ? { fields: ['name'] } : {}));
+
+		await node.execute.call(mockExecuteFunctions);
+
+		expect(GenericFunctions.erpNextApiRequest).toHaveBeenNthCalledWith(
+			1,
+			'GET',
+			'/api/resource/Customer',
+			{},
+			{ fields: JSON.stringify(['name']), limit_page_length: 10, limit_start: 0 },
+		);
+		expect(GenericFunctions.erpNextApiRequest).toHaveBeenNthCalledWith(
+			2,
+			'GET',
+			'/api/resource/Customer',
+			{},
+			{ limit_page_length: 10, limit_start: 0 },
+		);
+	});
+
+	it('should not carry filters over from a previous item', async () => {
+		mockExecuteFunctions.getInputData.mockReturnValue([{ json: {} }, { json: {} }]);
+		mockParameters((itemIndex) =>
+			itemIndex === 0
+				? { filters: { customProperty: [{ field: 'first_name', operator: 'is', value: 'Jane' }] } }
+				: {},
+		);
+
+		await node.execute.call(mockExecuteFunctions);
+
+		expect(GenericFunctions.erpNextApiRequest).toHaveBeenNthCalledWith(
+			2,
+			'GET',
+			'/api/resource/Customer',
+			{},
+			{ limit_page_length: 10, limit_start: 0 },
+		);
+	});
+
+	it('should send filters when the Filters collection has entries', async () => {
+		mockParameters({
+			filters: { customProperty: [{ field: 'first_name', operator: 'is', value: 'Jane' }] },
+		});
+
+		await node.execute.call(mockExecuteFunctions);
+
+		expect(GenericFunctions.erpNextApiRequest).toHaveBeenCalledWith(
+			'GET',
+			'/api/resource/Customer',
+			{},
+			expect.objectContaining({
+				filters: JSON.stringify([['Customer', 'first_name', '=', 'Jane']]),
+			}),
+		);
+	});
+});

packages/nodes-base/nodes/ERPNext/test/GenericFunctions.test.ts

@@ -0,0 +1,49 @@
+import type { IExecuteFunctions, IDataObject } from 'n8n-workflow';
+import { mockDeep } from 'vitest-mock-extended';
+
+import { erpNextApiRequestAllItems } from '../GenericFunctions';
+
+describe('ERPNext > GenericFunctions', () => {
+	describe('erpNextApiRequestAllItems', () => {
+		let mockExecuteFunctions: ReturnType<typeof mockDeep<IExecuteFunctions>>;
+
+		beforeEach(() => {
+			mockExecuteFunctions = mockDeep<IExecuteFunctions>();
+			mockExecuteFunctions.getCredentials.mockResolvedValue({
+				environment: 'selfHosted',
+				domain: 'https://erpnext.local',
+			});
+		});
+
+		it('should advance the offset by the page size and return every record once', async () => {
+			const allRecords: IDataObject[] = Array.from({ length: 1001 }, (_, index) => ({
+				name: `doc-${index}`,
+			}));
+			const requestedOffsets: number[] = [];
+
+			mockExecuteFunctions.helpers.requestWithAuthentication.mockImplementation(
+				async (_credentialsType, options) => {
+					const { limit_start: offset, limit_page_length: pageSize } = options.qs as {
+						limit_start: number;
+						limit_page_length: number;
+					};
+					requestedOffsets.push(offset);
+
+					return await Promise.resolve({ data: allRecords.slice(offset, offset + pageSize) });
+				},
+			);
+
+			const result = (await erpNextApiRequestAllItems.call(
+				mockExecuteFunctions,
+				'data',
+				'GET',
+				'/api/resource/Customer',
+				{},
+			)) as IDataObject[];
+
+			expect(requestedOffsets).toEqual([0, 1000, 2000]);
+			expect(result).toHaveLength(1001);
+			expect(new Set(result.map(({ name }) => name)).size).toBe(1001);
+		});
+	});
+});