Fixes [#1239] #1240

samueljoli · 2017-07-06T18:58:43Z

This PR extends Joi.object.rename() to allow the renaming of key properties via regex. The necessity arose from needing to validate against url query parameters and being case insensitive, while still being able to refer to the param in question in my src code in it's default, expected format.

Use case:
client requests...
base_url/resource?marketCode=191 - camel cased
base_url/resource?marketcode=191 - lower cased
base_url/resource?MarketCode=191 - pascal cased

/*...assuming your use case is for api development with hapi, in your handler func you'd be able to reference marketcode in it's format specified in your validation schema */

const marketCodeRegExp = /^marketcode$/i;

config: {
  validate: Joi.object().keys({
    marketCode: Joi.string()
  }).rename(marketCodeRegExp, 'marketCode')
}

//...handler.js
request.query.marketCode /*  would always evaluate to => 191 regardless of what format the parameter sent in */

AdriVanHoudt · 2017-07-07T07:57:42Z

lib/types/object/index.js

                delete target[rename.to];
            }
+            else if (typeof rename.from === 'object') {
+                const fromKey = Object.keys(target).find((key) => {


Should not test if the object is an actual regex?

@AdriVanHoudt comment addressed. Good catch

DavidTPate · 2017-07-07T18:06:40Z

lib/types/object/index.js

-            if (target[rename.from] === undefined) {
+
+
+            if (target[rename.from] === undefined && !(rename.from instanceof RegExp)) {


We call rename.from instanceof RegExp 3 times in here seems like a good case to use a boolean and just reference that. ie. let isRegExRename = rename.from instanceof RegExp

DavidTPate

Can you also add some a piece to the docs about this new feature please? Just a short sentence and example will suffice.

DavidTPate · 2017-07-07T18:10:20Z

lib/types/object/index.js

                delete target[rename.to];
            }
+            else if (rename.from instanceof RegExp) {
+                const fromKey = Object.keys(target).find((key) => {


Can we loop over this with a more traditional method instead? There's a heavy cost to using the closure here in the critical path of the validation which would be elevated through not using it.

const objectKeys = Object.keys(target); for (var i = 0, il = objectKeys.length; i < il; i++) { ... }

DavidTPate · 2017-07-07T18:14:08Z

lib/types/object/index.js

+                    return rename.from.test(key);
+                });
+
+                target[rename.to] = target[fromKey];


We're going to have some weird behavior here if we encounter more than one matching key. I would think we either want to error when we encounter multiple keys matching the RegExp or convert each one of them. Currently, we will only rename the last matching one that we encounter.

Ideally, something like this:

const objectKeys = Object.keys(target); for (var i = 0, il = objectKeys.length; i < il; i++) { if (rename.from.test(objectKeys[i]) { target[rename.to] = target[objectKeys[i]]; if (!rename.options.override) { // don't override the key if it already exists (so validation would fail I believe) } if (!rename.options.alias) { delete target[objectKeys[i]]; } } }

samueljoli · 2017-07-10T16:16:28Z

@DavidTPate Comments addressed

WesTyler

Looks good to me!

DavidTPate · 2017-07-14T18:20:40Z

lib/types/object/index.js

+                    if (rename.from.test(objectKeys[j])) {
+                        target[rename.to] = target[objectKeys[j]];
+
+                        if (!rename.options.override) {


From what I see in the docs this is not the correct usage of override.

override - if true, allows renaming a key over an existing key. Defaults to false.

Instead, this particular usage should be alias.

alias - if true, does not delete the old key name, keeping both the new and old keys in place. Defaults to false.

So this particular line should be if (!rename.options.alias) {

and then we also need to implement override for the regular expressions as well.

You can see how we implement that here.

DavidTPate · 2017-07-14T18:22:04Z

test/types/object.js

+            const schema = Joi.object({
+                fooBar: Joi.string(),
+                fooBaz: Joi.string()
+            }).rename(regex, 'fooBar', { override: true });


Once we fix the usage of alias we should update this test and also add a test for the override functionality.

I would expect that if we have multiple keys matching our regex and override set to false then we would throw an error. Similarly if we have override set to false and the property is already on the object we would throw an error.

samueljoli · 2017-07-20T20:04:52Z

@DavidTPate Comments addressed.

Things to note:
I didn't see the need to reimplement override when using regex. The snippet you referenced should be used as a catch all.

There was a question that arose when implementing alias, which was how can I alias a key that isn't defined on the schema originally?

If we're using regex, than the schema / target that we are validating against very well could have a key that is in a different case compared to how it's defined in the joi schema. So I added some logic to normalize the key in question back to the case form that it is defined in the schema.

I've added comments.

Marsup · 2017-07-24T21:00:58Z

lib/types/object/index.js

@@ -118,16 +119,42 @@ internals.Object = class extends Any {
                }
            }

-            if (target[rename.from] === undefined) {
+
+            const isRegExRename = rename.from instanceof RegExp;


Just store it when you're calling the rule, no need to check that on each validation.

Marsup · 2017-08-09T20:33:54Z

lib/types/object/index.js

+
+                        // Using regex: Key needs to be normalized to proper case
+
+                        for (let k = this._inner.children.length; k--;) {


This part is wrong, rename has ato argument, use that, plain and simple, don't try to be clever by looking for the matching child. And also make use of the options, some cases are missing here.

Defer to other reviews.

WesTyler · 2017-08-18T14:26:40Z

@Marsup - @samueljoli and I did a pretty big re-write of his implementation to simplify it. We also added several more tests and error handling around multiple matching keys with multiple: false, and store the result of instanceof Regexp on the renames object when the rule is called to prevent evaluating on every validation.

samueljoli · 2017-08-18T14:27:26Z

@Marsup Hey comments have been addressed. @WesTyler thanks for the assist.

Marsup

I'll just stick to that one comment and let you update before continuing the review, I think it makes more sense that I review once you've fixed that.

Marsup · 2017-09-20T22:17:56Z

lib/types/object/index.js

@@ -118,18 +119,53 @@ internals.Object = class extends Any {
                }
            }

-            if (target[rename.from] === undefined) {
-                delete target[rename.to];
+            if (rename.isRegExp) {


The place of this if is likely wrong, it should be a regexp path or a string path, trying to use both is madness. For example you were just lucky in your ignoreUndefined test cases :

const schema = Joi.object({ b: Joi.any() }).rename(/^a$/, 'b', { ignoreUndefined: true }); schema.validate({ a: 1 }) // Errors because it failed to rename a to b, it stopped the loop in the 1st statement

Hey alright so we addressed that test case, but I honestly have mixed feelings on the implementation for the fix. We are using a .filter to do the undefined check and also prevent a duplicate future iteration as a side effect of the filter callback. Don't like that, but it does prevent double iteration over the same keys.

Let us know what you think of this approach. We can also just split the regexp and string paths completely; that just meant duplicating the exact logic for the multiple and override checks into both blocks.

Marsup

I still think trying to insert the regexp case into the normal case is wrong, the more I look at the code the more it seems overly complex.

Marsup · 2017-09-21T16:59:52Z

lib/types/object/index.js

@@ -94,8 +95,22 @@ internals.Object = class extends Any {
        const renamed = {};
        for (let i = 0; i < this._inner.renames.length; ++i) {
            const rename = this._inner.renames[i];
+            const matchedTargetKeys = [];
+
+            const fromIsUndefined = rename.isRegExp ?


The semantic is bit different, ignoreUndefined is supposed to ignore, well, undefined :)
Finding that it's a match and that it's undefined is quite different.

Agreed. We will refactor to completely separate the string/regex paths. That should clean up this bit too.

Marsup · 2017-09-21T17:07:22Z

lib/types/object/index.js

+                    if (!rename.options.multiple &&
+                        matchedTargetKeys.length > 1) {
+
+                        errors.push(this.createError('object.rename.multiple', { from: matchedTargetKeys, to: rename.to }, state, options));


So in theory, if abortEarly is false, you could get two object.rename.multiple for a single rename, that seems weird.

That would be the desired result when the target object has two keys that both match the rename from regex, right? I agree it's weird, but I don't know what the other option would be.

Probably not, your current code is, I think, giving a single error containing all the fields that couldn't be renamed. The problem here is it's erroring on the case that was previously for strings, and also on this line that is made for regexps.

Oh I see what you mean now. Not "2 errors" as in the 2 fields, but literally hitting two different error blocks in the code for the same rename object. 👍

I think completely separating the regex/string code paths will solve that.

DavidTPate · 2017-09-21T17:45:59Z

lib/types/object/index.js

+            const matchedTargetKeys = [];
+
+            const fromIsUndefined = rename.isRegExp ?
+                Object.keys(target).filter((targetKey) => {


Instead of using Array.filter here, can we switch this to utilizing a plain old for.... We usually try to steer clear of Array.filter, Array.map, etc. within the critical path of code due to the performance cost of them.

Yeah, based on @Marsup's feedback above this whole block will be re-written and we won't need the filter anymore anyways :)

lock · 2020-01-09T07:44:01Z

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.

Fixes [#1239]

a78220b

AdriVanHoudt reviewed Jul 7, 2017

View reviewed changes

Samuel Joli added 3 commits July 7, 2017 11:56

use instance check over type check, add test for override enabled

5eb489a

Add missing instanceof check

a94decf

add instance check to hoek assertion

d009efd

DavidTPate reviewed Jul 7, 2017

View reviewed changes

DavidTPate suggested changes Jul 7, 2017

View reviewed changes

Samuel Joli and others added 4 commits July 10, 2017 11:19

Dry up regex instanceof check

e1ee49e

More tradtional loop

b9c2ed1

Update API.md

5b0b0aa

Fix linting

7294858

WesTyler previously approved these changes Jul 14, 2017

View reviewed changes

DavidTPate suggested changes Jul 14, 2017

View reviewed changes

DavidTPate reviewed Jul 14, 2017

View reviewed changes

Address comments. Implement using alias and override options properly.

cb125db

samueljoli added 2 commits July 20, 2017 16:14

Update API.md

1bd504f

Linting

3e265cf

Marsup requested changes Aug 9, 2017

View reviewed changes

samueljoli added 2 commits August 16, 2017 21:36

Add test to cover all rename options use cases

86ea779

Refactor

670f94d

Marsup requested changes Sep 20, 2017

View reviewed changes

Handle ignoreUndefined in object().rename()

84ae232

Marsup requested changes Sep 21, 2017

View reviewed changes

DavidTPate reviewed Sep 21, 2017

View reviewed changes

Separate logic into string and regex paths

dad79df

Marsup merged commit 2ab29c7 into hapijs:master Oct 8, 2017

Marsup added a commit that referenced this pull request Oct 8, 2017

Cleanup #1240.

8bc85a4

Marsup self-assigned this Oct 8, 2017

Marsup added this to the 11.3.0 milestone Oct 8, 2017

Marsup added the feature New functionality or improvement label Oct 8, 2017

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020

hueniverse unassigned Marsup Jul 15, 2020

		if (target[rename.from] === undefined) {


		if (target[rename.from] === undefined && !(rename.from instanceof RegExp)) {


		// Using regex: Key needs to be normalized to proper case

		for (let k = this._inner.children.length; k--;) {

Fixes [#1239] #1240

Fixes [#1239] #1240

Uh oh!

Conversation

samueljoli commented Jul 6, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

DavidTPate 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!

samueljoli commented Jul 10, 2017

Uh oh!

WesTyler 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!

samueljoli commented Jul 20, 2017 • 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!

WesTyler commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueljoli commented Aug 18, 2017

Uh oh!

Marsup 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!

Marsup 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lock bot commented Jan 9, 2020

Uh oh!

Uh oh!

samueljoli commented Jul 6, 2017 •

edited

Loading

samueljoli commented Jul 20, 2017 •

edited

Loading

WesTyler commented Aug 18, 2017 •

edited

Loading