Metadata Validation Service

[jbee]

This is an very early idea stage draft - mostly a collection of thoughts and ideas around a solution to make validation in DHIS2 more coherent and easier to understand and maintain.

Motivation

The main motivation is that the current metadata validation takes place on different levels using different patterns. This also provides much surface area for inconsistencies in the validation that takes place. It is mostly dependent upon the specific code path used to apply changes.

The majority of validations take place when using the importer, but there is also validation in service methods and controllers.
As the importer does not use services to do save/update or delete service level validation is passed by when using the importer.
The other way around when using the services importer validation is passed by. To make matters worse all validation is passed by when using the entity manager directly. This is more common than one might think as controllers at times use the entity manager to apply their changes after doing validation in the controller.

The reason such mixed patters did arise is that the validation used currently is tied with the operation. There is no way to do "importer validation" without doing an import or "service validation" without doing a save/update using the service. Frequently validation should occur before any actual manipulation is done which then is sometimes solved by adding validation to the controllers.

Goals

• Make validation be applied consistent and uniform in how it is encoded
• Make sure validation does always occur (independent of code path)
• Make it easier to understand when which set of validation logic does apply
• Make it easier to understand where to add or how to encode metadata validation logic
• Make it easier to find all relevant validation logic for a specific metadata object type

Non-Goals

• It is not the goal of this suggestion to replace any and all validations in DHIS2 with a generic common pattern or service.
  This is a proposal specifically for metadata object validation.
• The error responses generated by our API should remain the same as much as possible. In particular the responses already giving a detailed report like for example the importer should stay the same. For individual object modification the actual message is likely to change as more validation will apply using the errors used by importer. Making validation consistent will also make errors consistent which must mean the API responses will change but the way this is implemented should keep the change to those individual modification methods that currently do not use the importer or similar operations that already result in a detailed error report.

Challenges

How to make sure validation always occurs?

Idea: Validation is hooked in on store level when running a store create/update/delete. As validation sometimes also needs to occur earlier in the overall operation the affected objects could get a transient boolean validated which the validation sets to true once the objects were validated. This way if validation had been applied the store skips the validation avoiding doing the same validation twice.

Implementation

I made a draft PR to sketch the API https://github.com/dhis2/dhis2-core/pull/12103/files

[ismay]

Nice, that sounds good to me. The validation has seemed a little inconsistent to me at times, so this would be a welcome change from my perspective.

The error responses generated by our API should remain the same as much as possible. In particular the responses already giving a detailed report like for example the importer should stay the same. For individual object modification the actual message is likely to change as more validation will apply using the errors used by importer. Making validation consistent will also make errors consistent which must mean the API responses will change but the way this is implemented should keep the change to those individual modification methods that currently do not use the importer or similar operations that already result in a detailed error report.

Consistency is always good. For me, the most important things for a convenient validation response would be:

1. An http status code that's not in the success range
2. A response shape where the errors are easy to match to their respective fields (if that's applicable). The current errorResponse object can be a little inconsistent/inconvenient at times.

Let me know if my feedback does not apply to metadata validation, btw. I'm mostly basing this on the validation for dhis2 endpoints I've worked with in the past, and I can imagine that that does not translate 1-1 to metadata validation.

It is not the goal of this suggestion to replace any and all validations in DHIS2 with a generic common pattern or service.
This is a proposal specifically for metadata object validation.

I would love that though, as a long term goal separate from this proposal.

[jbee]

@ismay Thanks for your input. Always good to get another perspective.

An http status code that's not in the success range

I agree, yet our APIs so far had been designed so that endpoints that return an error report usually do so with a success status. Any change to this would be a breaking change. While I would welcome it in this case it is unlikely we will do it.

A response shape where the errors are easy to match to their respective fields (if that's applicable). The current errorResponse object can be a little inconsistent/inconvenient at times.

Please elaborate or give an example of what makes this difficult or how you would want it to be structured.

I would love that though, as a long term goal separate from this proposal.

Me too, but we have to start with a reasonable scope. We need to avoid introducing yet another way of doing validation in addition to the existing ones. Therefore the idea is within metadata (biggest part of platform) to switch all validation to so that we don't have a "old" and a "new" way simultaneous. At least within some modules of the codebase there will only be one way of doing validation. If this is a success other modules can adopt it later.

[ismay]

I agree, yet our APIs so far had been designed so that endpoints that return an error report usually do so with a success status. Any change to this would be a breaking change. While I would welcome it in this case it is unlikely we will do it.

Yeah, I understand. Would be good for a future issue then.

Please elaborate or give an example of what makes this difficult or how you would want it to be structured.

So I've used the errorProperty from the errorResponses in the past to link a validation error to a specific form field in the ui. The errorProperty is sometimes missing for an error that seems to clearly relate to a field. It would be nice to make sure that validation errors that relate to a field always have the errorProperty. That way we can show the error close to the field, which is nice for the user, then they know what field has failed validation.

Me too, but we have to start with a reasonable scope. We need to avoid introducing yet another way of doing validation in addition to the existing ones. Therefore the idea is within metadata (biggest part of platform) to switch all validation to so that we don't have a "old" and a "new" way simultaneous. At least within some modules of the codebase there will only be one way of doing validation. If this is a success other modules can adopt it later.

Yeah I see what you mean. A certain xkcd comes to mind 😅, your approach sounds like a good way to avoid adding another competing standard 👍️. Good to know that you're aiming for the same long term goal though!

Thanks for opening this up by the way. Good to talk about these kinds of things!

[jbee]

Yeah I see what you mean. A certain xkcd comes to mind

Exactly. We fell for that before so we have multiple "standards" already in the validation that follows a pattern or interface. If we do something we have to move those over to a single new standard which also can satisfy all the use cases where validation is more done in an ad-hoc way because the more standardized ways were not accessible for some reason.

[ismay]

Yeah, yeah that sounds good 👍️

[mortenoh]

I agree, yet our APIs so far had been designed so that endpoints that return an error report usually do so with a success status. Any change to this would be a breaking change. While I would welcome it in this case it is unlikely we will do it.

Yeah I think it would be great to find another solution here, but basically the reason is that we often will import multiple objects and some of them might fail, and some might be successful. We could potentially look into another solution for single object endpoints (POST/PUT to /api/dataElements/{id} for example).