Poll: Flatten Validation Errors?

[erikras]

This idea came from @clayne11's excellent write-up on #1310.

---

There are a few problems with the current way that validation errors are returned:

1. Array errors (_error) don't work with ImmutableJS
2. Using _error for object and array errors is just gross.

What if we flattened the error object down to a map from field name to error?

Deep

This is the way we currently return errors:

const errors = {
  test: {
    _error: 'Awkward special rule'
    foo: 'This is an error',
    bar: 'This is another error',
  },
  foobar: 'An error',
  anArrayField: [
    {
      test: 'This is not a valid value'
    }
  ]
  _error: 'This is a form wide error'
}
errors.anArrayField._error = 'Must have 3 entries'

Flattened

This is the proposed new way:

const errors = {
  test: 'No longer awkward',
  'test.foo': 'This is an error',
  'test.bar': 'This is another error',
  foobar: 'An error',
  anArrayField: 'Must have 3 entries' 
  'anArrayField[0].test': 'This is not a valid value',
  _error: 'This is a form wide error' // still needed :-(
}

This makes it trivial to represent errors at any depth in the form tree without having to rely on the magical _error property.

Poll Question

Should we migrate to a flattened error structure for the v6.0.0 release?

---

Please vote with the 👍 buttons. And feel free to give reasons in the comments.

[erikras]

Option 1

No, I prefer the deep error structure.

[erikras]

Option 2

No. I love the flattened structure, but save it for v7. Just release v6 already!

[erikras]

Option 3

Yes, I think it is worth adding another breaking change now before release.

[kristian-puccio]

If we do go with the flattened structure maybe provide some help functions aka lodash style so you can choose to work in either fashion.

Or maybe provide a function to apply the errors full stop.

Maybe:

validate() {
   let fieldErrors = {
      email: 'not an email!',
   };
   let formError = 'The whole thing is stuffed';
   return setValidateErrors(fieldErrors, formError);
}

I don't think my code above is right but hopefully you get the idea.
Basically we're using an object as an api to describe error state at the momment but maybe that could be expressed as parameters to a function. Which of course would return an object in whatever fashion it chooses.

[erikras]

@kristian-puccio How about:

validate(values, setError) {
  if(!values.name) {
    setError('name', 'Required')
  }
  if(!values.shipping.street) {
    setError('shipping.street', 'Required')
  }
  // no return
}

That has been considered before as well.

I do like the idea of providing a helper to convert between the two formats to minimize migration pains.

---

Edit: Wow, so the 👎 button works, too. 😆 It wasn't really a serious suggestion.

[kristian-puccio]

Interesting, feels sort of funny to not return something when redux is
pushing towards a more functional style of coding.
Nothing wrong with the approach just feels different.

Once of the things I do is I have a validate function that validates all my
forms as all my forms are all generated by an array of objects.
So I pass a bunch of data into a function and get back an object of the
errors.
I guess I would just pass that helper function the setError function and it
would use that instead of creating an object.

Would you still use _error as a key to setError to set form level errors?

On 18 August 2016 at 22:09, Erik Rasmussen [email protected] wrote:

@kristian-puccio https://github.com/kristian-puccio How about:

validate(values, setError) {
if(!values.name) {
setError('name', 'Required')
}
if(!values.shipping.street) {
setError('shipping.street', 'Required')
}
// no return
}

That has been considered before as well.

I do like the idea of providing a helper to convert between the two
formats to minimize migration pains.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1562 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACA_f6xqWvpQBMpXLc7GQAHor50x9pSDks5qhEt6gaJpZM4JnV3u
.

[pulse00]

I don't have a strong opinion regarding the flat or deep error object, but i'd like to propose that the field-level validation is taken in consideration when moving further. Right now the global validation logic is kinda awkward when you have highly dynamic and deeply nested forms.

Example: When having a deeply nested form with the structure:

{
  "name": "x",
  "companies": [
    {
      "name": "y",
      "addresses": [
        {
          "street": "z"
        }
      ]
    }
  ]
}

you basically have to rebuild the whole object hierarchy when validating the form. Now when you have the validation logic stored somewhere in the backend for each field to be applied dynamically, that's quite some pain to build the validation object. Example for a dynamic validation coming from the backend:

[
  {
    "name.companies.name": {
      "required": true,
      "validator": "customValidator"
    }
  },
  {
    "name.companies.addresses.street": {
      "required": false
    }
  }
]

Whereas passing a validation callback directly to each field would simplify this approach a lot.

I don't know the internal logic of redux-form well enough to judge what approach regarding flat or deep error objects whould help the field level validation, but i'm in favor of whatever option helps :)

[ruiaraujo]

Just release v6 already!

My vote was all about that. Let's make redux-form great again! 😈

[clayne11]

Having a helper function makes sense to me to help with the migration, but I don't think we should use that setError function you showed above. Let's stick with returning the error at the end of the function to keep things pure and simple

[babsonmatt]

If flattening the errors doesn't add much time I'd say just do it now.

[smirea]

I would personally prefer the deep format as I find it much easier to work with and rationalize about an error result that has the same structure as my input (or as similar as possible)

How about an alternative error format that has a set structure? Example returning an object with {$error: *, $values: *} should pass the $errorto the validation parent and continue traversing on the $values

Example given the following structure:

{
    name: String,
    price: Number,
    stores: [{
        city: String,
        zip: Number
    }]
}

The validation method could look very similar to the input:

const validate = ({name, price, stores}) => ({
    name: name ? null : 'Name is required',
    price: isNumeric(price) ? null : 'Invalid Price',
    stores: {
        $error: stores.some(obj => !!validateStore(obj)) ? 'Invalid stores, check the field errors' : null,
        $values: stores.map(validateStore),
    },
});

const validateStore = ({city, zip}) => ({
    city: city === ('' + city).toUpperCase() ? null : 'CITY MUST BE UPPERCASE',
});

const isNumeric = num => !Number.isNaN(parseFloat(price));

The format can easily be improved with a helper function that returns the error only if one of the children is invalid (though implementing this could be an exercise to the user)

const validateArray = (arrayError, arr, validateValue) => {
    const result = arr.map(validateValue);
    const hasError = result.some(obj => obj && Object.keys(obj).some(key => !!obj[key]));
    return !hasError ? null : {
        $error: arrayError,
        $values: result,
    };
}

Using the above would result in the entire validation looking like:

const validate = ({name, price, stores}) => ({
    name: name ? null : 'Name is required',
    price: isNumeric(price) ? null : 'Invalid Price',
    stores: validateArray('Invalid stores, check the field errors', stores, validateStore),
});

const validateStore = ({city, zip}) => ({
    city: city === ('' + city).toUpperCase() ? null : 'CITY MUST BE UPPERCASE',
});

const isNumeric = num => !Number.isNaN(parseFloat(price));

EDIT: I also think this would have a much less performance impact than any flattened since there wouldn't have to be any key parsing

[bradwestfall]

I think flattening them might be a mistake. It seems like it will require the api to make assumptions about error format we want back. In a way, it seems like it aggregates the errors which means we'll lose details for the sake of making displaying the errors easier. Keeping the errors in a more verbose "deep" format allows me to flatten them myself the way I want to in a way that makes sense for my app instead of having an abstraction do it for me.

And I think the helper method idea works, but the default should be to keep the errors in the more deep way, then the helper can flatten them.

[bradwestfall]

I'm actually developing against 6-rc-4 (which I really like btw, great job) and correct me if I'm wrong, but the errors for each field are delivered in the exact same format as validation returns them. So if I were to hardcode some errors for the sake of example:

validate: (values) => {
  return {
    email: {'foo': 'error message one', 'bar': 'error message two'}
  }
}

... then inside the <Field name="email" ... /> component I would get {'foo': 'error message one', 'bar': 'error message two'} for it's props.errors -- at least that's what I'm seeing as I'm testing it right now. So what would that look like flattened?

This is actually one of the great things about redux-form, it's hands-off approach when it comes to messing with the validation output I decided on. I think most people are probably going to bring in a validation tool anyways, I just feel like it's the validation tool's job to flatten errors if the developer chooses to do it a certain way, not redux-form

[clayne11]

@bradwestfall redux-form itself decides on the error format, that's the point of this discussion. There no assumption, it's an opinion.

Most of the time, in the case you're describing you would have a separate field for each email.foo and email.bar. Each field would get it's own error. The only thing this change proposes is how the errors are stored and internally propagated to the components. There's no changes being made at the component level, only at the validate level.

Instead of your example, you would return:

validate: (values) => {
  return {
    'email.foo': 'error message one', 
    'email.bar': 'error message two',
  }
}

The errors still get sent into the email.foo component and email.bar component in exactly the same way.

The issue with the flattened structure that I didn't see until now is that if you do use an object level component, in your example email, it's more expensive to round up all of the errors for that field because we now have to do a prefix search through all of the keys to pass back the full object of errors, whereas in the deep structure we can pass the entire error object down to the email component.

In these cases, maybe the better option is now to use the Fields component to pass down the multiple errors that you want for that object level field.

[bradwestfall]

@clayne11 Thanks for the clarification on "no changes being made at the component level"

But also, my error format wasn't intended to be two fields. Maybe I shouldn't have used "foo" and "bar", here's a more real example

validate: (values) => {
  return {
    email: {'required': 'Email is required', 'domain': 'We only allow .edu emails'}
  }
}

I was trying to illustrate two errors of the same field

[clayne11]

I'm not sure if redux-form actually supports object errors at all at the moment. I know it used to only support strings.

That being said, nothing would change in that case. We're not saying you have to flatten the error objects themselves. We're talking about flattening the error keys not the error values.

Error values are purely in user-land, whereas as error keys are part of the library's design decisions.

[bradwestfall]

Whether intended or not, it does support the use case I'm talking about, which is what I love about it. Thanks again

[bmv437]

My only concern with flattening is that will make you have to construct those keys during every validation loop. For big forms, the validate() function needs to be as performant as possible and I feel it will end up impacting the performance of the validate function.

Someone please correct me if I'm wrong here.

[clayne11]

In what way will this change affect performance? Either way you have to loop through all the values and construct an object of errors. I don't see any difference in performance.

[tacomanator]

As mentioned in #935 there are issues with the deep structure around handling simultaneous errors on a FieldArray and one or more elements inside the FieldArray. While there are trade-offs, at least the flat structure would solve this.

That said is the option to pass a validate method directly to each field (mentioned by several folks) also being considered? If you're going to change this, I feel it would be best to consider all options to minimize the chance it will be changed again.

Finally, I haven't dug into the new Fields component so forgive me if this is completely wrong, but my gut says each potential solution should be thought through w.r.t. this as well.

[smirea]

@clayne11 with a deeply nested object, say: {foo: [{bar: [{baz: [{a: 1, b: 2}]}]}]}, flattened it would be:

    'foo': error,
    'foo[0]': error,
    'foo[0].bar': error,
    'foo[0].bar[0]': error,
    'foo[0].bar[0].baz': error,
    'foo[0].bar[0].baz[0]': error,
    'foo[0].bar[0].baz[0].a': error,
    'foo[0].bar[0].baz[0].b': error,

each of those keys would have to be parsed, potentially using RegExp, which can considerably increase the time it takes to validate. This would be a problem as it would happen on every keypress

[clayne11]

You don't have to parse them - you're creating them. And they don't have to be parsed byredux-form either, as the key will be an exact match to the name of the field it's grabbing the error for.

[smirea]

oh, valid point

[bmv437]

Thanks @smirea and @clayne11, I see what you mean, you're setting, not getting. But you still need to build the key string when building the errors object right?

[clayne11]

That's correct, you will need to build the string yourself, but I don't think that's a performance issue. It's just a different implementation of the validate function than what we currently have.

[smirea]

What also worries me is that flattening the object also breaks the concept of separation of concerns between different validation methods.

With nested you could do:

const validate = ({name, stores}) => ({
    name: validateName(name),
    stores: stores.map(validateStore),
})

In the above, every method only knows about the input is given and their result only affects the input.

Using a flattened error object, this separation breaks and it's possible for one method to overwrite the errors of any other fields:

const validate = ({name, stores}) => ({
    ...validateName(name),
    ...validateStores(stores),
})

Yes, you can build helper methods that pass the context and prepends it to the resulting keys, but this approach creates an ability to shoot yourself in the foot where previously there was none.

[tacomanator]

@clayne11 It's not just building the string, redux-form has to then parse them too. Can't say how much this would impact performance if barely at all, but worth considering.

In the deep solution, redux-form might be able to optimize an error check for a changed field simply by checking if the corresponding object key exists and is not empty. In the flat solution, every string would have to be checked, on each keypress etc.

update: I missed your comment about not having to parse them in redux-form. This is true (duh), but redux-form still needs to then build the string to check against.

[bmv437]

What would be the best way to build those keys? Because it sounds like a mess.

Also, regarding what @pulse00 said, what does everyone think about field level validation functions?

[clayne11]

update: I missed your comment about not having to parse them in redux-form. This is true (duh), but redux-form still needs to then build the string to check against.

This always happens anyways. Whenever an error object changes each connected field grabs it's error out of the store and if the error has changed it will re-render the component.

Each field knows what it's name is, that's how the field knows where to get it's value and error from in the first place.

[clayne11]

@smirea This is true, it's a trade-off. The flattened structure makes error on array and objects first class citizens of redux-form, rather than an after thought, but you do lose the ability to "namespace" the errors.

I don't think it's a big issue to have one validation function overwriting the results of another another one though. That seems like a pretty unlikely bug.

[clayne11]

With regards to field-level validation functions, I think we all agree they're a good idea, but they're outside of the scope of this discussion. They would be something that would be nice to have in addition to the top-level validation function and require a fairly significant amount of work to add.

[tacomanator]

@clayne11 but redux-form stores data as a deep object. I have a hunch it's not as cut-and-dry as a simply matching object key, but haven't dug into the implementation enough to know.

[clayne11]

I've had several PRs merged in and I know the codebase very well. I'm confident that it is that cut-and-dry.

[tacomanator]

@clayne11 ok then I'll take your word for it on that one then.

Regarding field-level validation functions, I don't see why this would be good to have in addition to top level validation. What happens if you try to use both? Which wins? That seems confusing. I wouldn't favor supporting both unless there is a really good reason to do so (and I can't think of one), which is why I think it should be part of this discussion.

[clayne11]

Fair enough. Personally I'm not a fan of using field-level validation. When I'm building a form I typically have a certain set of data that I'm gathering from the user and validating. A component above the top-level form is responsible for taking that data and submitting it somewhere (typically my server).

The fields are simply a means of getting the data that I'm looking for. Having all of the validation logic together allows for:

1. Validating one field with respect to another since all of the values are passed into the validation function.
2. Defining a single validation function rather than splitting out the validation as a prop into every single field component. Adding another prop to every field seems pretty onerous in a large form.

IMO forms need a higher-level abstraction to coordinate the validation and submission functions of the form. At the end of the day, you're trying to gather say a user email and password or an application for a business loan. You need to ensure that the entire form is valid, not that a specific field has the correct value.

In terms of separation of concerns, I think it makes sense to keep the validation logic in one place. It makes it easier to test your form's schema holistically and also is easier to refactor the inner form structure without worrying about validation.

[tacomanator]

@clayne11 thank you.

Validating one field with respect to another since all of the values are passed into the validation function.

This is an important issue, but could be solved by passing the full values as a second parameter. Not sure if this causes a performance hit.

Defining a single validation function rather than splitting out the validation as a prop into every single field component. Adding another prop to every field seems pretty onerous in a large form.

I both agree and disagree. On the one hand, adding it to each field means you don't have to define the fields in two places: the field itself and in the validate method. On the other hand, it could easily be abused with lots of complicated, inlined validation functions.

In terms of separation of concerns, I think it makes sense to keep the validation logic in one place. It makes it easier to test your form's schema holistically and also is easier to refactor the inner form structure without worrying about validation.

Agree.

How about performance?

A a single validate function, all fields are validated on every change. As individual functions, only the field being changed need be validated. Right?

[davidkpiano]

Adding another prop to every field seems pretty onerous in a large form.

Why? That's (basically) how it works in normal HTML...

<input type="text" required pattern="..." />

The input control above has two validators defined inline: required and pattern. The idea of keeping validation in a single place, defined at the beginning is a fallacy for a couple reasons:

• Async validation (which is "solved" I suppose)
• No way to isolate validation for only the field that changed
• No way to have dynamic validation after form initialization (this is not an uncommon use-case!)

I proposed this a while ago: #98 Having field-specific validation functions would be a huge performance improvement, because validation doesn't need to be run for fields that haven't changed, nor for fields that don't have validators.

[naw]

If the library is agnostic about the values of the errors as mentioned here, and if the user chooses to make those values arrays (of strings, for example), then it may be difficult to build the helper mentioned here because the helper wouldn't know whether a given array it encounters represents an array of errors for a field or represents an array field. Unless the helper has some prior knowledge of the form structure, that is.

Flat or deep, I hope the final solution will be agnostic enough to accommodate non-string errors such as arrays.

[erikras]

Okay. Thank you all for voting. At the time of this comment, it's a pretty resounding landslide, at least for the present course of action:

{
  no: 34,
  yes: 9
}

How else would I format the results? 😆

I'm going to leave a few more days for v6rc5 testing, and then .

[gaearon]

I would be wary of adding more indirection via strings (unless that’s the route you’re already taking I guess). This makes the library more incompatible with static typing (Flow or TypeScript).

[andrewmclagan]

plus 1 for no... if it means anything now.

[wmertens]

So if the only real problem is arrays in Immutablejs, how about only allowing objects?

const errors = {
  _error: 'This is a form wide error',
  test: {
    _error: 'Awkward special rule'
    foo: 'This is an error',
    bar: 'This is another error',
  },
  foobar: 'An error',
  anArrayField: {
    _error: 'Must have 3 entries',
    0: {
      test: 'This is not a valid value'
    }
  },
}

[ShockiTV]

Well, for me the text error information is just error value and I dont see differences between objects and arrays, for me it is still object.
And I like to strongly differentiate between structure information and actual values.

const errors = {
  value: 'This is a form wide error',
  children: {
    test: {
      value: 'Awkward special rule',
      children: {
        foo: {
          value: 'This is an error'
        },
        bar: {
          value: 'This is another error'
        }
      }
    },
    foobar: {
      value: 'An error'
    },
    andArrayField: {
      value: 'Must have 3 entries',
      children: {
        0: {
          children: {
            test: {
              value: 'This is not a valid value'
            }
          }
        }
      }
    }
  }
}

Of course my own internal structure would be normalized, but people dont like to read and connect things in structure referenced by numbers. So this is user facing structure.

Maybe even enable multiple errors per structural object, as they can happen

const errors = {
  values: {
    0: 'This is a form wide error',
    1: 'Second form wide error'
  },
  children: {
  ... 
  }
}

[jedwards1211]

@erikras it seems to me like redux-form could easily support both ways of doing it simultaneously, even mixed together in the same object:

new SumissionError({
  list: [
   'required'
  ],
  'list[1]': 'required',
})

(would mark both list[0] and list[1] as required)
Basically you could try to find the deeply nested error first, and check the flat path if that doesn't work.
(Having a field named list[1] is already not viable with redux-form anyway, right?)

[jedwards1211]

@erikras I've been thinking about how to make a more systematic, future-proof format for communicating validation errors between server and client that's immune to edge cases.

• The problem with _error in nested objects is if you need to communicate validation for a field named _error for any reason.
• The problem with string keys is if you need to have a field named list[0] for whatever reason.

The only ways around this are to use either something like @ShockiTV's proposal, or array paths:

[
  {path: ['list', 0], error: 'required'},
  {path: ['list', 1, 'count'], error: 'must be a number'},
]

[dhurlburtusa]

Did any code changes come out of this discussion? Was a flattening function added?

I don't have a strong opinion about the error structure, but what I would like to see is some easy way I could get an object where its keys match exactly the name attribute of the invalid input controls of the form. Then I can easily get a reference to the input control and do something with it like set focus on it.

If there isn't a built-in way to convert the error structure (as returned by onSubmitFail) into the flattened structure mentioned above, I'll be glad to create one. (Actually, I already created it. See #2365.)

[bbthorntz]

Not sure if this is still ongoing, but another +1 for flattening the errors. We receive the following format from an API when validating an array of items (for example, domain names):

{
    "domains": "Needs at least 5 domains",
    "domains.0": "Invalid domain",
    "domains.1": "Invalid domain"
}

As previously discussed, this type of object doesn't flatten easily and makes it difficult to display errors correctly on our UI.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.