Ctf2 fixes #365
Ctf2 fixes #365
Conversation
Change-Id: Ied093d7edf0e7de76e0dc637699711cf0653dc6b Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I62cc5e26645ea1d2bd2150af99d5aeb878061e1f Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: Ie8752173c78caf0d0f5cef3cb1a5d2aa58c1e8d8 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
WalkthroughThese changes enhance CTF metadata parsing by introducing JSON alias expansion to resolve field references to their full definitions, extending variant tag support to IntegerDeclaration types, and adding three new constants for metadata keyword access. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java (1)
301-316: Review alias expansion logic for potential issues.The
expandAliasesmethod has two potential concerns:
Broad string replacement: Line 304 replaces any string value that matches an alias name, regardless of context. This could inadvertently replace legitimate string values (e.g., if an alias is named "type", "name", or "id").
No cycle detection: If circular alias references exist (A → B → A), the recursive expansion will cause infinite recursion and a StackOverflowError.
Consider these improvements:
For issue 1, limit replacement to specific known alias reference locations rather than all strings:
private static void expandAliases(JsonObject obj, Map<String, JsonObject> metadata, String contextKey) { // Only expand in known alias reference contexts (e.g., "field-class", "type", etc.) if (ALIAS_REFERENCE_KEYS.contains(contextKey)) { // perform replacement } }For issue 2, add cycle detection:
+private static void expandAliases(JsonObject obj, Map<String, JsonObject> metadata) { + expandAliases(obj, metadata, new HashSet<>()); +} + +private static void expandAliases(JsonObject obj, Map<String, JsonObject> metadata, Set<String> visited) { for (String key : obj.keySet()) { JsonElement value = obj.get(key); - if (value.isJsonPrimitive() && value.getAsJsonPrimitive().isString() && metadata.containsKey(value.getAsString())) { + String aliasName = value.getAsString(); + if (value.isJsonPrimitive() && value.getAsJsonPrimitive().isString() && metadata.containsKey(aliasName)) { + if (visited.contains(aliasName)) { + throw new CTFException("Circular alias reference detected: " + aliasName); + } + visited.add(aliasName); - obj.add(key, metadata.get(value.getAsString())); + JsonObject expanded = metadata.get(aliasName); + expandAliases(expanded, metadata, visited); + obj.add(key, expanded); + visited.remove(aliasName); } else if (value.isJsonObject()) { - expandAliases(value.getAsJsonObject(), metadata); + expandAliases(value.getAsJsonObject(), metadata, visited); } else if (value.isJsonArray()) { for (JsonElement elem : value.getAsJsonArray()) { if (elem.isJsonObject()) { - expandAliases(elem.getAsJsonObject(), metadata); + expandAliases(elem.getAsJsonObject(), metadata, visited); } } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(21-185)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(21-185)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
46-54: LGTM! Constants are well-defined.The three new constants (FIELD_CLASS, NAME, PATH) follow the existing pattern and are properly documented. These additions support the enhanced JSON metadata parsing introduced in the other files.
Also applies to: 176-179
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java (1)
365-375: Good extension to support IntegerDeclaration as variant tag.The updated logic appropriately handles both EnumDeclaration (with intersection validation) and IntegerDeclaration (without value validation, as integers can represent any variant option) as valid variant tag types. This aligns with the CTF2 parsing enhancements described in the PR.
| String[] jsonBlocks = json.split("\u001e"); //$NON-NLS-1$ | ||
| Map<String, JsonObject> metadata = new HashMap<>(); | ||
|
|
||
| // Second pass: expand string references and parse |
There was a problem hiding this comment.
Clarify or correct the "Second pass" comment.
The comment states "Second pass" but there's no visible first pass in the code. The loop performs JSON parsing, alias expansion, and fragment creation in a single pass.
Consider updating the comment to accurately describe the operation:
- // Second pass: expand string references and parse
+ // Parse fragments, expand aliases, and build metadata tree📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Second pass: expand string references and parse | |
| // Parse fragments, expand aliases, and build metadata tree |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java
around line 257, the inline comment "Second pass: expand string references and
parse" is misleading because there is no prior "first pass" and the loop
performs parsing, alias expansion, and fragment creation in one pass; update the
comment to accurately describe the work being done (for example: "Parse metadata
entries, expand string references/aliases, and create fragments"), or split the
logic into clearly named helper methods with comments for each stage if you
prefer to retain multi-pass terminology.
| if (obj.has(JsonMetadataStrings.TYPE) && obj.get(JsonMetadataStrings.TYPE).getAsString().equals(JsonMetadataStrings.FRAGMENT_FIELD_ALIAS)) { | ||
| metadata.put(obj.get(JsonMetadataStrings.NAME).getAsString(), obj.get(JsonMetadataStrings.FIELD_CLASS).getAsJsonObject()); | ||
| } |
There was a problem hiding this comment.
Add null checks before accessing NAME and FIELD_CLASS.
Line 264 assumes that FRAGMENT_FIELD_ALIAS nodes always have both NAME and FIELD_CLASS fields. If either is missing, this will throw a NullPointerException.
Apply this diff to add defensive checks:
if (obj.has(JsonMetadataStrings.TYPE) && obj.get(JsonMetadataStrings.TYPE).getAsString().equals(JsonMetadataStrings.FRAGMENT_FIELD_ALIAS)) {
- metadata.put(obj.get(JsonMetadataStrings.NAME).getAsString(), obj.get(JsonMetadataStrings.FIELD_CLASS).getAsJsonObject());
+ if (obj.has(JsonMetadataStrings.NAME) && obj.has(JsonMetadataStrings.FIELD_CLASS)) {
+ metadata.put(obj.get(JsonMetadataStrings.NAME).getAsString(), obj.get(JsonMetadataStrings.FIELD_CLASS).getAsJsonObject());
+ }
}🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java
around lines 263 to 265, the code assumes FRAGMENT_FIELD_ALIAS objects always
contain NAME and FIELD_CLASS, which can cause NullPointerException; update the
conditional to also check obj.has(JsonMetadataStrings.NAME) and
obj.has(JsonMetadataStrings.FIELD_CLASS) (and ensure the retrieved members are
non-null before calling getAsString()/getAsJsonObject()), and only call
metadata.put(...) when both checks pass (optionally log or skip entries when
fields are missing).
| JsonObject fieldClass = ((JsonStructureFieldMemberMetadataNode) variant).getFieldClass().getAsJsonObject(); | ||
| if (fieldClass.has(SELECTOR_FIELD_LOCATION)) { | ||
| JsonArray location = fieldClass.get(SELECTOR_FIELD_LOCATION).getAsJsonArray(); | ||
| JsonArray location = (fieldClass.get(SELECTOR_FIELD_LOCATION).getAsJsonObject().get(JsonMetadataStrings.PATH)).getAsJsonArray(); |
There was a problem hiding this comment.
Add null safety checks for nested JSON navigation.
The chained method calls assume that SELECTOR_FIELD_LOCATION is a JsonObject containing a PATH field that is a JsonArray. If the structure is malformed, this will throw an exception.
Consider adding intermediate checks or using a try-catch block:
if (fieldClass.has(SELECTOR_FIELD_LOCATION)) {
- JsonArray location = (fieldClass.get(SELECTOR_FIELD_LOCATION).getAsJsonObject().get(JsonMetadataStrings.PATH)).getAsJsonArray();
+ JsonElement selectorElement = fieldClass.get(SELECTOR_FIELD_LOCATION);
+ if (selectorElement != null && selectorElement.isJsonObject()) {
+ JsonElement pathElement = selectorElement.getAsJsonObject().get(JsonMetadataStrings.PATH);
+ if (pathElement != null && pathElement.isJsonArray()) {
+ JsonArray location = pathElement.getAsJsonArray();
- variantTag = location.get(location.size() - 1).getAsString();
- hasTag = true;
+ variantTag = location.get(location.size() - 1).getAsString();
+ hasTag = true;
+ }
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java
around line 270, the code directly chains
JsonElement.get/getAsJsonObject/getAsJsonArray calls which will throw NPEs or
IllegalStateExceptions if SELECTOR_FIELD_LOCATION or PATH are missing or of the
wrong type; add explicit null/type checks (e.g., check that
fieldClass.has(SELECTOR_FIELD_LOCATION) and that the retrieved element
isJsonObject, that it has PATH and that PATH isJsonArray) before calling
getAsJsonArray, or wrap the access in a try-catch and produce a clear
error/return path; ensure you handle the malformed case by logging or throwing a
descriptive exception rather than letting a chained call blow up.
1 participant
Note the code was "assisted" by claude sonnet 4/4.5 but the suggestions were bad and all rejected.
What it does
Partially fixes CTF2 parsing
How to test
Load an LTTng trace from here #364. The error will be after the metadata parses
Follow-ups
CTF2 doesn't parse still as there are no dynamic length string parsers.
Review checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.