Repository navigation
Make JsonDocument Underlying Storage Type Aware (Document store / SQL store) #233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
674d2ee
f6cb67a
6e7795d
957a897
502c373
0544358
82dc37f
c0ed72a
e9f25da
e6be8b9
ee7678f
63b0dec
7f91cf0
7376b56
ea2aefd
e3ccb5b
3e3e6cf
e6a6f7e
483eb16
e5926ea
b261463
7cf8e17
9c474db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| package org.hypertrace.core.documentstore; | ||
|
|
||
| public enum DocumentType { | ||
| SQL_STORE, | ||
| DOCUMENT_STORE | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,19 +11,40 @@ public class JSONDocument implements Document { | |
|
|
||
| private static ObjectMapper mapper = new ObjectMapper(); | ||
| private JsonNode node; | ||
| private DocumentType documentType; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid issues like NullPointerException, I'd default it to |
||
|
|
||
| public JSONDocument(String json) throws IOException { | ||
| node = mapper.readTree(json); | ||
| } | ||
|
|
||
| public JSONDocument(String json, DocumentType documentType) throws IOException { | ||
| node = mapper.readTree(json); | ||
| this.documentType = documentType; | ||
| } | ||
|
|
||
| public JSONDocument(Object object) throws IOException { | ||
| node = mapper.readTree(mapper.writeValueAsString(object)); | ||
| } | ||
|
|
||
| public JSONDocument(Object object, DocumentType documentType) throws IOException { | ||
| node = mapper.readTree(mapper.writeValueAsString(object)); | ||
| this.documentType = documentType; | ||
| } | ||
|
|
||
| public JSONDocument(JsonNode node) { | ||
| this.node = node; | ||
| } | ||
|
|
||
| public JSONDocument(JsonNode node, DocumentType documentType) { | ||
| this.node = node; | ||
| this.documentType = documentType; | ||
| } | ||
|
|
||
| @Override | ||
| public DocumentType getDocumentType() { | ||
| return this.documentType; | ||
| } | ||
|
|
||
| @Override | ||
| public String toJson() { | ||
| try { | ||
|
|
@@ -39,6 +60,12 @@ public static JSONDocument errorDocument(String message) { | |
| return new JSONDocument(objectNode); | ||
| } | ||
|
|
||
| public static JSONDocument errorDocument(String message, DocumentType documentType) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please explain why this is significant?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's not needed for feature parity, I'd keep this change separate (and probably defer it until it's needed).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
| ObjectNode objectNode = mapper.createObjectNode(); | ||
| objectNode.put("errorMessage", message); | ||
| return new JSONDocument(objectNode, documentType); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return toJson(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,10 +29,12 @@ | |
| import org.bson.json.JsonMode; | ||
| import org.bson.json.JsonWriterSettings; | ||
| import org.hypertrace.core.documentstore.Document; | ||
| import org.hypertrace.core.documentstore.DocumentType; | ||
| import org.hypertrace.core.documentstore.JSONDocument; | ||
| import org.hypertrace.core.documentstore.model.options.ReturnDocumentType; | ||
|
|
||
| public final class MongoUtils { | ||
|
|
||
| public static final String FIELD_SEPARATOR = "."; | ||
| public static final String PREFIX = "$"; | ||
| private static final String LITERAL = PREFIX + "literal"; | ||
|
|
@@ -137,10 +139,10 @@ public static Document dbObjectToDocument(BasicDBObject dbObject) { | |
| jsonString = dbObject.toJson(relaxed); | ||
| JsonNode jsonNode = MAPPER.readTree(jsonString); | ||
| JsonNode decodedJsonNode = recursiveClone(jsonNode, MongoUtils::decodeKey, identity()); | ||
| return new JSONDocument(decodedJsonNode); | ||
| return new JSONDocument(decodedJsonNode, DocumentType.DOCUMENT_STORE); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once you start having a default, these changes are redundant. |
||
| } catch (IOException e) { | ||
| // throwing exception is not very useful here. | ||
| return JSONDocument.errorDocument(e.getMessage()); | ||
| return JSONDocument.errorDocument(e.getMessage(), DocumentType.DOCUMENT_STORE); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,7 @@ | |
| import org.hypertrace.core.documentstore.Collection; | ||
| import org.hypertrace.core.documentstore.CreateResult; | ||
| import org.hypertrace.core.documentstore.Document; | ||
| import org.hypertrace.core.documentstore.DocumentType; | ||
| import org.hypertrace.core.documentstore.Filter; | ||
| import org.hypertrace.core.documentstore.JSONDocument; | ||
| import org.hypertrace.core.documentstore.Key; | ||
|
|
@@ -1276,7 +1277,7 @@ public Document next() { | |
| } catch (IOException | SQLException e) { | ||
| System.out.println("prepare document failed!"); | ||
| closeResultSet(); | ||
| return JSONDocument.errorDocument(e.getMessage()); | ||
| return JSONDocument.errorDocument(e.getMessage(), DocumentType.SQL_STORE); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1299,7 +1300,7 @@ protected Document prepareDocument() throws SQLException, IOException { | |
| jsonNode.remove(DOCUMENT_ID); | ||
| } | ||
|
|
||
| return new JSONDocument(MAPPER.writeValueAsString(jsonNode)); | ||
| return new JSONDocument(MAPPER.writeValueAsString(jsonNode), DocumentType.SQL_STORE); | ||
| } | ||
|
|
||
| private void addColumnToJsonNode( | ||
|
|
@@ -1430,7 +1431,7 @@ public Document next() { | |
| return prepareDocument(); | ||
| } catch (IOException | SQLException e) { | ||
| closeResultSet(); | ||
| return JSONDocument.errorDocument(e.getMessage()); | ||
| return JSONDocument.errorDocument(e.getMessage(), DocumentType.SQL_STORE); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1447,7 +1448,7 @@ protected Document prepareDocument() throws SQLException, IOException { | |
| jsonNode.put(CREATED_AT, String.valueOf(createdAt)); | ||
| jsonNode.put(UPDATED_AT, String.valueOf(updatedAt)); | ||
|
|
||
| return new JSONDocument(MAPPER.writeValueAsString(jsonNode)); | ||
| return new JSONDocument(MAPPER.writeValueAsString(jsonNode), DocumentType.SQL_STORE); | ||
| } | ||
|
|
||
| protected void closeResultSet() { | ||
|
|
@@ -1508,7 +1509,7 @@ protected Document prepareDocument() throws SQLException, IOException { | |
| } | ||
| } | ||
| } | ||
| return new JSONDocument(MAPPER.writeValueAsString(jsonNode)); | ||
| return new JSONDocument(MAPPER.writeValueAsString(jsonNode), DocumentType.SQL_STORE); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same everywhere. |
||
| } | ||
|
|
||
| private String getColumnValue( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SQL_STOREandDOCUMENT_STORErepresent the underlying store types, not exactly the document types.Can we go with something like
FLAT_STRUCTURE,NESTED_STRUCTURE,?
Or, even simply,
FLAT,NESTEDso that we can call themFlat documentandNested documentin our future discussions.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind adding this attribute is to identify whether the document contains annotated values (
valueList,valueMap, etc.) in theattributesfield/column or not. For documents containing just one column, does it make sense to call them nested or flat? Do you think smth likePLAINandANNOTATEDwould make more sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense. Using
FLATandNESTEDnow.