Skip to content
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.

Sign up for GitHub

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

Handled dollar prefixed values for "createOrReplace" Mongo implementation #207

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Handled dollar prefixed values for "createOrReplace" Mongo implementation #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6274ec
Handled dollar prefixed values for "createOrReplace" Mongo implementa…
suresh-prakash Aug 29, 2024
ba4898b
Merge branch 'main' into handle_doller_prefix_values
suresh-prakash Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion document-store/src/integrationTest/resources/create/document_one.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
},
{
"name": "Mars",
"color": "Red",
"color": "$Red",
"humans_live": "possibly in future",
"tags": {}
}
Expand Down
2 changes: 1 addition & 1 deletion document-store/src/integrationTest/resources/create/document_one_response.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
},
{
"name": "Mars",
"color": "Red",
"color": "$Red",
"humans_live": "possibly in future",
"tags": {}
}
Expand Down
26 changes: 24 additions & 2 deletions document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ArrayNode;
import com.fasterxml.jackson.databind.node.JsonNodeFactory;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.fasterxml.jackson.databind.node.TextNode;
import com.mongodb.BasicDBObject;
import com.mongodb.client.model.ReturnDocument;
import java.io.IOException;
Expand All @@ -35,6 +37,7 @@
public final class MongoUtils {
public static final String FIELD_SEPARATOR = ".";
public static final String PREFIX = "$";
private static final String UNICODE_FOR_FIELD_PREFIX = "\\u0024";
private static final String LITERAL = PREFIX + "literal";
private static final String UNSUPPORTED_OPERATION = "No MongoDB support available for: '%s'";
private static final ObjectMapper MAPPER = new ObjectMapper();
Expand All @@ -54,15 +57,19 @@
return null;
}

return key.replace("\\", "\\\\").replace(PREFIX, "\\u0024").replace(FIELD_SEPARATOR, "\\u002e");
return key.replace("\\", "\\\\")
.replace(PREFIX, UNICODE_FOR_FIELD_PREFIX)
.replace(FIELD_SEPARATOR, "\\u002e");
}

public static String decodeKey(final String key) {
if (key == null) {
return null;
}

return key.replace("\\u002e", FIELD_SEPARATOR).replace("\\u0024", PREFIX).replace("\\\\", "\\");
return key.replace("\\u002e", FIELD_SEPARATOR)
.replace(UNICODE_FOR_FIELD_PREFIX, PREFIX)
.replace("\\\\", "\\");
}

public static String getLastField(final String fieldPath) {
Expand Down Expand Up @@ -99,6 +106,10 @@
JsonNode src,
UnaryOperator<String> function,
final UnaryOperator<ObjectNode> emptyObjectConverter) {
if (src.isTextual()) {
return new TextNode(function.apply(src.asText()));

Check warning on line 110 in document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java#L110 

Added line #L110 was not covered by tests
}

if (!src.isObject()) {
return src;
}
Expand All @@ -113,6 +124,17 @@
if (value.isObject()) {
newValue = recursiveClone(value, function, emptyObjectConverter);
}
if (value.isArray()) {
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically in this kind of function you'd have object/array/primitives as sibling code paths, with recursion inside object and array. Here it looks like we handle primitives, else assume it's an object and only treat array differently inside the processing of object fields? Am I reading that right and if so, is there a reason to do it that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are treating "primitive values" differently.
Also, we are treating "object keys" differently.
Both needs encoding so that a $ prefix is (treated literally and) not treated as the special field lookup operator.

We are still doing similar thing. Nested inside objects and arrays and handling primitives differently. Because, we also want object keys to be encoded, this method turned out this way.

In this PR, I'm just handling the missing case of "primitive values" (especially when they are strings). So, just piggy bagging onto the existing method.

newValue = new ArrayNode(JsonNodeFactory.instance);

Check warning on line 128 in document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java#L128 

Added line #L128 was not covered by tests
for (int i = 0; i < value.size(); i++) {
final JsonNode elementValue =
recursiveClone(value.get(i), function, emptyObjectConverter);
((ArrayNode) newValue).add(elementValue);

Check warning on line 132 in document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java#L130-L132 

Added lines #L130 - L132 were not covered by tests
}
}
if (newValue.isTextual()) {
newValue = new TextNode(function.apply(newValue.asText()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this, we have already applied transformation to each value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry my mistake. I read it as array node.

}
tgt.set(newFieldName, newValue);
}
return tgt.isEmpty() ? emptyObjectConverter.apply(tgt) : tgt;
Expand Down
Loading