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

update: fix async-app app chaining when mounting sub app #45

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
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
28 changes: 27 additions & 1 deletion features/advanced.feature
Original file line number Diff line number Diff line change
Expand Up @@ -239,4 +239,30 @@ Scenario: get negative TODO permissions with errors
}
}
}
"""
"""

Scenario: mounted apps (nest level 1)
Given app setting "trust proxy" is enabled
Copy link
Collaborator

Choose a reason for hiding this comment

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

NITPICK:

Suggested change
Given the request header X-Forwarded-For is "1.1.1.1"
And the request header X-Forwarded-For is "1.1.1.1"

Given the request header X-Forwarded-For is "1.1.1.1"
When GET /sub/test
Then the response is 200
And the response payload includes
"""
{
"route": "/sub/test",
"ip": "1.1.1.1"
}
"""

Scenario: mounted apps (nest level 2)
Given app setting "trust proxy" is enabled
Copy link
Collaborator

Choose a reason for hiding this comment

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

NITPICK:

Suggested change
Given the request header X-Forwarded-For is "10.0.1.3"
And the request header X-Forwarded-For is "10.0.1.3"

Given the request header X-Forwarded-For is "10.0.1.3"
When GET /sub/sub/test
Then the response is 200
And the response payload includes
"""
{
"route": "/sub/sub/test",
"ip": "10.0.1.3"
}
"""
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "async-app",
"version": "4.9.0",
"version": "4.10.0",
"description": "An express wrapper for handling async middlewares, order middlewares, schema validator, and other stuff",
"type": "commonjs",
"main": "dist/index.js",
Expand Down Expand Up @@ -52,7 +52,9 @@
"build": "tsc",
"dist": "rm -rf dist && npm run build",
"lint": "tslint -p .",
"lint:fix": "tslint -p . --fix",
"test": "cucumber-js --require-module ts-node/register --exit -r src/test.ts",
"test:debug": "node --inspect-brk=0.0.0.0:9429 --no-lazy ./node_modules/.bin/cucumber-js --require-module ts-node/register --exit -r src/test.ts",
"watch": "tsc -w",
"prepack": "npm run test",
"prepare": "npm run dist"
Expand Down
110 changes: 59 additions & 51 deletions src/async.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ interface AsyncOptions<TEntities extends Entities, TSchema> {
mapAsyncResultFn?: MapAsyncResultFn<TEntities>;
}

type Options<TEntities extends Entities> = {
type Options<TEntities extends Entities> = {
statusCode: number;
isLastMiddleware: boolean;
validateSchema?: ValidateSchema;
Expand All @@ -36,11 +36,11 @@ const copyDecorators = <TSrc extends Decorator, TDest extends Decorator>(
src: TSrc,
dest: TDest,
) => {
const keys = Object
.keys(src)
.filter(k => k.startsWith('$')) as (keyof Decorator)[];
const keys = Object.keys(src).filter(k =>
k.startsWith('$'),
) as (keyof Decorator)[];

keys.forEach(k => dest[k] = src[k]);
keys.forEach(k => (dest[k] = src[k]));
};

const mapMiddleware = <TEntities extends Entities>(
Expand All @@ -64,16 +64,16 @@ const mapMiddleware = <TEntities extends Entities>(
const isAsync = isAsyncMiddleware(middleware);

const mappedVal = options.mapAsyncResultFn
? await options.mapAsyncResultFn(result, {
isAsyncMiddleware: isAsync,
isLastMiddleware: options.isLastMiddleware,
req,
})
: result;
? await options.mapAsyncResultFn(result, {
isAsyncMiddleware: isAsync,
isLastMiddleware: options.isLastMiddleware,
req,
})
: result;

let val = mappedVal;
let isRaw = false;
let headers: Record<string, string|string[]> | undefined;
let headers: Record<string, string | string[]> | undefined;

if (mappedVal instanceof CustomResponse) {
val = mappedVal.value;
Expand Down Expand Up @@ -125,7 +125,9 @@ const mapMiddleware = <TEntities extends Entities>(
res.json(val || {});
}
} catch (err) {
if (!err || !(err instanceof CustomError)) { return next(err); }
if (!err || !(err instanceof CustomError)) {
return next(err);
}

const { statusCode, error, extra } = err;

Expand All @@ -148,42 +150,48 @@ const mapMiddleware = <TEntities extends Entities>(
};

export default <TEntities extends Entities, TSchema>({
errorHandler,
compileSchema,
mapAsyncResultFn,
}: AsyncOptions<TEntities, TSchema> = {}): Converter<TEntities, TSchema> =>
(args, context) => {
const statusCode = args.find(isNumber) || DEFAULT_STATUS_CODE;

const schema = args
.filter(isSchema<TSchema>())
.find(s => s.$scope === 'response');
const rawSchema = schema
errorHandler,
compileSchema,
mapAsyncResultFn,
}: AsyncOptions<TEntities, TSchema> = {}): Converter<TEntities, TSchema> => (
args,
context,
) => {
if (context.method === 'use') {
return args;
}
Comment on lines +160 to +162
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might be a little bit too eager — one other solution would be to make isMiddleware return false whenever we send a App instance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine!


const statusCode = args.find(isNumber) || DEFAULT_STATUS_CODE;

const schema = args
.filter(isSchema<TSchema>())
.find(s => s.$scope === 'response');
const rawSchema = schema
? schema.$schema
? schema.$schema
? schema.$schema
: schema
: undefined;
const validateSchema =
compileSchema && rawSchema && compileSchema(rawSchema, context);

const middlewares = args.filter(isMiddleware);
const lastMiddleware = middlewares[middlewares.length - 1];

// 4 argument middlewares are error handlers, and we leave them untouched
return args.map(m =>
isMiddleware(m) && m.length < 4
? mapMiddleware(
m,
// Check if this is the last valid middleware (not the statusCode arg)
{
isLastMiddleware: m === lastMiddleware,
mapAsyncResultFn,
statusCode,
validateSchema,
},
// Passes a custom error handler
errorHandler,
)
: m,
);
};
: schema
: undefined;
const validateSchema =
compileSchema && rawSchema && compileSchema(rawSchema, context);

const middlewares = args.filter(isMiddleware);
const lastMiddleware = middlewares[middlewares.length - 1];

// 4 argument middlewares are error handlers, and we leave them untouched
return args.map(m =>
isMiddleware(m) && m.length < 4
? mapMiddleware(
m,
// Check if this is the last valid middleware (not the statusCode arg)
{
isLastMiddleware: m === lastMiddleware,
mapAsyncResultFn,
statusCode,
validateSchema,
},
// Passes a custom error handler
errorHandler,
)
: m,
);
};
48 changes: 37 additions & 11 deletions src/examples/advanced/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,22 @@ import bodyParser from 'body-parser';
import express from 'express';
import { join } from 'path';

import { computePermissions, createCustomResponse, deprecate } from '../..';
import createApp, { Req } from './async-app';
import {
App,
computePermissions,
createCustomResponse,
deprecate,
} from '../..';
import createApp, { ExampleEntities, Req } from './async-app';
import can, { entities } from './can';
import { addTodo, addUser, getTodosForUser } from './db';
import load from './load';
import purgeUser from './purge-user';

const app = createApp();

app.disable('x-powered-by');

app.use(bodyParser.json());

// --- Deprecated ----------------------------------------------------------- //
Expand Down Expand Up @@ -46,11 +54,13 @@ app.get(
app.post(
'/users',
'Creates a user', // This is the summary string for the endpoint
{ // This is the expected schema of the body
{
// This is the expected schema of the body
name: 'string',
username: 'string',
},
{ // This is the expected schema of the response (optional)
{
// This is the expected schema of the response (optional)
$scope: 'response',
username: 'string',
},
Expand Down Expand Up @@ -95,12 +105,14 @@ app.get(
'/todos/:username',
'Returns the TODOs for the specified user',
{
$schema: [{
id: 'number',
item: 'string',
owner: 'string',
readOnly: 'boolean',
}],
$schema: [
{
id: 'number',
item: 'string',
owner: 'string',
readOnly: 'boolean',
},
],
$scope: 'response',
},
load.user.fromParams(),
Expand Down Expand Up @@ -185,6 +197,20 @@ app.use(
express.static(join(__dirname, '../../../src/examples/advanced/docs')),
);

app.disable('x-powered-by');
// --- Mounted sub app --- //
const mountSubApp = (app: App<ExampleEntities, {}>) => {
const subApp = createApp();
app.use('/sub', subApp);

subApp.get('/test', 'Mounted app route information', (req: Req) => ({
ip: req.ip,
route: join(req.app.path(), req.path),
}));

return subApp;
};

const sub = mountSubApp(app);
mountSubApp(sub);

export default app;
Loading
Loading