ebi-ait / checklist

Template repository for checklists
Apache License 2.0
1 stars 0 forks source link

improve schema validation error messages #67

Closed amnonkhen closed 2 months ago

amnonkhen commented 3 months ago
ESapenaVentura commented 3 months ago

So, we've had a bit of a discussion today after investigating on how to improve our error messages.

As a summary:

We have decided to move forward with ajv-errors just for the allOf/oneOf errors to support previous ENA checklists. However, moving forward, we expect to improve the messages from the client side.

I will investigate and prepare error/solution examples.

ESapenaVentura commented 3 months ago

I've done some testing and thinking, and I've jumped to the following conclusion:

The only error that we need to define is at the level of 'allOf'. For each of the subschemas there, define an error message that states that just one of the required elements must be specified. The reason we can't have 2 types of error messages (only 1 synonym allowed and required property missing) is that the property missing will always error out at all subschemas within the oneOf, so the only solution is to use a string to overwrite all the errors to a "general" one (still specific enough to indicate which element failed and what's the failure, but not good enough to distinguish between the 2 types of errors).

I have prepared:

ESapenaVentura commented 3 months ago

It's failing to upload the file so happy to share it! Let's discuss the next steps in the meeting tomorrow, but I think since we just want this dealt with for the current schemas and we will not support it moving forward, I would suggest to just run the script that adds that error to the current schemas and move on

ESapenaVentura commented 3 months ago

Just had a chat with @amnonkhen - We have agreed to just modify the subschemas under the allOf restrictions, thus making that whenever a synonym is missing, or more than one is used, it will throw the same error of "Just one of the following properties: must be specified".

By using the 'errorMessage' keyword at this level, we also delete the extra 'oneOf', resulting in a simple, clean error that is good enough.

Thanks to @amnonkhen for the example on how to implement this:

{
    "allOf": [
        {
            "errorMessage": "Just one of the following properties: 'collection_date', 'collection date' or 'Event Date/Time' must be specified",
            "oneOf": [
                {
                    "required": [
                        "collection_date"
                    ]
                },
                {
                    "required": [
                        "collection date"
                    ]
                },
                {
                    "required": [
                        "Event Date/Time"
                    ]
                }
            ]
        },
        {
            "errorMessage": "Just one of the following properties: 'broad-scale environmental context', 'environment (biome)' must be specified",
            "oneOf": [
                {
                    "required": [
                        "broad-scale environmental context"
                    ]
                },
                {
                    "required": [
                        "environment (biome)"
                    ]
                }
            ]
        }
    ]
}

The next steps are: