Help witt RAD-359 Improving validation error messages.

Development
1

Hi all

I have been working on this issue for a while now and its getting frustrating. Due to my internships I can only work a few hours a day and spending days on one particular issue is getting me frustrated.

On import of MRRT templates they are validated against an xsd file to make sure they follow the MRRT standard. Recently we noticed that the xsd validation stops on the first violation and reports that error to the user as a sax parse exception so the user can handle. Which means if a file has 20 violations, they user might have to retry to import the template twenty times for the validator to report all 20 violations. To allow the validator to continue checking for violations until its found all before reporting, so that the user can fix all at once before reimporting the template, I wrote this code.

openmrs-module-radiology/XsdMrrtReportTemplateValidator.java at RAD-359 · ivange94/openmrs-module-radiology · GitHub

In that commit, the code checks for all violations, adds them to a list of violations then at the end throws an exception that reports all the violations, not just the first. Well, the requirement was to wrap the list of exceptions into a single exception but I wrapped just the error messages.

if (!exceptions.isEmpty()) {
    throw new APIException(exceptions.toString());
}

I can’t figure out how to rap the list of exceptions into a single exception as I cannot pass List into the constructor of the Exception.

This validator is used the MrrtReportTemplateFileParser to validate the template files before creating MrrtReportTemplate object. You can see code here openmrs-module-radiology/DefaultMrrtReportTemplateFileParser.java at RAD-359 · ivange94/openmrs-module-radiology · GitHub

When a file does not meet the mrrt standard and hence validation fails, the error is reported using

catch (APIException exception) {
            request.getSession()
                    .setAttribute(WebConstants.OPENMRS_ERROR_ATTR,
                        "Failed to import " + templateFile.getOriginalFilename() + " => " + exception.getMessage());
  }

Here is a sample output of trying to import a file with two validation violations

Screen Shot 2016-09-30 at 8.53.56 PM.png2852×300 108 KB

Issues with current implementation

  1. Output is not user friendly: All that output is just trying to say that the file does not have ‘title’ and ‘template_attribute’ tags which are mandatory.

  2. I wrapped the exception messages inside the a new exception instead of wrapping all the exceptions objects inside the new exception. Well I have google and I have not figured out how to do this. Maybe I am not understand what @teleivo trying to say.

I would need some help in reporting that error on the UI so that it looks more user friendly and also how to wrap a list object inside one exception. I have tried googling but I don’t get anything useful.

Quote @wyclif “Why not use spring validator instead of implementing your own validator?”

Well I tried to use spring validator but I couldn’t figure out how to integrate the code with the spring validator. From what I know spring validator validates an object to make sure its valid. So every object that needs validation, say Report, will need a ReportValidator. But I don’t think that will work here because an MrrtReportTemplateValidator will be validating an MrrtReportTemplate to make sure it is valid. But am not sure this will work because my validator validates an html file against a schema and if its valid parse the contents of the file and then instantiate an MrrtReportTemplate. My point is my validator is called manually and is called even before an MrrtReportTemplate object is validated. Look at this line,

2

As I mentioned earlier this is very achievable with spring validators, I understand you don’t have to since you can create your own validation and error message binding and display mechanism in the UI but it would be a waste of time trying to reinvent something already provided out of the box by a framework like spring that you already have on your classpath anyways. I want to think you might be overthinking and complicating things because this should be fairly trivial if you’re familiar with the spring framework, it’s validation and error binding mechanism.

3

There’s also something you’re not mentioning in your question, is the template you’re validating just a schema or it’s one with data elements that have been input by the user? Because in case it’s one that has data elements that have been input by the user then I think you might want to bind an error message to its respective field so that it gets displayed alongside the input with an invalid value but this is being fancy.

Another thing is that you should really not be throwing exceptions from a validator because then what’s the point of the validator? If you want to switch to a spring validator in theory, validators should just push the error messages into the errors object as opposed to throwing exceptions.

Another really straight forward work around is to wrap the validation call in a try…catch clause in the controller and just push the ‘pretty’ error message from the thrown exception into the http session so that the openmrs framework displays it for you on the next page, you can see how this is done here .

The other thing is that, it also depends on the kind of controller you using i.e annotated vs the old one that extends SimpleFormController, for an annotated controller the validator is called by your code so why not call the validator when the object you wish to validator has been populated otherwise you seem to be doing something wrong.

4

You can create a new class that extends APIException and add a list of objects containing the errors. An exception is a normal Java class, you can put in it whatever you want. Check SAXParseException to see if it’s possible to extract the important message, but XSD validation errors use to be cryptic. Try different parsers.

5

Its an html file(or you can call it schema). It does not have any data entered by the user.

Different templates have different fields. And I cannot know before hand what fields are present in the template. All I can check is if the template follows a particular structure. So I don’t see how I can bind error messages to fields.

I am not validating the fields of a form submission. The system imports mrrt template files which are html files. All I am checking is to make sure the html file been imported follows the mrrt standard, like say it must have a ‘title’ tag or must have one ‘meta’ tag denoting the charset used in the template file.

6

Then I don’t think you should bother with spring validators because it’s more applicable to Java beans. You could just maintain a list of error messages in your controller and add the error messages to the model if any are found, then in your form you check if the list is not empty and you list them, see the snippets below:

Controller (I’m assuming this in the legacy UI)

List<String> errorCodes = new ArrayList<String>();
try{
   //import template...
   .................................
   //validate the template
   if (some check){
       errorCodes.add("some.check.failed"); 
   } else if (another check){
       errorCodes.add("another.check.failed");
   }
} catch(SomeParseException e){
   // Alternatively you could add this to the errorCodes list
   request.setAttribute(WebConstants.OPENMRS_ERROR_ATTR, "some.error", WebRequest.SCOPE_SESSION);
}
model.addAttribute("errorCodes", errorCodes);

Then in your JSP you could have this may be at the top

<c:if test="${fn:length(errorCodes) > 0}">
    //print the translated error messages
</c:if>
1 Like