Concept map JSON serialization #404
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
dmitrii-ubskii
force-pushed
the
concept-map-json
branch
from
73fa39d
to
d7f5b54
Compare
dmitrii-ubskii
commented
Mar 23, 2023
Comment on lines
+44
to
+49
| @CheckReturnValue | ||
| default JsonObject toJSON() { | ||
| JsonObject object = Json.object(); | ||
| map().forEach((resVar, resConcept) -> object.add(resVar, resConcept.toJSON())); | ||
| return object; | ||
| } |
There was a problem hiding this comment.
All implementations of toJSON() are done in the api package. Otherwise the implementation would be duplicated between e.g. ThingImpl and ThingImpl.Remote, since ...Impl.Remote don't extend their enclosing counterparts.
Comment on lines
+59
to
+77
| private boolean JSONValuesAreEqual(JsonValue left, JsonValue right) { | ||
| if (Objects.equals(left, right)) return true; | ||
| if (left == null || right == null) return false; | ||
| if (left.isObject() && right.isObject()) return JSONObjectsAreEqual(left.asObject(), right.asObject()); | ||
| if (left.isArray() && right.isArray()) return JSONArraysAreEqual(left.asArray(), right.asArray()); | ||
| return false; | ||
| } | ||
|
|
||
| private boolean JSONObjectsAreEqual(JsonObject left, JsonObject right) { | ||
| if (left.size() != right.size()) return false; | ||
| return left.names().stream().allMatch((name) -> JSONValuesAreEqual(left.get(name), right.get(name))); | ||
| } | ||
|
|
||
| private boolean JSONArraysAreEqual(JsonArray left, JsonArray right) { | ||
| if (left.size() != right.size()) return false; | ||
| for (int i = 0; i < left.size(); i++) | ||
| if (!JSONValuesAreEqual(left.get(i), right.get(i))) return false; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
JsonObjects are ordered, and by default the comparison would respect the order of keys. Since we don't specify the order, we have to define our own equality check for JsonValues.
dmitrii-ubskii
requested review from
alexjpwalker and
haikalpribadi
as code owners
benjaminasmall
approved these changes
Mar 23, 2023
alexjpwalker
approved these changes
Mar 23, 2023
| plugin = "pretty", | ||
| glue = "com.vaticle.typedb.client.test.behaviour", | ||
| features = "external/vaticle_typedb_behaviour/concept/serialization.feature", | ||
| tags = "not @ignore and not @ignore-typedb" |
There was a problem hiding this comment.
Can you double check the @ignore-typedb is correct here? I think we have one for client-java or clients specifically or something? Might as well do a drive-by of the other BDD tests in this repo while you're at it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the goal of this PR?
We implement the JSON serialization of concept maps according to typedb/typedb-behaviour#238.