Make JsonDocument Underlying Storage Type Aware (Document store / SQL store)#233
Conversation
suddendust
requested review from
avinashkolluru,
kotharironak,
skjindal93 and
suresh-prakash
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #233 +/- ##
============================================
+ Coverage 78.49% 79.34% +0.84%
- Complexity 1069 1072 +3
============================================
Files 206 208 +2
Lines 5237 5262 +25
Branches 450 451 +1
============================================
+ Hits 4111 4175 +64
+ Misses 818 770 -48
- Partials 308 317 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
suddendust
changed the title
suresh-prakash
left a comment
suresh-prakash
left a comment
There was a problem hiding this comment.
Thanks for keeping this PR short and sweet. 🙂
| SQL_STORE, | ||
| DOCUMENT_STORE |
There was a problem hiding this comment.
SQL_STORE and DOCUMENT_STORE represent the underlying store types, not exactly the document types.
Can we go with something like
FLAT_STRUCTURE,
NESTED_STRUCTURE,
?
Or, even simply, FLAT, NESTED so that we can call them Flat document and Nested document in our future discussions.
There was a problem hiding this comment.
The idea behind adding this attribute is to identify whether the document contains annotated values (valueList, valueMap, etc.) in the attributes field/column or not. For documents containing just one column, does it make sense to call them nested or flat? Do you think smth like PLAIN and ANNOTATED would make more sense?
There was a problem hiding this comment.
The nesting of valueList, valueMap, etc. is not handled by the document-store itself. It is something maintained by the clients. So, in a true-sense the annotation is the responsibility of the clients and the document-store itself is agnostic of that.
There was a problem hiding this comment.
Yes, makes sense. Using FLAT and NESTED now.
| return new JSONDocument(objectNode); | ||
| } | ||
|
|
||
| public static JSONDocument errorDocument(String message, DocumentType documentType) { |
There was a problem hiding this comment.
Could you please explain why this is significant?
There was a problem hiding this comment.
Returning the type of an error json doc indicates that that error was encountered while generating a doc of this particular type. Callers can use this information in future.
There was a problem hiding this comment.
If it's not needed for feature parity, I'd keep this change separate (and probably defer it until it's needed).
|
|
||
| private static ObjectMapper mapper = new ObjectMapper(); | ||
| private JsonNode node; | ||
| private DocumentType documentType; |
There was a problem hiding this comment.
To avoid issues like NullPointerException, I'd default it to NESTED.
| JsonNode jsonNode = MAPPER.readTree(jsonString); | ||
| JsonNode decodedJsonNode = recursiveClone(jsonNode, MongoUtils::decodeKey, identity()); | ||
| return new JSONDocument(decodedJsonNode); | ||
| return new JSONDocument(decodedJsonNode, DocumentType.DOCUMENT_STORE); |
There was a problem hiding this comment.
Once you start having a default, these changes are redundant.
| } | ||
| } | ||
| return new JSONDocument(MAPPER.writeValueAsString(jsonNode)); | ||
| return new JSONDocument(MAPPER.writeValueAsString(jsonNode), DocumentType.SQL_STORE); |
2 participants
Description
This PR adds this method
DocumentType getDocumentType();toDocument.javawhich returns eitherFLATorNESTED. This information can be used by library users to make certain decisions, like inentity-service, we can decide the kind of getter to use based on whether the doc is nested or flat.Testing
I have added some new test cases to
JsonDocumentTest.Checklist: