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

feat: add 'rename' context-item #971

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/chain/context-handler-impl.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { ContextBuilder } from '../context-builder';
import { ChainCondition, CustomCondition } from '../context-items';
import { check } from '../middlewares/check';
import { Bail } from '../context-items/bail';
import { Rename } from '../context-items/rename';
import { ContextHandler, ContextHandlerImpl } from './';

let builder: ContextBuilder;
Expand Down Expand Up @@ -74,3 +75,10 @@ describe('#optional()', () => {
expect(builder.setOptional).toHaveBeenNthCalledWith(3, false);
});
});

describe('#rename()', () => {
it('adds a Rename item', () => {
contextHandler.rename('foo');
expect(builder.addItem).toHaveBeenCalledWith(new Rename('foo'));
});
});
6 changes: 6 additions & 0 deletions src/chain/context-handler-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Optional } from '../context';
import { ChainCondition, CustomCondition } from '../context-items';
import { CustomValidator } from '../base';
import { Bail } from '../context-items/bail';
import { Rename } from '../context-items/rename';
import { ContextHandler } from './context-handler';
import { ValidationChain } from './validation-chain';

Expand Down Expand Up @@ -37,4 +38,9 @@ export class ContextHandlerImpl<Chain> implements ContextHandler<Chain> {

return this.chain;
}

rename(newPath: string) {
this.builder.addItem(new Rename(newPath));
return this.chain;
}
}
1 change: 1 addition & 0 deletions src/chain/context-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ export interface ContextHandler<Chain> {
bail(): Chain;
if(condition: CustomValidator | ValidationChain): Chain;
optional(options?: Partial<Optional> | true): Chain;
rename(newPath: string): Chain;
}
21 changes: 21 additions & 0 deletions src/chain/context-runner-impl.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { FieldInstance, InternalRequest, ValidationHalt, contextsKey } from '../
import { ContextBuilder } from '../context-builder';
import { ContextItem } from '../context-items';
import { Result } from '../validation-result';
import { Rename } from '../context-items/rename';
import { ContextRunnerImpl } from './context-runner-impl';

let builder: ContextBuilder;
Expand Down Expand Up @@ -206,3 +207,23 @@ describe('with dryRun: true option', () => {
expect(req.query).toHaveProperty('bar', 456);
});
});

it('contextItem is instance of Rename', async () => {
const rename = new Rename('bar');
const mockItem = { run: jest.fn() };
builder.setFields(['foo']).addItem(rename).addItem(mockItem);
selectFields.mockReturnValue([
{ location: 'query', path: 'foo', originalPath: 'foo', value: 123, originalValue: 123 },
]);

const req = { query: { foo: 123 } };
await contextRunner.run(req);
expect(req.query).toEqual({ bar: 123 });
expect(mockItem.run).toHaveBeenCalledWith(expect.any(Context), 123, {
req: expect.objectContaining({
query: { bar: 123 },
}),
location: 'query',
path: 'bar',
});
});
20 changes: 13 additions & 7 deletions src/chain/context-runner-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as _ from 'lodash';
import { InternalRequest, Request, ValidationHalt, contextsKey } from '../base';
import { Context, ReadonlyContext } from '../context';
import { ContextBuilder } from '../context-builder';
import { Rename } from '../context-items/rename';
import { SelectFields, selectFields as baseSelectFields } from '../select-fields';
import { Result } from '../validation-result';
import { ContextRunner } from './context-runner';
Expand Down Expand Up @@ -43,14 +44,19 @@ export class ContextRunnerImpl implements ContextRunner {
path,
});

// An instance is mutable, so if an item changed its value, there's no need to call getData again
const newValue = instance.value;
if (contextItem instanceof Rename) {
// change instance path
instance.path = contextItem.newPath;
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it correct to change instance.path?

Copy link
Member

Choose a reason for hiding this comment

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

I'll guess no because of the below:

setData(path: string, value: any, location: Location) {
const instance = this.dataMap.get(getDataMapKey(path, location));
if (!instance) {
throw new Error('Attempt to write data that did not pre-exist in context');
}

but we can always change behaviour.

And side comment, I'd just find it a bit more classy if we could use context's APIs instead of checking for the specific implementation of Rename here.

} else {
// An instance is mutable, so if an item changed its value, there's no need to call getData again
const newValue = instance.value;

// Checks whether the value changed.
// Avoids e.g. undefined values being set on the request if it didn't have the key initially.
const reqValue = path !== '' ? _.get(req[location], path) : req[location];
if (!options.dryRun && reqValue !== instance.value) {
path !== '' ? _.set(req[location], path, newValue) : _.set(req, location, newValue);
// Checks whether the value changed.
// Avoids e.g. undefined values being set on the request if it didn't have the key initially.
const reqValue = path !== '' ? _.get(req[location], path) : req[location];
if (!options.dryRun && reqValue !== instance.value) {
path !== '' ? _.set(req[location], path, newValue) : _.set(req, location, newValue);
}
}
} catch (e) {
if (e instanceof ValidationHalt) {
Expand Down
67 changes: 67 additions & 0 deletions src/context-items/rename.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { ContextBuilder } from '../context-builder';
import { Meta } from '../base';
import { Rename } from './rename';

it('the new path is identical to the old one', () => {
Copy link
Member

Choose a reason for hiding this comment

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

nitpicking, but I usually like to make test names read nicely with the it.
For example:

Suggested change
it('the new path is identical to the old one', () => {
it('does nothing when the new path is identical to the old one', () => {

const context = new ContextBuilder().build();
const meta: Meta = { req: {}, location: 'body', path: 'foo' };

expect(new Rename('foo').run(context, 'value', meta)).resolves;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(new Rename('foo').run(context, 'value', meta)).resolves;
await expect(new Rename('foo').run(context, 'value', meta)).resolves;

though I'm afraid that .resolves might not be asserting anything 🤔

});

it('throws an error if trying to rename more than one field', async () => {
const context = new ContextBuilder().setFields(['foo', 'bar']).build();
const meta: Meta = { req: {}, location: 'body', path: 'foo' };

await expect(new Rename('_foo').run(context, 'value', meta)).rejects.toThrow();
});

describe('throws an error if using wildcards', () => {
it('in context fields', async () => {
const context = new ContextBuilder().setFields(['foo.*']).build();
const meta: Meta = { req: {}, location: 'body', path: 'foo' };

await expect(new Rename('_foo').run(context, 'value', meta)).rejects.toThrow();
});

it('in new path', async () => {
const context = new ContextBuilder().setFields(['foo']).build();
const meta: Meta = { req: {}, location: 'body', path: 'foo' };

await expect(new Rename('foo.*').run(context, 'value', meta)).rejects.toThrow();
});
});

it('throws an error if the new path is already assigned to a property', async () => {
const context = new ContextBuilder().setFields(['foo']).build();
const meta: Meta = {
req: {
body: {
foo: 'value',
_foo: 'bar',
},
},
location: 'body',
path: 'foo',
};

await expect(new Rename('_foo').run(context, 'value', meta)).rejects.toThrow();
});

it('throws an error if trying to rename to an existing property', () => {
const context = new ContextBuilder().setFields(['foo']).build();
const meta: Meta = {
req: {
body: {
foo: 'value',
},
},
location: 'body',
path: 'foo',
};

expect(new Rename('_foo').run(context, 'value', meta)).resolves;
expect(meta.req.body).toEqual({
_foo: 'value',
});
});
30 changes: 30 additions & 0 deletions src/context-items/rename.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import * as _ from 'lodash';
import { Meta } from '../base';
import { Context } from '../context';
import { ContextItem } from './context-item';

export class Rename implements ContextItem {
constructor(readonly newPath: string) {}

async run(context: Context, value: any, meta: Meta) {
const { req, location, path } = meta;

if (path !== this.newPath) {
if (context.fields.length !== 1) {
throw new Error('Cannot rename multiple fields.');
}
const field = context.fields[0];
if (field.includes('*') || this.newPath.includes('*')) {
throw new Error('Cannot use rename() with wildcards.');
}

if (_.get(req[location], this.newPath) !== undefined) {
throw new Error(`Cannot rename to req.${location}.${path} as it already exists.`);
Comment on lines +21 to +22
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that a client might, due to a bug, send more information than required and litter logs with 5XX errors (as I think it'll probably flow all the way to next()).

Perhaps it's best to remove this?

}
Copy link
Member Author

Choose a reason for hiding this comment

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

These errors are thrown at runtime, should we use context.addError() even if it is not a validation error?


_.set(req[location], this.newPath, value);
_.unset(req[location], path);
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

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

These should obey the dryRun option

}
return Promise.resolve();
}
}