Feature Request: Improved Error Details

[ravensorb]

The current error response looks like this

{
    "code": 422,
    "errors": { // <-- plural name indicates array but structure indicates single error
        "Event": { // <-- Dynamic based on entity
            "name": [ // <-- I think this is the property that caused the issue?
            { // <-- singular name indicates single instance, structure is array but array of what?
                "already_taken": "UnitTest-SaveEvent-Event-Name-781812392",
                "id": "b54fee1209004056ba511726121ef8a3"
            },
            "must_not_be_empty" // <-- no idea what this is
            ]
        }
    }
}

This information is relatively easy to read from a human perspective :) That said, it seems to be very difficult to parse consistently from a system perspective as much of it is dynamic and changes based on the error. It would be great if we could restructure the error message to be easier to handle from a system perspective

As a possible recommendation, what do you think of this?

{
    "code": 422,
    "errors": [
        {
            "entityType" : "Event",
            "errorCode" : "already_taken",
            "errorDetails" : {
                "id": "b54fee1209004056ba511726121ef8a3",
                "property": "name",
                "value": "UnitTest-SaveEvent-Event-Name-781812392"
            }
        }
    ]
}

or even better something like (this handles multiple "details")

{
    "code": 422,
    "errors": [
        {
            "type" : "Event",
            "code" : "already_taken",
            "id": "b54fee1209004056ba511726121ef8a3",
            "details" : [
                {
                     "property": "name",
                     "value": "UnitTest-SaveEvent-Event-Name-781812392"
                }
            ]
        }
    ]
}

Original thread: https://groups.google.com/forum/#!topic/structr/EXcXwuxZW0U

[amorgner]

Thanks for this feature request, makes sense. This will require some (breaking) changes in Structr, so it will probably wait until v2.0 (but 2.0 will perhaps be earlier than expected).

[cmorgner]

I'm currently working on an improvement of the structure of semantic error messages. The intended purpose was to provide machine-readable error messages that can be parsed easily. The current implementation obviously doesn't reach this goal, so I propose a new structure for semantic error messages.

The proposed structure has just two levels, and a very simple error object with four fields:

name	contents	null?
type	the type of the entity	no
property	the name of the property	yes
token	a semantic error token	no
detail	error details, e.g. the pattern that must be matched	yes

some examples

{
  "code": 422,
  "errors": [
    {
      "type": "Folder",
      "property": "name",
      "token": "must_not_be_empty",
      "detail": null
    },
    {
      "type": "Folder",
      "property": "name",
      "token": "must_match",
      "detail": "[:_a-zA-Z0-9\\s\\-\\.öäüÖÄÜß]+"
    },
    {
      "type": "Folder",
      "property": "name",
      "token": "must_not_be_empty",
      "detail": null
    }
  ]
}

{
  "code": 422,
  "errors": [
    {
      "type": "Folder",
      "property": null,
      "token": "already_taken",
      "detail": "732f69c4bea64d5d8b04250734e18948"
    }
  ]
}

What do you think?

Everyone that uses the current implementation of semantic error messages, please provide feedback about the amount of changes that would be necessary on your side to implement the new model.

Thank you!

[ravensorb]

I like it a lot! Couple of questions

• Wouldn't it make sense to have a key for the ID of the entity that triggered the error to help troubleshoot faster (it could be null for when there isn't one). Note: For a "create" maybe include the "name" property for the same reason?
• Any thoughts on including a stacktrack when debugging is enabled (sort of like an extended details)?
• Is it possible that there is more than on error per entity (example not-null error + unique constraint)? If so how would this be handled?

[cmorgner]

• If you look at the second example, the detail property contains the ID of the existing entity in the case of an already_taken response, so yes, the entity ID will be included when a uniqueness constraint is violated.
• Yes, including a stack trace is a good idea, maybe configurable in the structr.conf file. Will do that! :)
• If there is more than one error (like in the first example), there will be a separate error object for each of the tokens. This adds some overhead to the error handling method(s), but IMHO it's cleaner than grouping the errors by type like in the existing implementation.

[ravensorb]

• Ok, that makes sense however what about for other types of issues. Example, an update fails with a general error. Will the ID be included in that case? If you think about it, knowing the object ID at all times would provide a large benefit. If we need to dynamically determine based on the type of error when the detail will contain the ID, it become more difficult to handle errors consistently as it will require a lot more conditional logic on the client site (especially when multiple async requests are made).
• Excellent
• Ok, that makes sense -- if there are multiple error objects that will work although this takes me back to my first post. If there are multiple errors per object there needs to be a way to identify what entity triggered the error and still allow from some sort of context of what type of error details Example, unique constraint and pattern match error -- those would be two error objects, however based on your example there would be no way to correlate them to the same entity as the pattern match details contains the regex and not the entity ID.

[cmorgner]

I'm not sure whether the ID can be included in all cases, but it definitely seems to be a good idea! Thank you for your valuable input!

[ravensorb]

Any chance there is a an update on the new implementation? I am looking to move to production soon and the inability to effectively detect and react to errors is rather problematic (I am trying not to resort to regex to figure out what happened in the back end 😄)

[cmorgner]

The changes are part of a larger refactoring / upgrade which is available in the filesystem-browser branch. Can you try this branch, or are you using a snapshot version?

[ravensorb]

I am currently using build 87. Is it a simple swamp (build and replace)? If so, I'd be more than happy to give it a try.

[amorgner]

The new error messages format are in the master branch now, but have not yet released a new snapshot containing these.

[ravensorb]

This is awesome news! Is it pretty safe to do a build and upgrade?

[amorgner]

Yes, it's pretty stable.

[ravensorb]

Just a quick update -- I am running a build off the source and while I see the new error structure I have noticed that about 90% of the time the error result is blank. I get either a 400 or a 500 error code with nothing in the error collection.

Update: Here is a post on the google groups about the issue: https://groups.google.com/forum/#!topic/structr/rGYvOF3dEfk

[ravensorb]

Any update to this -- the number of times structr returns a blank error is rather significant which makes troubleshooting extremely difficult.

[ravensorb]

Please consider increase the error logging. A majority of the instances where an error occurs there is both no details and nothing written to the log files.

Examples:

• Invalid error results in 400 error in json response with no errors in the collection and nothing in the structr log file
• Post to create related entity with incorrectly escaped values results in a 200 success message however there is no entry to the log file and the relationship is not actually created

Basically almost all 400 and 500 errors have no details in both the response and in the structr log file.

[eric-schleicher]

• 1 to improve the quality of the logging. I just created Bug or Quirk: 500 with no reason as to why when body includes remoteRelationship that does not exist #424 which is effectively the same issue.
  while 500 are expected when the reponse is literally '500' with nothing in the log(s) either it burdensome to debug.

[cmorgner]

Closing this issue since the initial feature request ("improve error response format") is implemented. Increasing the verbosity of error messages is a different issue and can be tracked with issue #413.