jurassix / react-validation-mixin

Simple validation mixin (HoC) for React.
MIT License
283 stars 38 forks source link

Decoding via "he" fails when errors are nested (expects String - gets an Object) #59

Closed idolize closed 8 years ago

idolize commented 8 years ago

I've noticed that react-validation-mixin contains code to decode error messages using the following branching paths:

export default function decode(list, key) {
  if (defined(list, key)) {
    const value = get(list, key);
    if (Array.isArray(value)) {
      return value.map(he.decode);
    }
    return value ? he.decode(value) : '';
  }
}

However, when I run getValidationMessages() in my project it blows up here, because value is coming back as an Object keyed with Arrays for each invalid field. Thus, when decode runs it tries to run he.decode on an Object (which expects a String and thus can't find the String.replace method and throws an Exception).

screen shot 2015-11-12 at 12 12 59 pm

I'm using react-validation-mixin version 5.3.1.

idolize commented 8 years ago

It appears get(list, key) is returning another Object mapped by key when the error comes from inside a nested object.

Maybe instead of:

export default function decode(list, key) {
  if (defined(list, key)) {
    const value = get(list, key);
    if (Array.isArray(value)) {
      return value.map(he.decode);
    }
    return value ? he.decode(value) : '';
  }
}

it should be recursive? Something like:

export default function decode(list, key) {
  if (defined(list, key)) {
    const value = get(list, key);
    if (isPlainObject(value)) { // isPlainObject from lodash
      return Object.keys(value).map(decode.bind(this, value));
    }
    if (Array.isArray(value)) {
      return value.map(he.decode);
    }
    return value ? he.decode(value) : '';
  }
}
idolize commented 8 years ago

(I updated the title because it seems like it only happens on validation errors that come from within nested objects)

jurassix commented 8 years ago

Thanks for the report. I'll try to get some tests around this.

idolize commented 8 years ago

Thanks @jurassix

jurassix commented 8 years ago

I'm thinking the easiest place to handle this is inside the Joi strategy and not a top level concern of this library. Also, the Joi validation results are returned flat, mapping over them and applying decode is simpler.

However, I have one issue now. getValidationMessages needs to return an array of error messages, in order to achieve this I have to flatten a complex object. Looking for a good solution for this.

idolize commented 8 years ago

If the Joi strategy is updated to return objects keyed by the "invalid property name" with the values being the error strings, wouldn't something like Object.keys(errors).map(key => errors[key]) suffice?

jurassix commented 8 years ago

We support nested schema's so objects can have a structure like:

{a: {b: {c: "is required"}}}
jurassix commented 8 years ago
function flattenErrorsObject(obj) {
  var values = [];
  if(_.isObject(obj)) {
    values = _.values(obj);
  }  else if(_.isString(obj)) {
    return obj;
  }
  var flat = values.map(value => {
    if (!_.isString(value)) {
      return flattenErrorsObject(value);
    }
    return value;
  });  
  return _.flatten(flat);
}
var test = {a:'me', b: {c: 'c', d: {e:['f','g']}}};
> flattenErrorsObject(test)
< ["me", "c", "f", "g"]
jurassix commented 8 years ago

@idolize Think you can code review this? Is there a better way to do this?

jurassix commented 8 years ago

@idolize I refactored the above algorithm here

This issue should be fixed with the following patches:

react-validation-mixin v5.3.2 joi-validation-strategy v0.3.2