If-then-else support #144
If-then-else support #144
Conversation
Covered case: there is only "if" schema.
Covered case: there is only "then" or "else" schema.
|
|
||
| private Schema ifSchema; | ||
| private Schema thenSchema; | ||
| private Schema elseSchema; |
There was a problem hiding this comment.
Please add final modifier to all members.
| } | ||
| try { | ||
| conditionalSchema.getIfSchema().get().validate(subject); | ||
| } catch (ValidationException ifSchemaValidationException) { |
There was a problem hiding this comment.
Instead of the try-catch please use the ValidatingVisitor#getFailureOfSchema(Schema, Object) method. It can be a little bit faster, since it just returns but does not throw the exception, so the stacktrace generation doesn't take time.
| try { | ||
| conditionalSchema.getIfSchema().get().validate(subject); | ||
| } catch (ValidationException ifSchemaValidationException) { | ||
| Schema elseSchema = conditionalSchema.getElseSchema().orElseThrow(() -> ifSchemaValidationException); |
There was a problem hiding this comment.
I think if the "if" schema fails and there is no "else" schema, then the input is valid, so you don't have to throw the ifSchemaValidationException. See the first schema in if-then-else.json .
|
|
||
| // if-then | ||
|
|
||
| @Test(expected = ValidationException.class) |
There was a problem hiding this comment.
Please use the TestSupport.failureOf() method to define expected failures. This lets us to specify the expected exception much more accurately than just asserting on the exception class.
You can find a couple of good examples in ArraySchemaTest
| } | ||
|
|
||
| @Test(expected = ValidationException.class) | ||
| public void ifSubschemaFailureThenSubschemaFailure() { |
There was a problem hiding this comment.
This should successfully validate (see my comment in ConditionalSchema )
| } | ||
|
|
||
| @Test(expected = ValidationException.class) | ||
| public void ifSubschemaFailureThenSubschemaSuccess() { |
There was a problem hiding this comment.
This should successfully validate (see my comment in ConditionalSchema )
Use ValidatingVisitor#getFailureOfSchema(Schema, Object) method instead of try-catch Treat schema as valid if "if" schema is invalid but there is no "else" schema
|
|
||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public class ConditionalSchemaValidatingVisitor extends Visitor { |
There was a problem hiding this comment.
@zgyorffi If you want to utilize the visitor pattern, then please follow the current convention. The visitor design pattern aims to separate the traversal and the processing of the data it handles (the schema, in this case). In this context it means that in the Visitor superclass the following methods should also exist:
visitIfSchema(Schema ifSchema)visitThenSchema(Schema thenSchema)visitElseSchema(Schema elseSchema)
And the default Visitor#visitConditionalSchema(ConditionalSchema) method should call these in the above order. This is the separated traversal step. Then you can override this 3 method in the ConditionalSchemaValidatingVisitor subclass to perform the validation. The class can have some mutable fields (eg. you will want to store the ValidationException of the IfSchema, if it exists), it won't hurt thread safety, because from the caller side there is only a local reference to the ConditionalSchemaValidatingVisitor instance.
The reason why I ask for this change is that this current implementation seemingly follows the existing visitor structure, while in fact it doesn't.
| if (thenSchemaException != null) { | ||
| owner.failure(new ValidationException(conditionalSchema, | ||
| new StringBuilder(new StringBuilder("#")), | ||
| "Data is invalid for schema of \"then\" ", |
There was a problem hiding this comment.
Please change it to "input is invalid against the \"then\" schema"
| if (elseSchemaException != null) { | ||
| owner.failure(new ValidationException(conditionalSchema, | ||
| new StringBuilder(new StringBuilder("#")), | ||
| "Data is invalid for schema of both \"if\" and \"else\" ", |
There was a problem hiding this comment.
Please change it to "input is invalid against both the \"if\" and \"else\" schema"
| // if-then | ||
|
|
||
| @Test | ||
| public void ifSubschemaSuccessThenSubschemaFailure() { |
There was a problem hiding this comment.
If you pass a Schema.Builder to the failureOf() then it will call both buildWithLocation() and also set an expected pointer. You also don't have to explicitly pass the expectedViolatedSchema if it is the same as the root schema (it matters only when we check sub-schema failures). So this test can be written as
ConditionalSchema.Builder subject = ConditionalSchema.builder().ifSchema(MAX_LENGTH_STRING_SCHEMA).thenSchema(PATTERN_STRING_SCHEMA);
TestSupport.failureOf(subject)
.expectedKeyword("then")
.input("bar")
.expect();
But if you prefer being more explicit then you can leave it as-is.
2 participants
No description provided.