Skip to content

Don't delete object key when it equals the rename.to in a rename with regex#1763

Closed
paixaop wants to merge 3 commits intohapijs:masterfrom
paixaop:master
Closed

Don't delete object key when it equals the rename.to in a rename with regex#1763
paixaop wants to merge 3 commits intohapijs:masterfrom
paixaop:master

Conversation

@paixaop
Copy link

@paixaop paixaop commented Mar 28, 2019

Calling rename with a regex where the key matches the rename.to causes the key to get deleted from the object.

Example: I use the folllowing schema to validate an AWS x-api-key header:

Joi.object()
    .keys({
        'x-api-key': stringsSchema.awsApiKey.required(),
    })
    .rename(/x-api-key/i, 'x-api-key', {
        // alias: true,
        override: true,
        ignoreUndefined: true,
    })

Using rename to force a value such as 'X-ApI-KeY' to be all lower case, and therefore normalizing it to 'x-api-key'.
The bug is when the header is already all lower case 'x-api-key', the rename.from is the same as rename.to so it gets deleted if option.alias = false, leaving the object with no x-api-key at all.

So this:

const Joi = require('joi');

const schema = Joi.object()
    .keys({
        'x-api-key': Joi.string().token().min(30).max(128),
    })
    .rename(/x-api-key/i, 'x-api-key', {
        // alias: true,
        override: true,
        ignoreUndefined: true,
    });

const result = Joi.attempt({
      'x-api-key': 'ABCEFGHIJKLMNOPQRSTUVXYWZabcefghijklmnopqrstuvxywz0123456789'
 }, schema);

console.log(result); // prints { }

@paixaop
Fix the case where the rename regex matches the rename.to which was c…
84eafa4 
…ausing the original key to be deleted if rename.options.alias was false
@paixaop paixaop changed the title Fix the case where the rename regex deletes the key Don't delete object key when it equals the rename.to in a rename with regex Mar 28, 2019
@paixaop
Copy link
Author

paixaop commented Mar 28, 2019

Test 837) object rename() using regex errors multiple times when abortEarly is false: is failing because the schema sets a .rename(/z/i, 'z') which eventually deletes z and does not rename it.
In my view this is a bad behavior, using a rename to get a key deletion, I would suggest this check to be modified so the deletion is not the expected outcome. Or am I missing a use case for this ?

@WesTyler
Copy link
Contributor

WesTyler commented Mar 29, 2019

It was not clear to me that you were fixing the issue with deleted keys, and also implementing a new feature. Neither the PR discussion nor the git commits mention the new . keysToLowerCase() method you've added.

Please separate the two into different branches and different PRs so they can be reviewed separately. I would also suggest opening up a issue on keysToLowerCase so that we can discuss the new feature first :)

@WesTyler WesTyler closed this Mar 29, 2019
@Marsup
Copy link
Collaborator

Marsup commented Mar 29, 2019

While I understand what you're trying to do, nodejs already lowercases the headers so I don't really see the point. As Wes already told, you didn't open an issue 1st, so maybe I'm lacking the context usually asked in issues.

@paixaop
Copy link
Author

paixaop commented Mar 30, 2019

@Marsup it's true that node lower cases the headers, but AWS API Gateway does not and this complicates things when working with AWS Lambda and using Joi for input validation.

@WesTyler I will separate the two pull requests. Thanks!

@paixaop paixaop mentioned this pull request Mar 30, 2019
Closed
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@paixaop @WesTyler @Marsup