We could display a generic message by default. We could reuse the includeStackTrace
property or add a new property.
Comment From: wilkinsona
IIRC, when we discussed this recently we decided that we would use a separate property.
Comment From: wilkinsona
I'm reopening this as I'm not sure about the property being named include-details
. I wonder if people may be confused by having a details property that controls both the exception message and any binding error and a stack trace property that control only the stack trace. Arguably, all three could be considered detailed information. IIRC, the problem that prompted this issue being raised was only concerned with the exception message. While I think it's a good idea to control the inclusion of binding errors are well, I wonder if we'd be better with two new properties.
Comment From: wilkinsona
Looking at https://github.com/spring-projects/spring-boot/issues/21132 again, I also wonder if a generic message for all response statuses might be a bit misleading. For example, we're now returning an error response with the message "An error occurred while processing the request" as a result of a ResponseStatusException
that maps to a 404. I'm not sure that the message is appropriate for a 404. Arguably, it's not really an error or, if it is, it's a client-side error so it didn't occur while the server was processing the request. I'm wondering if we should omit the message entirely or return a different message depending on the response status. Given that the property is about inclusion, omitting entirely is probably more inline with what people will expect.
Comment From: scottfrederick
While I think it's a good idea to control the inclusion of binding errors are well, I wonder if we'd be better with two new properties.
When the error is caused by a binding exception, the handling of the message
and errors
fields in the response becomes a bit tangled. Both fields will leak similar internal details of the application (e.g. object, class, and field names). It would be more clear to have separate include-message
and include-binding-errors
fields, but it's hard to imagine a case where a user would want to set those two flags to different values. For example, would a user want to include the message
field that gives the name of the object that failed to bind but exclude the errors
with more details?
I agree that the name is vague and requires more information to understand what it controls. Splitting it would be easy enough to implement, as would changing the name to something more clear (maybe include-message-errors
?).
I also wonder if a generic message for all response statuses might be a bit misleading.
This was debated a bit during the implementation. Previously, the message
field in a servlet error response was always present and never empty (it contained the value No message available
if the exception contains an empty message). There was concern that omitting the field or leaving it empty would break the existing contract, but the idea of a contract w.r.t. fields in a Map
is not clear to me.
After this change, the message
for a servlet response field could contain one of three hard-coded values - No message available
, An error occurred while processing the request
, or Validation failed
.
Previously, the reactive path would return an empty message
instead of No message available
when the exception contains an empty message. After this change, message
will also be empty if include-details=false
.
I'm wondering if we should omit the message entirely or return a different message depending on the response status. Given that the property is about inclusion, omitting entirely is probably more inline with what people will expect.
The choice was to keep the servlet and reactive paths consistent with past behavior w.r.t. whether message
would ever be omitted (never in either path) or empty (never in servlet path, conditionally in reactive path). We could choose instead to bring the two paths into alignment by conditionally returning an empty message
in a servlet response. Returning an empty message
makes more sense to me when an include-xxxx=false
.
I'm not in favor of returning different hard-coded messages depending on the response status, as I think other fields like status
and exception
give enough information in these cases.
Comment From: scottfrederick
After further discussion, we've decided to split the include-details
property into two properties.
- A property named
include-message
will control themessage
field. In both the servlet and reactive paths, themessage
field will be present in the output map with an empty value when the message is excluded. - A property named
include-binding-errors
will control theerrors
field. Theerrors
field will be absent from the output map when binding errors are excluded.
Both of the new properties will accept the values NEVER
, ALWAYS
, and ON_PARAM
. The include-stacktrace
property will be changed to accept ON_PARAM
in addition to ON_TRACE_PARAM
, and ON_TRACE_PARAM
will be deprecated.