Returning multiple errors as a response

81 views
Skip to first unread message

Paweł Nawrocki

unread,
Apr 3, 2017, 9:14:17 AM4/3/17
to OpenLMIS Dev
Hello everyone!

As per discussion on OLMIS-2074, I'm starting this topic to establish a standard approach to handling more than one validation error in a single response.
I've investigated what we currently do with this problem, and most of the time our approach is to take the very first error and return a message with it (code snippet from referencedata).

userValidator.validate(userDto, bindingResult);
if (bindingResult.hasErrors()) {
  throw new ValidationMessageException(bindingResult.getFieldError().getDefaultMessage());
}

Of course, we're kind of losing valuable information that could've been provided to user. As mentioned during our previous discussion, fulfillment service does handle returning multiple errors in one of it's endpoints, although the code is not appropriately styled, so it has to be refractored anyways.

if (bindingResult.hasErrors()) {
  return new ResponseEntity<>(getErrors(bindingResult), HttpStatus.BAD_REQUEST);
}

So, to start the discussion - what are the most obvious approaches that could solve this?

1. We could return another kind of exception for validation errors, that would be an array of messages. That would, however, bring some inconsistency to our API, since the response would have different structure depending on number of errors (object for single error, and array for multiple errors).

2. We could always return error messages as arrays. This would make API consistent, though this would be a major change to our APIs, while it might also be more of a complication, since still most of the time we return single error messages, and those would have to be extracted from arrays.

3. Jakub, Nikodem and I had a short discussion on this, and we agreed that a good idea might be to include an optional "field errors" array to our error responses, that would contain messages for specific fields rejected by validator. This would be sort of extension our current error messages, without breaking backwards compatibility.

So, single error message would still look like:
{ "messageKey": ..., "message": "some error occured" }
While message for multiple errors would contain additional field, for example:
{ "messageKey": ..., "message": "validation error occured", "fieldErrors": [ { "field": "fieldName", "messageKey": ..., "message": "field error description" } ] }

A drawback of the third approach would be being bound to this structure, though I think validating fields is the only place where we want to return multiple errors, so that would be fair enough.

Cheers,
Paweł

Nick Reid

unread,
Apr 3, 2017, 9:51:01 AM4/3/17
to Paweł Nawrocki, OpenLMIS Dev

I like the fieldErrors approach — its something easy enough to tack on, and I think we could make this data structure recursive if we ever had reason to (and it could be ignored by clients that don't get it)


I'm not 100% in love with using the 'fieldErrors' key name and feel we should use abstract terminology (incase we don't match a form submission... somehow)


I'd suggest the key 'details'


Our response body might look like:


{

  "messageKey": "foo.bar",

  "message": "foo",

  "details": [

    {

      "id": "some.key", // optional? not sure about this name, but could be "name"

      "messageKey": "bar.go.to",

      "message": "You are not at the bar",

      "details": [ ..... ] // yes, this could be a bad idea

    }, {

        ....

    }]

}



 



Nick Reid | nick...@villagereach.org
Friendly Neighborhood Spiderman, Information Systems Group


VillageReach Starting at the Last Mile
2900 Eastlake Ave. E, Suite 230, Seattle, WA 98102, USA
CELL: +1.510.410.0020
SKYPE: nickdotreid
www.villagereach.org


From: openlm...@googlegroups.com <openlm...@googlegroups.com> on behalf of Paweł Nawrocki <pnaw...@soldevelo.com>
Sent: Monday, April 3, 2017 6:14:17 AM
To: OpenLMIS Dev
Subject: [openlmis-dev] Returning multiple errors as a response
 
--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.
To post to this group, send email to openlm...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/7140df2b-ef19-45a8-9191-1005e0254335%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Darius Jazayeri

unread,
Apr 3, 2017, 5:30:57 PM4/3/17
to Nick Reid, OpenLMIS Dev, pnawrocki
I would expect our standard error response to look a lot like Spring's Errors/BindingResult interface.

E.g. you have global errors and field errors.

I don't see why we would genericize this to just be "details", because that seems less clear for the most common use case. (When do you ever submit something that doesn't have fields?)

-Darius (by phone)


On Apr 3, 2017 6:51 AM, "Nick Reid" <nick...@villagereach.org> wrote:

I like the fieldErrors approach — its something easy enough to tack on, and I think we could make this data structure recursive if we ever had reason to (and it could be ignored by clients that don't get it)


I'm not 100% in love with using the 'fieldErrors' key name and feel we should use abstract terminology (incase we don't match a form submission... somehow)


I'd suggest the key 'details'


Our response body might look like:


{

  "messageKey": "foo.bar",

  "message": "foo",

  "details": [

    {

      "id": "some.key", // optional? not sure about this name, but could be "name"

      "messageKey": "bar.go.to",

      "message": "You are not at the bar",

      "details": [ ..... ] // yes, this could be a bad idea

    }, {

        ....

    }]

}



 



To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.

Paweł Albecki

unread,
Nov 3, 2017, 11:58:38 AM11/3/17
to OpenLMIS Dev
Hi all,

I would like to follow-up this discussion. I see that there is an agreement that API responses need to be consistent. We shouldn't return array or errors (proposed approach 1) if there is many errors and object json if there is one error. On the other hand, returning always an array of errors (approach 2) is not very clean solution and forcing this approach would be breaking change to most of endpoints in OpenLMIS services. 

The approach 3 is backwards compatible and give us possibility to implement a solution bit by bit – starting with fill a gap in error handling conventions (http://docs.openlmis.org/en/latest/conventions/errorHandling.html#how-the-api-responds-with-validation-error-messages) and refactor requisition validators that use own, wrong pattern (more here: https://groups.google.com/forum/#!topic/openlmis-dev/I7wA6-RH_Zo). 

There is an open question if we want to have additional "fieldErrors" property in error response or if it should be more generic like: "details". I think both solutions are fine and if we can find some use case for use more generic pattern, we should go with it.   

Does it sound good for everyone? Please share your opinion.

-Paweł

Paweł Albecki

unread,
Dec 6, 2017, 5:10:05 AM12/6/17
to OpenLMIS Dev
Hi again,

The topic was on agenda of one of the Tech Committee ( 2017-11-14 ) and we decided to go with option 3 and keep backward compatibility. Flat response was preferred over nesting details within details. On style guide code review, we decided to use map of field errors instead of list. Result of agreement you can see in updated conventions.

This doesn't resolve all of our issues as flattening fields in fieldErrors (like requisitionLineItems[0].totalConsumedQuantity) could be undesirable because this require putting an array index as an argument and line item is not a unique entity. 

Now we need to decide between:

1. Nested fields solution

  "fieldErrors": {
    "requisitionLineItems": {
      "0c4b5efe-259c-44c9-8969-f157f778ee0f": {
        "stockOnHand": {
          "message": "Stock on hand cannot be negative",
          "messageKey": "requisition.error.validation.stockOnHand.cannotBeNegative"
        }, //and other field errors for this same line item go here
      }, //and different line items errors go here
    }
  }

2. Flattened fields solution

  "fieldErrors": {
    "requisitionLineItems[0].stockOnHand": {
      "message": "Stock on hand cannot be negative",
      "messageKey": "requisition.error.validation.stockOnHand.cannotBeNegative"
    }, //other field errors for this same line item and errors for different line items go here
  }


Regards,
Paweł



On Monday, April 3, 2017 at 3:14:17 PM UTC+2, Paweł Nawrocki wrote:

Nick Reid

unread,
Dec 11, 2017, 4:28:38 PM12/11/17
to Paweł Albecki, OpenLMIS Dev

So, I'd like to say that I really want the nested fields solution.... 


I've said the following comment in internal reviews, I'm just making it public on the dev list (so our discussion tomorrow can mention this) 


My only concern with the flattened fields solution is I'd like the flattened keys to be more meaningful. The current suggestion uses an indexed array in the error response, and that just worries me because its brittle. Lists can change, and the order of items in a list can change.


I would replace the array with a string-ified UUID. I'd imagine it could look like this:



  "fieldErrors": {
    "requisitionLineItems.0c4b5efe-259c-44c9-8969-f157f778ee0f.stockOnHand": {
      "message": "Stock on hand cannot be negative",
      "messageKey": "requisition.error.validation.stockOnHand.cannotBeNegative"
    }, //other field errors for this same line item and errors for different line items go here
  }

- nick -


Nick Reid | nick...@villagereach.org
Software Developer, Information Systems Group


VillageReach Starting at the Last Mile
2900 Eastlake Ave. E, Suite 230, Seattle, WA 98102, USA
CELL: +1.510.410.0020
SKYPE: nickdotreid
www.villagereach.org


From: openlm...@googlegroups.com <openlm...@googlegroups.com> on behalf of Paweł Albecki <palb...@soldevelo.com>
Sent: Wednesday, December 6, 2017 2:10:05 AM
To: OpenLMIS Dev
Subject: [openlmis-dev] Re: Returning multiple errors as a response
 

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.

Łukasz Lewczyński

unread,
Dec 12, 2017, 3:25:11 AM12/12/17
to Nick Reid, Paweł Albecki, OpenLMIS Dev
I prefer the nested fields solution. I can imagine a situation where on UI we have an array of objects but the same array is presented as java.util.Set in the backend. This can cause that the order of elements will be different than the order on UI. I think all of our elements have a ID field so there should not be a problem with that.

Also the flattened solution requires some string operations to retrieve data needed to find correct element in the DTO.


Łukasz Lewczyński
Software Developer
llewc...@soldevelo.com

On Mon, Dec 11, 2017 at 10:28 PM, Nick Reid <nick...@villagereach.org> wrote:

So, I'd like to say that I really want the nested fields solution.... 


I've said the following comment in internal reviews, I'm just making it public on the dev list (so our discussion tomorrow can mention this) 


My only concern with the flattened fields solution is I'd like the flattened keys to be more meaningful. The current suggestion uses an indexed array in the error response, and that just worries me because its brittle. Lists can change, and the order of items in a list can change.


I would replace the array with a string-ified UUID. I'd imagine it could look like this:



  "fieldErrors": {
    "requisitionLineItems.0c4b5efe-259c-44c9-8969-f157f778ee0f.stockOnHand": {
      "message": "Stock on hand cannot be negative",
      "messageKey": "requisition.error.validation.stockOnHand.cannotBeNegative"
    }, //other field errors for this same line item and errors for different line items go here
  }

- nick -


To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CY4PR02MB2199C1005EF6D0C1B8282BBD94370%40CY4PR02MB2199.namprd02.prod.outlook.com.

For more options, visit https://groups.google.com/d/optout.

Nikodem Graczewski

unread,
Dec 12, 2017, 9:32:06 AM12/12/17
to Łukasz Lewczyński, Nick Reid, Paweł Albecki, OpenLMIS Dev
Hello everyone,

the one problem I can see with identifying objects by their UUID is that not all objects must have UUID meaning we won't be able to target them this way.

Best regards,
Nikodem

Nikodem Graczewski
Software Developer

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAAdp53zDq-_ON28%3Dey7_W%3DAS0ewgPWCEi5%2B0eBKfg-cbGycLCA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

Łukasz Lewczyński

unread,
Dec 12, 2017, 9:38:47 AM12/12/17
to Nikodem Graczewski, Nick Reid, Paweł Albecki, OpenLMIS Dev
I found one example: Orderable DTO contains a list of ProgramOrderables DTO but those objects does not have a ID field. The problem here is only in the DTO representation because there is the ID field in ProgramOrderable domain class so if we add the ID field the problem will be resolved. Do you know any other examples?


Łukasz Lewczyński
Software Developer
llewc...@soldevelo.com

Nikodem Graczewski

unread,
Dec 12, 2017, 8:11:56 PM12/12/17
to Łukasz Lewczyński, Nick Reid, Paweł Albecki, OpenLMIS Dev
Hi Łukasz,

not really... This was only a thought I had in my mind.

Best regards,
Nikodem

Nikodem Graczewski
Software Developer

Paweł Albecki

unread,
Dec 18, 2017, 12:16:35 PM12/18/17
to Nikodem Graczewski, Łukasz Lewczyński, Nick Reid, OpenLMIS Dev
I think we can use any identifier there, it doesn't have to be id. For program orderables it can be program code or id as we can edit PO only through orderables (and pair Program-Orderable is a key itself).
--

Paweł Albecki
Software Developer
palb...@soldevelo.com

Reply all
Reply to author
Forward
0 new messages