Returning multiple errors as a response

Pawel_Nawrocki · April 3, 2017, 1:14pm

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?

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).
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.
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 · April 3, 2017, 1:50pm

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.reid@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: openlmis-dev@googlegroups.com openlmis-dev@googlegroups.com on behalf of Paweł Nawrocki pnawrocki@soldevelo.com

Sent: Monday, April 3, 2017 6:14:17 AM

To: OpenLMIS Dev

Subject: [openlmis-dev] Returning multiple errors as a response

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?

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).
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.
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ł

–

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
openlmis-dev@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 · April 3, 2017, 9:30pm

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.reid@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

}, {

…

}]

}

Nick Reid | nick.reid@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: openlmis-dev@googlegroups.com openlmis-dev@googlegroups.com on behalf of Paweł Nawrocki pnawrocki@soldevelo.com

Sent: Monday, April 3, 2017 6:14:17 AM

To: OpenLMIS Dev

Subject: [openlmis-dev] Returning multiple errors as a response

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?

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).

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.

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ł

–

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
openlmis-dev@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.

–

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 openlmis-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CY1PR02MB1754BEB1D5A33B3CF307270394080%40CY1PR02MB1754.namprd02.prod.outlook.com.

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

Pawel_Albecki1 · November 3, 2017, 3:58pm

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ł

···

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

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?

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).

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.

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ł

Pawel_Albecki1 · December 6, 2017, 10:10am

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:

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

}

``

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:

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?

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).

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.

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 · December 11, 2017, 9:28pm

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.reid@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: openlmis-dev@googlegroups.com openlmis-dev@googlegroups.com on behalf of Paweł Albecki palbecki@soldevelo.com

Sent: Wednesday, December 6, 2017 2:10:05 AM

To: OpenLMIS Dev

Subject: [openlmis-dev] Re: Returning multiple errors as a response

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:

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

}

``

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:

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?

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).

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.

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ł

**

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+unsubscribe@googlegroups.com.

To post to this group, send email to
openlmis-dev@googlegroups.com.

To view this discussion on the web visit
https://groups.google.com/d/msgid/openlmis-dev/e7eeacbf-187c-4b12-b904-27c620872e03%40googlegroups.com.

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

llewczynski · December 12, 2017, 8:24am

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.

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

···

On Mon, Dec 11, 2017 at 10:28 PM, Nick Reid nick.reid@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 -

Nick Reid | nick.reid@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: openlmis-dev@googlegroups.com openlmis-dev@googlegroups.com on behalf of Paweł Albecki palbecki@soldevelo.com

Sent: Wednesday, December 6, 2017 2:10:05 AM

To: OpenLMIS Dev

Subject: [openlmis-dev] Re: Returning multiple errors as a response

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:

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

}

``

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:

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?

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).

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.

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ł

**

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+unsubscribe@googlegroups.com.

To post to this group, send email to
openlmis-dev@googlegroups.com.

To view this discussion on the web visit
https://groups.google.com/d/msgid/openlmis-dev/e7eeacbf-187c-4b12-b904-27c620872e03%40googlegroups.com.

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

–

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 openlmis-dev@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.

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

ngraczewski · December 12, 2017, 2:32pm

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

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

···

On Tue, Dec 12, 2017 at 9:24 AM, Łukasz Lewczyński llewczynski@soldevelo.com wrote:

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.

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+unsubscribe@googlegroups.com.

To post to this group, send email to openlmis-dev@googlegroups.com.

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.

Nikodem Graczewski

Software Developer

ngraczewski@soldevelo.com

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

On Mon, Dec 11, 2017 at 10:28 PM, Nick Reid nick.reid@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 -

Nick Reid | nick.reid@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: openlmis-dev@googlegroups.com openlmis-dev@googlegroups.com on behalf of Paweł Albecki palbecki@soldevelo.com

Sent: Wednesday, December 6, 2017 2:10:05 AM

To: OpenLMIS Dev

Subject: [openlmis-dev] Re: Returning multiple errors as a response

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:

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

}

``

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:

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?

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).

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.

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ł

**

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+unsubscribe@googlegroups.com.

To post to this group, send email to
openlmis-dev@googlegroups.com.

To view this discussion on the web visit
https://groups.google.com/d/msgid/openlmis-dev/e7eeacbf-187c-4b12-b904-27c620872e03%40googlegroups.com.

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

–

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 openlmis-dev@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.

llewczynski · December 12, 2017, 2:38pm

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?

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

···

On Tue, Dec 12, 2017 at 3:32 PM, Nikodem Graczewski ngraczewski@soldevelo.com wrote:

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

**
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

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

Nikodem Graczewski

Software Developer

ngraczewski@soldevelo.com

On Tue, Dec 12, 2017 at 9:24 AM, Łukasz Lewczyński llewczynski@soldevelo.com wrote:

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.

**
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+unsubscribe@googlegroups.com.

To post to this group, send email to openlmis-dev@googlegroups.com.

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
Software Developer
llewczynski@soldevelo.com

On Mon, Dec 11, 2017 at 10:28 PM, Nick Reid nick.reid@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 -

Nick Reid | nick.reid@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: openlmis-dev@googlegroups.com openlmis-dev@googlegroups.com on behalf of Paweł Albecki palbecki@soldevelo.com

Sent: Wednesday, December 6, 2017 2:10:05 AM

To: OpenLMIS Dev

Subject: [openlmis-dev] Re: Returning multiple errors as a response

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:

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

}

``

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:

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?

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).

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.

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ł

**

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+unsubscribe@googlegroups.com.

To post to this group, send email to
openlmis-dev@googlegroups.com.

To view this discussion on the web visit
https://groups.google.com/d/msgid/openlmis-dev/e7eeacbf-187c-4b12-b904-27c620872e03%40googlegroups.com.

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

–

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 openlmis-dev@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.

ngraczewski · December 13, 2017, 1:11am

Hi Łukasz,

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

Best regards,
Nikodem

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

···

On Tue, Dec 12, 2017 at 3:38 PM, Łukasz Lewczyński llewczynski@soldevelo.com wrote:

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?

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

Nikodem Graczewski

Software Developer

ngraczewski@soldevelo.com

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

On Tue, Dec 12, 2017 at 3:32 PM, Nikodem Graczewski ngraczewski@soldevelo.com wrote:

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

**
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

Nikodem Graczewski

Software Developer

ngraczewski@soldevelo.com

On Tue, Dec 12, 2017 at 9:24 AM, Łukasz Lewczyński llewczynski@soldevelo.com wrote:

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.

**
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+unsubscribe@googlegroups.com.

To post to this group, send email to openlmis-dev@googlegroups.com.

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
Software Developer
llewczynski@soldevelo.com

On Mon, Dec 11, 2017 at 10:28 PM, Nick Reid nick.reid@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 -

Nick Reid | nick.reid@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: openlmis-dev@googlegroups.com openlmis-dev@googlegroups.com on behalf of Paweł Albecki palbecki@soldevelo.com

Sent: Wednesday, December 6, 2017 2:10:05 AM

To: OpenLMIS Dev

Subject: [openlmis-dev] Re: Returning multiple errors as a response

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:

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

}

``

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:

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?

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).

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.

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ł

**

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+unsubscribe@googlegroups.com.

To post to this group, send email to
openlmis-dev@googlegroups.com.

To view this discussion on the web visit
https://groups.google.com/d/msgid/openlmis-dev/e7eeacbf-187c-4b12-b904-27c620872e03%40googlegroups.com.

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

–

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 openlmis-dev@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.

Pawel_Albecki1 · December 18, 2017, 5:16pm

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).

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

···

On Wed, Dec 13, 2017 at 2:11 AM, Nikodem Graczewski ngraczewski@soldevelo.com wrote:

Hi Łukasz,

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

Best regards,
Nikodem

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

–

Nikodem Graczewski

Software Developer

ngraczewski@soldevelo.com

On Tue, Dec 12, 2017 at 3:38 PM, Łukasz Lewczyński llewczynski@soldevelo.com wrote:

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?

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

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

On Tue, Dec 12, 2017 at 3:32 PM, Nikodem Graczewski ngraczewski@soldevelo.com wrote:

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

**
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

Nikodem Graczewski

Software Developer

ngraczewski@soldevelo.com

On Tue, Dec 12, 2017 at 9:24 AM, Łukasz Lewczyński llewczynski@soldevelo.com wrote:

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.

**
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+unsubscribe@googlegroups.com.

To post to this group, send email to openlmis-dev@googlegroups.com.

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
Software Developer
llewczynski@soldevelo.com

On Mon, Dec 11, 2017 at 10:28 PM, Nick Reid nick.reid@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 -

Nick Reid | nick.reid@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: openlmis-dev@googlegroups.com openlmis-dev@googlegroups.com on behalf of Paweł Albecki palbecki@soldevelo.com

Sent: Wednesday, December 6, 2017 2:10:05 AM

To: OpenLMIS Dev

Subject: [openlmis-dev] Re: Returning multiple errors as a response

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:

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

}

``

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:

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?

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).

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.

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ł

**

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+unsubscribe@googlegroups.com.

To post to this group, send email to
openlmis-dev@googlegroups.com.

To view this discussion on the web visit
https://groups.google.com/d/msgid/openlmis-dev/e7eeacbf-187c-4b12-b904-27c620872e03%40googlegroups.com.

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

–

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 openlmis-dev@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.

Paweł Albecki

    Software Developer

     palbecki@soldevelo.com