fix: JanusGraph property cardinality and hierarchy serialization fixes#1197
fix: JanusGraph property cardinality and hierarchy serialization fixes#1197pallakartheekreddy merged 6 commits intodevelopfrom
Conversation
- Enforces VertexProperty.Cardinality.single to prevent property accumulation in JanusGraph. - Fixes childNodes serialization corruption in HierarchyManager by using .asJava.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SonarCloud Analysis Results 🔍Quality Gate Results for Services:Please review the analysis results for each service. Ensure all quality gates are passing before merging. |
1 similar comment
SonarCloud Analysis Results 🔍Quality Gate Results for Services:Please review the analysis results for each service. Ensure all quality gates are passing before merging. |
SonarCloud Analysis Results 🔍Quality Gate Results for Services:Please review the analysis results for each service. Ensure all quality gates are passing before merging. |
SonarCloud Analysis Results 🔍Quality Gate Results for Services:Please review the analysis results for each service. Ensure all quality gates are passing before merging. |
pallakartheekreddy
left a comment
pallakartheekreddy
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR correctly identifies and addresses two real bugs from the Neo4j → JanusGraph migration. The core approach (using VertexProperty.Cardinality.single) is the right fix. However, there are a few issues that should be addressed before merging.
Critical: removeProps semantics silently broken
File: NodeAsyncOperations.java — setPrimitiveData (last line of the method)
result.put(key, value != null ? value : "");When a caller sets a metadata value to null to signal property deletion (via removeProps), setPrimitiveData converts it to "" before the value ever reaches upsertNode's property loop. The loop then:
- Drops existing vertex properties for the key ✓
- Skips writing when
value == null— but value is now"""\ notnull` ✗ - Writes
""back to JanusGraph instead of deleting the property
DefinitionNode.scala:129 sets dbNode.getMetadata.put(prop, null) to request deletion. After this PR, those properties get written as empty strings instead of being removed — a silent data regression.
Fix: Don't replace null with "". Let null propagate as null into result so the if (value == null) continue in upsertNode correctly skips writing (and therefore leaves the explicit pre-delete as a deletion):
result.put(key, value); // keep null as nullMajor: finally block inconsistency across methods
upsertNode was correctly upgraded to use try/finally for transaction cleanup. However addNode, upsertRootNode, and updateNodes still use the old pattern:
// Old pattern (still in addNode, upsertRootNode, updateNodes):
} catch (Exception e) {
if (null != tx)
tx.rollback();If an exception is thrown inside the catch block, transactions leak. Please apply the same finally pattern consistently:
} finally {
if (null != tx && tx.isOpen()) {
try { tx.rollback(); } catch (Exception ex) { ... }
}
}Major: getNodeWithoutRelations — ClassCastException risk on migrated nodes
The new accumulation logic in JanusGraphNodeUtil (building List<Object> when duplicate property keys exist in JanusGraph) is a sensible backward-compatibility measure for nodes corrupted before this fix. However:
- Callers doing
(String) node.getMetadata().get("status")will throwClassCastExceptionon any node that has accumulated duplicate values. - Since new writes now use
Cardinality.single(dropping existing before write), this code path only fires for pre-migration corrupted nodes. Please add a comment making this explicit and indicating a cleanup timeline. - The unchecked cast
(List<Object>) existingon the accumulation line will produce a compiler warning. Add@SuppressWarnings("unchecked")or restructure to avoid it.
Minor: Object[] case in setPrimitiveData misses primitive arrays
} else if (value instanceof Object[]) {
value = JsonUtils.serialize(Arrays.asList((Object[]) value));
}int[], long[], boolean[] are NOT Object[] — they'll pass through unchanged as raw primitive arrays that JanusGraph cannot store. Not a problem for current usage, but worth a comment or explicit handling.
Minor: Removed named catch around tx.commit() reduces debuggability
The old code had explicit telemetry on commit failure (optimistic locking conflicts, constraint violations). The new plain tx.commit() falls through to the generic catch, making these failures harder to diagnose in production logs. Consider restoring the named catch just for commit:
try { tx.commit(); } catch (Exception commitEx) {
TelemetryManager.error("Commit failed for " + identifier + ": " + commitEx.getMessage(), commitEx);
throw commitEx;
}Minor: No regression test for the core cardinality fix
The test suite wasn't extended with a test that upserts the same node twice and asserts the field has exactly one value. This is the direct regression test for the main bug — without it, property accumulation could silently return in a future refactor.
Minor: PR checklist and stack version in description
The PR description checklist is entirely unchecked, and the test configuration lists scala-2.12, play-2.7.2. This project uses Scala 2.13 / Play Framework 3.0.5. Please update the description to confirm the correct stack was tested.
Positive Notes
- Using
Cardinality.singleis the correct fix for JanusGraph's defaultlistcardinality behavior. - The explicit pre-delete loop before writing (
it.next().remove()) is a good belt-and-suspenders measure. - Refactoring
setPrimitiveDatafromstream + peekto an explicit loop is a correctness improvement (peekfor mutation is an anti-pattern). - The
finallyblock inupsertNodeis an improvement. TestHierarchyManagerSerializationis a good regression test for the hierarchy read path.- Error messages now include
e.getMessage()for better debuggability.
|
|
SonarCloud Analysis Results 🔍Quality Gate Results for Services:Please review the analysis results for each service. Ensure all quality gates are passing before merging. |
2 participants
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please choose appropriate options.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes in the below checkboxes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: