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

refactor(core): Create controller for binary data (no-changelog) #7363

Merged
merged 8 commits into from
Oct 6, 2023
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
7 changes: 6 additions & 1 deletion packages/cli/src/ResponseHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { Request, Response } from 'express';
import { parse, stringify } from 'flatted';
import picocolors from 'picocolors';
import { ErrorReporterProxy as ErrorReporter, NodeApiError } from 'n8n-workflow';

import { Readable } from 'node:stream';
import type {
IExecutionDb,
IExecutionFlatted,
Expand Down Expand Up @@ -101,6 +101,11 @@ export function sendSuccessResponse(
res.header(responseHeader);
}

if (data instanceof Readable) {
data.pipe(res);
return;
}

if (raw === true) {
if (typeof data === 'string') {
res.send(data);
Expand Down
52 changes: 2 additions & 50 deletions packages/cli/src/Server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,11 @@ import type { RequestOptions } from 'oauth-1.0a';
import clientOAuth1 from 'oauth-1.0a';

import {
BinaryDataService,
Credentials,
LoadMappingOptions,
LoadNodeParameterOptions,
LoadNodeListSearch,
UserSettings,
FileNotFoundError,
} from 'n8n-core';

import type {
Expand Down Expand Up @@ -74,7 +72,6 @@ import {
import { credentialsController } from '@/credentials/credentials.controller';
import { oauth2CredentialController } from '@/credentials/oauth2Credential.api';
import type {
BinaryDataRequest,
CurlHelper,
ExecutionRequest,
NodeListSearchRequest,
Expand All @@ -99,6 +96,7 @@ import {
WorkflowStatisticsController,
} from '@/controllers';

import { BinaryDataController } from './controllers/binaryData.controller';
import { ExternalSecretsController } from '@/ExternalSecrets/ExternalSecrets.controller.ee';
import { executionsController } from '@/executions/executions.controller';
import { isApiEnabled, loadPublicApiVersions } from '@/PublicApi';
Expand Down Expand Up @@ -208,8 +206,6 @@ export class Server extends AbstractServer {

push: Push;

binaryDataService: BinaryDataService;

constructor() {
super('main');

Expand Down Expand Up @@ -374,7 +370,6 @@ export class Server extends AbstractServer {
this.endpointPresetCredentials = config.getEnv('credentials.overwrite.endpoint');

this.push = Container.get(Push);
this.binaryDataService = Container.get(BinaryDataService);

await super.start();
LoggerProxy.debug(`Server ID: ${this.uniqueInstanceId}`);
Expand Down Expand Up @@ -581,6 +576,7 @@ export class Server extends AbstractServer {
Container.get(ExternalSecretsController),
Container.get(OrchestrationController),
Container.get(WorkflowHistoryController),
Container.get(BinaryDataController),
];

if (isLdapEnabled()) {
Expand Down Expand Up @@ -1442,50 +1438,6 @@ export class Server extends AbstractServer {
}),
);

// ----------------------------------------
// Binary data
// ----------------------------------------

// View or download binary file
this.app.get(
`/${this.restEndpoint}/data`,
async (req: BinaryDataRequest, res: express.Response): Promise<void> => {
const { id: binaryDataId, action } = req.query;
let { fileName, mimeType } = req.query;
const [mode] = binaryDataId.split(':') as ['filesystem' | 's3', string];

try {
const binaryPath = this.binaryDataService.getPath(binaryDataId);

if (!fileName || !mimeType) {
try {
const metadata = await this.binaryDataService.getMetadata(binaryDataId);
fileName = metadata.fileName;
mimeType = metadata.mimeType;
res.setHeader('Content-Length', metadata.fileSize);
} catch {}
}

if (mimeType) res.setHeader('Content-Type', mimeType);

if (action === 'download') {
res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`);
}

if (mode === 's3') {
const readStream = await this.binaryDataService.getAsStream(binaryDataId);
readStream.pipe(res);
return;
} else {
res.sendFile(binaryPath);
}
} catch (error) {
if (error instanceof FileNotFoundError) res.writeHead(404).end();
else throw error;
}
},
);

// ----------------------------------------
// Settings
// ----------------------------------------
Expand Down
54 changes: 54 additions & 0 deletions packages/cli/src/controllers/binaryData.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { Service } from 'typedi';
import express from 'express';
import { BinaryDataService, FileNotFoundError, isValidNonDefaultMode } from 'n8n-core';
import { Get, RestController } from '@/decorators';
import { BinaryDataRequest } from '@/requests';

@RestController('/binary-data')
@Service()
export class BinaryDataController {
constructor(private readonly binaryDataService: BinaryDataService) {}

@Get('/')
async get(req: BinaryDataRequest, res: express.Response) {
const { id: binaryDataId, action } = req.query;

if (!binaryDataId) {
return res.status(400).end('Missing binary data ID');
}

if (!binaryDataId.includes(':')) {
return res.status(400).end('Missing binary data mode');
}

const [mode] = binaryDataId.split(':');

if (!isValidNonDefaultMode(mode)) {
return res.status(400).end('Invalid binary data mode');
}

let { fileName, mimeType } = req.query;

try {
if (!fileName || !mimeType) {
try {
const metadata = await this.binaryDataService.getMetadata(binaryDataId);
fileName = metadata.fileName;
mimeType = metadata.mimeType;
res.setHeader('Content-Length', metadata.fileSize);
} catch {}
}

if (mimeType) res.setHeader('Content-Type', mimeType);

if (action === 'download') {
res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`);
}

return await this.binaryDataService.getAsStream(binaryDataId);
} catch (error) {
if (error instanceof FileNotFoundError) return res.writeHead(404).end();
else throw error;
}
}
}
143 changes: 143 additions & 0 deletions packages/cli/test/integration/binaryData.api.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
import fsp from 'node:fs/promises';
import { Readable } from 'node:stream';
import { BinaryDataService, FileNotFoundError } from 'n8n-core';
import * as testDb from './shared/testDb';
import { mockInstance, setupTestServer } from './shared/utils';
import type { SuperAgentTest } from 'supertest';

jest.mock('fs/promises');

const throwFileNotFound = () => {
throw new FileNotFoundError('non/existing/path');
};

const binaryDataService = mockInstance(BinaryDataService);
let testServer = setupTestServer({ endpointGroups: ['binaryData'] });
let authOwnerAgent: SuperAgentTest;

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

afterEach(() => {
jest.restoreAllMocks();
});

describe('GET /binary-data', () => {
const fileId = '599c5f84007-7d14-4b63-8f1e-d726098d0cc0';
const fsBinaryDataId = `filesystem:${fileId}`;
const s3BinaryDataId = `s3:${fileId}`;
const binaryFilePath = `/Users/john/.n8n/binaryData/${fileId}`;
const mimeType = 'text/plain';
const fileName = 'test.txt';
const buffer = Buffer.from('content');
const mockStream = new Readable();
mockStream.push(buffer);
mockStream.push(null);

describe('should reject on missing or invalid binary data ID', () => {
test.each([['view'], ['download']])('on request to %s', async (action) => {
binaryDataService.getPath.mockReturnValue(binaryFilePath);
fsp.readFile = jest.fn().mockResolvedValue(buffer);

await authOwnerAgent
.get('/binary-data')
.query({
fileName,
mimeType,
action,
})
.expect(400);

await authOwnerAgent
.get('/binary-data')
.query({
id: 'invalid',
fileName,
mimeType,
action,
})
.expect(400);
});
});

describe('should return binary data [filesystem]', () => {
test.each([['view'], ['download']])('on request to %s', async (action) => {
binaryDataService.getAsStream.mockResolvedValue(mockStream);

const res = await authOwnerAgent
.get('/binary-data')
.query({
id: fsBinaryDataId,
fileName,
mimeType,
action,
})
.expect(200);

const contentDisposition =
action === 'download' ? `attachment; filename="${fileName}"` : undefined;

expect(binaryDataService.getAsStream).toHaveBeenCalledWith(fsBinaryDataId);
expect(res.headers['content-type']).toBe(mimeType);
expect(res.headers['content-disposition']).toBe(contentDisposition);
});
});

describe('should return 404 on file not found [filesystem]', () => {
test.each(['view', 'download'])('on request to %s', async (action) => {
binaryDataService.getAsStream.mockImplementation(throwFileNotFound);

await authOwnerAgent
.get('/binary-data')
.query({
id: fsBinaryDataId,
fileName,
mimeType,
action,
})
.expect(404);
});
});

describe('should return binary data [s3]', () => {
test.each([['view'], ['download']])('on request to %s', async (action) => {
binaryDataService.getAsStream.mockResolvedValue(mockStream);

const res = await authOwnerAgent
.get('/binary-data')
.query({
id: s3BinaryDataId,
fileName,
mimeType,
action,
})
.expect(200);

expect(binaryDataService.getAsStream).toHaveBeenCalledWith(s3BinaryDataId);

const contentDisposition =
action === 'download' ? `attachment; filename="${fileName}"` : undefined;

expect(res.headers['content-type']).toBe(mimeType);
expect(res.headers['content-disposition']).toBe(contentDisposition);
});
});

describe('should return 404 on file not found [s3]', () => {
test.each(['view', 'download'])('on request to %s', async (action) => {
binaryDataService.getAsStream.mockImplementation(throwFileNotFound);

await authOwnerAgent
.get('/binary-data')
.query({
id: s3BinaryDataId,
fileName,
mimeType,
action,
})
.expect(404);
});
});
});
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 @@ -34,7 +34,8 @@ export type EndpointGroup =
| 'mfa'
| 'metrics'
| 'executions'
| 'workflowHistory';
| 'workflowHistory'
| 'binaryData';

export interface SetupProps {
applyAuth?: boolean;
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/test/integration/shared/utils/testServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ import { RoleService } from '@/services/role.service';
import { UserService } from '@/services/user.service';
import { executionsController } from '@/executions/executions.controller';
import { WorkflowHistoryController } from '@/workflows/workflowHistory/workflowHistory.controller.ee';
import { BinaryDataController } from '@/controllers/binaryData.controller';

/**
* Plugin to prefix a path segment into a request URL pathname.
Expand Down Expand Up @@ -316,6 +317,9 @@ export const setupTestServer = ({
case 'workflowHistory':
registerController(app, config, Container.get(WorkflowHistoryController));
break;
case 'binaryData':
registerController(app, config, Container.get(BinaryDataController));
break;
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/BinaryData/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ export function areValidModes(modes: string[]): modes is BinaryData.Mode[] {
return modes.every((m) => BINARY_DATA_MODES.includes(m as BinaryData.Mode));
}

export function isValidNonDefaultMode(mode: string): mode is BinaryData.NonDefaultMode {
return BINARY_DATA_MODES.filter((m) => m !== 'default').includes(mode as BinaryData.Mode);
}

export async function ensureDirExists(dir: string) {
try {
await fs.access(dir);
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@ export { NodeExecuteFunctions, UserSettings };
export * from './errors';
export { ObjectStoreService } from './ObjectStore/ObjectStore.service.ee';
export { BinaryData } from './BinaryData/types';
export { isValidNonDefaultMode } from './BinaryData/utils';
2 changes: 1 addition & 1 deletion packages/editor-ui/src/stores/workflows.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1393,7 +1393,7 @@ export const useWorkflowsStore = defineStore(STORES.WORKFLOWS, {
const rootStore = useRootStore();
let restUrl = rootStore.getRestUrl;
if (restUrl.startsWith('/')) restUrl = window.location.origin + restUrl;
const url = new URL(`${restUrl}/data`);
const url = new URL(`${restUrl}/binary-data`);
url.searchParams.append('id', binaryDataId);
url.searchParams.append('action', action);
if (fileName) url.searchParams.append('fileName', fileName);
Expand Down
Loading