Json Compatibility Checker #155
Conversation
| @@ -380,6 +380,18 @@ project('serializers') { | |||
| testCompile group: 'io.pravega', name: 'pravega-test-testcommon', version: pravegaVersion | |||
| } | |||
|
|
|||
| shadowJar { | |||
There was a problem hiding this comment.
why do we need this for json compatibility checker?
There was a problem hiding this comment.
I do not understand why would this change for the json compatibility checker, which is just a package being added to the project. I think the structure of the build.gradle file remains the same, with shading of dependencies and creation of fat jars.
There was a problem hiding this comment.
my question is why is this being added ? you are not creating a new module. and all existing projects that we wanted shaded, are already done so.
|
|
||
| import static io.pravega.schemaregistry.rules.jsoncompatibility.BreakingChangesStore.BreakingChanges; | ||
|
|
||
| public class ArrayTypeComparator { |
There was a problem hiding this comment.
add some java docs to explain what this class does
| BreakingChanges itemsValidationChanges = itemValidation(toCheck, toCheckAgainst); | ||
| if(itemsValidationChanges != null) | ||
| return itemsValidationChanges; | ||
| //TO DO: Add contains and tupleValidation |
There was a problem hiding this comment.
nit: use "todo" instead of "to do" so that the IDE can parse it and highlight it.
also any TODO in the code should either be accomponied by a github issue OR it should be implemented before the code is merged.
| } | ||
| public BreakingChanges compareArrays(JsonNode toCheck, JsonNode toCheckAgainst) { | ||
| if (toCheck.isArray() && toCheckAgainst.isArray()) | ||
| return arraySimpleBodyComparision(toCheck, toCheckAgainst); |
There was a problem hiding this comment.
do you need to do other checks after arraySimpleBodyComparison?
if so, returning from here in case where there are no breaking changes as part of this may not be the desired goal. right?
There was a problem hiding this comment.
In array definitions for json, simple body check will not be used. Had added this for required and enum arrays but seems like this is redundant now since I have other helper methods for them. I am keeping it for the moment to see if it can be used for tuple validation of arrays.
There was a problem hiding this comment.
you perhaps misunderstood my point. you are "returning" after doin array simple body comparisons.. i am sure there are other checks that need to be done if this is true. if its false, its ok to return. but if its true, shouldnt you move to the next check and perform that. right?
| if(toCheck.has("uniqueItems")) { | ||
| if (toCheck.get("uniqueItems").isBoolean() && toCheck.get("uniqueItems").asText() == "true") { | ||
| if (toCheckAgainst.get("uniqueItems") == null) | ||
| return BreakingChanges.ARRAY_UNIQUE_ITEMS_CONDITION_ENABLED; |
There was a problem hiding this comment.
both this and following conditional blocks are returning the same response.. can we consolidate them into a single block?
|
|
||
| private BreakingChanges minMaxItems(JsonNode toCheck, JsonNode toCheckAgainst) { | ||
| if (toCheck.get("maxItems") != null && toCheckAgainst.get("maxItems") == null) | ||
| return BreakingChanges.ARRAY_MAX_ITEMS_CONDITION_ADDED; |
There was a problem hiding this comment.
dont we also need max items condition removed?
There was a problem hiding this comment.
max items removed would be a relaxation for the reader schema and make the writing schema a subset of the space covered with no max items (as present in the reader schema). So a schema with no max items will be able to read data written by a schema with some max items condition.
|
|
||
| @Override | ||
| public boolean canBeRead(SchemaInfo writtenUsing, List<SchemaInfo> readUsing) { | ||
| return false; |
There was a problem hiding this comment.
why are you returning false?
shouldnt it be canBeRead(readUsing, writtenUsing)?
There was a problem hiding this comment.
Done. Yes. Had not implemented it then.
There was a problem hiding this comment.
Will also add tests for this in the IT for the checker.
There was a problem hiding this comment.
One thing i do not see is how this is intended to be injected into the "service".
I dont know if you intend to also make the changes to make service pluggable for compatibility checkers. so then checkers can be loaded on the fly. and avro compatibility checker can be moved out of service.
it a checker is not registered for a type, then only allowAny, DenyAll compatibiliies are allowed.. else the whole spectrum is.
ldr7
force-pushed
the
json-compatibility
branch
from
a87d962
to
619c081
Compare
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
Signed-off-by: Atharva Joshi <[email protected]>
ldr7
force-pushed
the
json-compatibility
branch
from
ed14115
to
cab2df5
Compare
Change log description
Compatibility checker for Json schemas to ensure that only non breaking schema evolutions occur. This means that the data written by the old schemas should be readable for the new schema and/or vice-versa.
Purpose of the change
Closes #146
What the code does
The code contains classes to check for compatibility of each Json data type between two Json schemas. The following comparators are provided:
How to verify it
Run the unit and integration tests.