feat: add 'rename' context-item by fedeci · Pull Request #971 · express-validator/express-validator

fedeci · 2021-01-02T13:37:41Z

Description

This allows to rename a property.

// req: { body: { name: 'foo' } } becomes 
// req: { body: { username: 'foo' } }
body('name').rename('username').isString()

// req: { body: { names: ['foo', 'bar'] } } throws
body('names.*').rename('username').isString()

// req: { body: { name: 'foo', surname: 'bar' } } throws
body(['name', 'surname']).rename('username')

// req: { body: { name: 'foo', surname: 'bar' } } throws
body('name').rename('surname')

Closes #785

To-do list

I have added tests for what I changed.

This pull request is ready to merge.

fedeci · 2021-01-02T13:41:04Z

+    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.`);
+      }


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

coveralls · 2021-01-02T13:52:22Z

Coverage remained the same at 100.0% when pulling db52eb4 on fedeci:rename-sanitizer into 0b0b6c8 on express-validator:master.

fedeci · 2021-01-02T14:10:53Z

-          const newValue = instance.value;
+          if (contextItem instanceof Rename) {
+            // change instance path
+            instance.path = contextItem.newPath;


Is it correct to change instance.path?

I'll guess no because of the below:

express-validator/src/context.ts

Lines 66 to 70 in 6715a6a

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.

gustavohenke

ohey only 1.5 years later

gustavohenke · 2022-09-25T09:01:26Z

+      if (this.newPath.includes('*')) {
+        throw new Error('Cannot use rename() with wildcards.');
+      }


WDYT of moving this to the constructor/.rename()?

Just thinking that failing on server start might be a better developer experience than on request.

gustavohenke · 2022-09-25T09:10:41Z

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


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?

gustavohenke · 2022-09-25T09:12:13Z

+      _.set(req[location], this.newPath, value);
+      _.unset(req[location], path);


These should obey the dryRun option

gustavohenke · 2022-09-25T09:14:59Z

+import { Meta } from '../base';
+import { Rename } from './rename';
+
+it('the new path is identical to the old one', () => {


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', () => {

gustavohenke · 2022-09-25T09:15:25Z

+  const context = new ContextBuilder().setFields(['foo']).build();
+  const meta: Meta = { req: {}, location: 'body', path: 'foo' };
+
+  expect(new Rename('foo').run(context, 'value', meta)).resolves;


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 🤔

gustavohenke · 2022-09-25T09:17:23Z

-          const newValue = instance.value;
+          if (contextItem instanceof Rename) {
+            // change instance path
+            instance.path = contextItem.newPath;


I'll guess no because of the below:

express-validator/src/context.ts

Lines 66 to 70 in 6715a6a

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.

fedeci added 2 commits January 1, 2021 18:23

feat: add 'Rename' context-item

7a1f079

feat: add 'rename' to context-handler

a663a4b

fedeci requested a review from gustavohenke January 2, 2021 13:37

fedeci added the i: enhancement label Jan 2, 2021

fedeci commented Jan 2, 2021

View reviewed changes

fedeci added 2 commits January 2, 2021 14:47

feat(context-runner): update instance.path on rename

d19d8d3

style: run eslint

55097d4

fedeci commented Jan 2, 2021

View reviewed changes

feat: assume no wildcards in old path

db52eb4

fedeci force-pushed the rename-sanitizer branch from e26d3df to db52eb4 Compare January 18, 2021 14:07

fedeci added this to the v7.0.0 milestone Feb 23, 2021

fedeci added PR: new feature and removed i: enhancement labels Feb 23, 2021

gustavohenke requested changes Sep 25, 2022

View reviewed changes

gustavohenke force-pushed the master branch 2 times, most recently from 3177f64 to 6e160b1 Compare April 7, 2023 12:26

gustavohenke removed this from the v7.0.0 milestone Apr 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add 'rename' context-item#971

feat: add 'rename' context-item#971
fedeci wants to merge 5 commits into
express-validator:masterfrom
fedeci:rename-sanitizer

fedeci commented Jan 2, 2021 •

edited

Loading

Uh oh!

fedeci Jan 2, 2021

Uh oh!

coveralls commented Jan 2, 2021 •

edited

Loading

Uh oh!

fedeci Jan 2, 2021

Uh oh!

gustavohenke Sep 25, 2022

Uh oh!

gustavohenke left a comment

Uh oh!

gustavohenke Sep 25, 2022

Uh oh!

gustavohenke Sep 25, 2022

Uh oh!

gustavohenke Sep 25, 2022

Uh oh!

gustavohenke Sep 25, 2022

Uh oh!

gustavohenke Sep 25, 2022

Uh oh!

gustavohenke Sep 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	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');
	}

		if (_.get(req[location], this.newPath) !== undefined) {
		throw new Error(`Cannot rename to req.${location}.${path} as it already exists.`);

		_.set(req[location], this.newPath, value);
		_.unset(req[location], path);

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

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

Uh oh!

Conversation

fedeci commented Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

To-do list

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gustavohenke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fedeci commented Jan 2, 2021 •

edited

Loading

coveralls commented Jan 2, 2021 •

edited

Loading