-
-
Notifications
You must be signed in to change notification settings - Fork 616
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
Rename field with wildcard support #1215
base: master
Are you sure you want to change the base?
Conversation
if (typeof actualResult === 'string') { | ||
context.renameFieldInstance(actualResult, meta); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask why you thought of making use of the custom validators?
Changing the meaning of the return of a custom validator will be quite disruptive for everybody, and is certainly a breaking change.
Perhaps a new function in the chain would be a better idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We ran into a problem where we needed to rename the search
field on the bases of the input. I thought of the custom validator as a solution because it solves both of the requirements. To avoid the breaking change, we can accept the return type of an object containing a field, for example { express_rename: 'foo' }
.
src/context.ts
Outdated
if (this.fields.length !== 1) { | ||
throw new Error('Attempt to rename multiple fields.'); | ||
} | ||
if (/\.|\*/g.test(originalPath)) { | ||
throw new Error('Attempt to rename a wildcard field'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thinking here. I suppose that the callback should be able to handle the name to give to a field though -- as it has access to the full original path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some success on renaming wildcard paths but need to do more work for wildcard support
- Wildcard support - Test cases added for wildcard and non-wildcard field rename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: should the old property be removed when renaming the item?
renameFieldInstance
will work for setting the new property with the same value as the old field instance, but it won't touch the old one.
express-validator/src/chain/context-runner-impl.ts
Lines 50 to 54 in eba0cfe
// 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); | |
} |
src/chain/validators.ts
Outdated
* @param evaluator the custom evaluator based on the `CustomValidator` | ||
* @returns the current validation chain | ||
*/ | ||
rename(evaluator: CustomValidator): Return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this is better!
A few comments here though:
- This shouldn't stay in the
Validators
interface.ContextHandler
sounds more like it. - Let's add a type specific for the rename callback, so that
CustomValidator
can evolve independently from the rename functionality. - WDYT of accepting a string value too, which works as a shortcut? For example,
rename('banana')
automatically renames that field tobanana
.
src/context-items/rename-field.ts
Outdated
import { ContextItem } from './context-item'; | ||
|
||
export class RenameFieldContextItem implements ContextItem { | ||
message: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you move rename
out of Validators
, I think that this won't be necessary anymore
src/context-items/rename-field.ts
Outdated
export class RenameFieldContextItem implements ContextItem { | ||
message: any; | ||
|
||
constructor(private readonly evaluator: CustomValidator, private readonly negated: boolean) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for negated
- Shifted rename functionality to context handler
3177f64
to
6e160b1
Compare
Description
This allows to rename fields using rename context item.
Example
Request Body
Output
closes #785
To-do list
Wildcard support
I have added tests for what I changed.