JSON Schema Validation helper method

Question

This is what I wrote to validate JSON to the given schema.

public void validateJsonSchema(ObjectNode jsonSchema, ObjectNode json) throws InvalidRequestException {
 String errorMessage = null;
 if (json != null) {
 if(jsonSchema == null) {
 errorMessage = "json exist in request but there is no schema to validate it against";
 }
 else {
 try {
 JsonSchema jsonSchema = JsonSchemaFactory.byDefault().getJsonSchema(jsonSchema);
 ProcessingReport processingReport = jsonSchema.validate(json);
 if (!processingReport.isSuccess()) {
 StringBuilder sb = new StringBuilder();
 processingReport.forEach(jsonError -> sb.append(String.format("[%s] ", jsonError)));
 errorMessage = String.format("json validation failed. Errors: %s", sb.toString());
 }
 } catch (ProcessingException e) {
 errorMessage = String.format("json validation threw ProcessingException. Error Message: %s", e.getMessage());
 }
 }
 }
 if (errorMessage != null) {
 throw new InvalidRequestException(errorMessage);
 }
}

Here are few things that I had in my mind while writing this code:

1. I don't want to return anything from the method. Schema is either valid or invalid. In case of invalid, I just throw an exception. Since it's part of a Helper class, I think it's okay to put some business logic (throwing exception) with in this code.

2. I like 1 point of return or 1 point of exception throwing. Thats why I am using errorMessage to capture any error and if it exists, throw it at the end. By this approach, the code readability is much better.

3. The jsonSchema and json both are optional. In case if json doesn't exist, there is no point of getting into validation.

Answer 1

Contrary to Julien Rousé and you, I'm rather against the "1 point of return" policy. It doesn't make it more readable. It might make it a bit easier to formally prove correctness of the code, but I have never seen someone do that outside of an academic context.

As it is now, when looking at what would happen if the json == null you'd first encounter the if(json != null) line. Then you scroll down 20 or more lines to find the end of that if block finding that there's no else block. But the method doesn't end here yet. There's another if (message != null) line where I forgot if I had encountered any initialisation in the meantime so I have to go over the entire method again to see if it is null or not ...

Compare that with a method that starts with:

public void validateJsonSchema(ObjectNode jsonSchema, ObjectNode json) throws InvalidRequestException {
 if (json == null) {
 return;
 } 

By writing in this "return as early as possible" style, you immediatly see that empty json objects are always valid. If at some point it's clear what the result of the method should be, then I want that to be clear explicitly at that point. I don't want to tire myself constantly looking up and down the method to see if I didn't miss anything (for example: was this message really still null when we got here?).

That's why I would prefer to structure the method like this:

public void validateJsonSchema(ObjectNode jsonSchema, ObjectNode json) throws InvalidRequestException {
 if (json == null) {
 return;
 }
 if(jsonSchema == null) {
 throw new InvalidRequestException("json exist in request but there is no schema to validate it against");
 }
 try {
 JsonSchema jsonSchema = JsonSchemaFactory.byDefault().getJsonSchema(jsonSchema);
 ProcessingReport processingReport = jsonSchema.validate(json);
 if (!processingReport.isSuccess()) {
 StringBuilder sb = new StringBuilder();
 processingReport.forEach(jsonError -> sb.append(String.format("[%s] ", jsonError)));
 throw new InvalidRequestException(String.format("json validation failed. Errors: %s", sb.toString()));
 }
 } catch (ProcessingException e) {
 throw new InvalidRequestException(String.format("json validation threw ProcessingException. Error Message: %s", e.getMessage()));
 }
}

I'd say this is more readable and especially easier to find out what the result is of the special cases. Not that I had any trouble reading/understanding your code ofcourse. I just prefer the way that takes a little bit less effort to understand completely.

Answer 2

You wrote a bullet list so I try to comment each bullet separately

1. I have mixed sentiment about this. I do agree with this stack overflow post, I'll quote

   An exception is thrown when a fundamental assumption of the current code block is found to be false.

   Here you want to validate a Json schema, so your code validate it or not, but if the parameter for JsonSchema is null it goes contrary to what is expected so I believe you are right to throw the exception.

   But for the same reason, I believe you should not return an exception when processingReport.isSuccess() returns false. My take on this would be to return true of false to indicate if validation is ok or not. That might have to do with personal taste.

2. I like it the same way, but sometimes it's good to throw an exception as soon as you can. Here I it doesn't really matter, your code is readable and understandable, do as you please.

3. I see nothing wrong with the fact that if json is null you don't check for jsonSchema. The only thing maybe I would be wary of is because you don't return a value, you consider json == null the same as a valid json, which might be a bit confusing.

• Em Ae: thanks for the input. Regarding point 1. If i return true or false i would nee a mechanism to let callee know what were json errors. In my case, i need to tell clients that json was invalid because of these errors. thats why i am building an error message and throwing an exception which contains those errors. regarding 3: This is correct assumption. Since if json is null, the rest of the business logic should continue. the json is an optional parameter to api. so a null json means that there were no validation errros so we can continue to process remaining request.
• Julien Rousé: Maybe you could return an object containing a boolean for knowing if validation is true or false and a list of error if needed? This way exception would be thrown only when something exceptional happen, as I believe they should be. Or maybe an another way of returning your errors without using exception? (Your solution is fine, I'm nitpicking a bit since you asked a review ;) )