Skip to content

Conversation

@aptend
Copy link
Collaborator

@aptend aptend commented Jan 8, 2026

PR Type

Bug fix, Tests


Description

  • Refactored column metadata comparison to compare one column at a time

  • Extracted stringAt() method for building individual column metadata strings

  • Fixed special character handling in metadata comparison logic

  • Added comprehensive unit tests for @metacmp flag parsing and behavior


Diagram Walkthrough

flowchart LR
  A["RSMetaData.equals()"] -->|"Compare per-column"| B["stringAt(index)"]
  B -->|"Build metadata string"| C["Column info"]
  D["fullString()"] -->|"Refactored to use"| B
  E["ScriptParser tests"] -->|"Validate @metacmp parsing"| F["Document-level flags"]
  E -->|"Validate @metacmp parsing"| G["SQL-level flags"]
  F -->|"Priority handling"| G
Loading

File Walkthrough

Relevant files
Bug fix
RSMetaData.java
Refactor metadata comparison to per-column logic                 

src/main/java/io/mo/result/RSMetaData.java

  • Refactored equals() method to compare metadata column-by-column
    instead of full strings
  • Extracted new stringAt(int index) method to build individual column
    metadata strings
  • Moved special character check into the else-if condition for non-full
    metadata comparison
  • Updated fullString() method to reuse stringAt() for consistency
+14/-10 
Tests
ScriptParserTest.java
Add comprehensive @metacmp flag parsing tests                       

src/test/java/io/mo/util/ScriptParserTest.java

  • Added testDocumentLevelMetacmpFlag() to verify document-level @metacmp
    flag parsing
  • Added testSQLLevelMetacmpFlag() to verify SQL-level @metacmp flag
    parsing
  • Added testMetacmpPriority() to validate SQL-level flags override
    document-level
  • Added testMetacmpBooleanParsing() to test case-insensitive boolean
    value parsing
  • Added testInvalidMetacmpFormat() to verify invalid formats are safely
    ignored
  • Added testMixedMetacmpFlags() to test complex scenarios with mixed
    flag levels
+226/-0 

@qodo-code-review
Copy link

qodo-code-review bot commented Jan 8, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Non-descriptive names: The newly introduced variables f1 and f2 are not self-descriptive and reduce readability
in the metadata comparison logic.

Referred Code
String f1 = this.stringAt(i);
String f2 = meta.stringAt(i);
if (!f1.equalsIgnoreCase(f2)) {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing null guards: The new code calls meta.stringAt(i) without checking whether meta is null or whether meta
has compatible bounds, which can lead to unhandled runtime exceptions depending on calling
context.

Referred Code
public boolean equals(RSMetaData meta) {
    for (int i = 0; i < columnCount; i++) {
        if (this.fullMetaInfo) {
            String f1 = this.stringAt(i);
            String f2 = meta.stringAt(i);
            if (!f1.equalsIgnoreCase(f2)) {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Potential sensitive logging: The new per-column comparison logs the full constructed column metadata strings (f1/f2),
which may include sensitive schema/label information depending on usage and should be
reviewed/redacted if necessary.

Referred Code
String f1 = this.stringAt(i);
String f2 = meta.stringAt(i);
if (!f1.equalsIgnoreCase(f2)) {
    LOG.error("The meta info does not equal with each other,one is ["
            + f1 + "],but the other is [" + f2 + "]");
    return false;

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Jan 8, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Skip special‐char columns first

Move the containSpecialChar check to the top of the loop in the equals method to
ensure columns with special characters are always skipped during comparison.

src/main/java/io/mo/result/RSMetaData.java [68-84]

 for (int i = 0; i < columnCount; i++) {
+    if (containSpecialChar(this.columnLabels[i])) {
+        continue;
+    }
     if (this.fullMetaInfo) {
         String f1 = this.stringAt(i);
         String f2 = meta.stringAt(i);
         if (!f1.equalsIgnoreCase(f2)) {
             LOG.error(...);
             return false;
         }
-    } else if (!containSpecialChar(this.columnLabels[i]))  {
+    } else {
         if (!this.columnLabels[i].equalsIgnoreCase(meta.getColumnLable(i))) {
             LOG.error(...);
             return false;
         }
     }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a logic bug introduced in the refactoring where columns with special characters are no longer skipped in all cases, which was the original behavior.

High
General
Override equals and hashCode

Implement equals(Object) and hashCode() in RSMetaData to adhere to Java's object
contract, ensuring correct behavior in collections.

src/main/java/io/mo/result/RSMetaData.java [67-86]

-public boolean equals(RSMetaData meta) {
-    ...
+@Override
+public boolean equals(Object obj) {
+    if (this == obj) return true;
+    if (!(obj instanceof RSMetaData)) return false;
+    return equals((RSMetaData) obj);
 }
 
+@Override
+public int hashCode() {
+    return Objects.hash(
+        columnCount,
+        fullMetaInfo,
+        Arrays.hashCode(columnLabels),
+        Arrays.hashCode(types),
+        Arrays.hashCode(precisions),
+        Arrays.hashCode(scales)
+    );
+}
+

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly advises implementing equals(Object) and hashCode() for contract compliance, which is a significant improvement for correctness when using the class in collections.

Medium
Ensure temporary test files are deleted

Improve temporary file handling in tests by using a try-finally block for
explicit deletion instead of relying on deleteOnExit().

src/test/java/io/mo/util/ScriptParserTest.java [657-674]

 @Test
 public void testDocumentLevelMetacmpFlag() throws Exception {
     File tempFile = File.createTempFile("test_metacmp_doc_", ".sql");
-    tempFile.deleteOnExit();
-    
-    try (PrintWriter writer = new PrintWriter(new FileWriter(tempFile))) {
-        writer.println("--- @metacmp(true)");
-        writer.println("SELECT 1;");
-        writer.println("SELECT 2;");
+    try {
+        try (PrintWriter writer = new PrintWriter(new FileWriter(tempFile))) {
+            writer.println("--- @metacmp(true)");
+            writer.println("SELECT 1;");
+            writer.println("SELECT 2;");
+        }
+        
+        ScriptParser parser = new ScriptParser();
+        TestScript testScript = parser.parseScript(tempFile.getAbsolutePath());
+        
+        assertNotNull("TestScript should not be null", testScript);
+        assertEquals("Should have compareMeta set to true", Boolean.TRUE, testScript.getCompareMeta());
+        assertEquals("Should have 2 commands", 2, testScript.getTotalCmdCount());
+    } finally {
+        if (tempFile != null && tempFile.exists()) {
+            tempFile.delete();
+        }
     }
-    
-    ScriptParser parser = new ScriptParser();
-    TestScript testScript = parser.parseScript(tempFile.getAbsolutePath());
-    
-    assertNotNull("TestScript should not be null", testScript);
-    assertEquals("Should have compareMeta set to true", Boolean.TRUE, testScript.getCompareMeta());
-    assertEquals("Should have 2 commands", 2, testScript.getTotalCmdCount());
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that using try-finally for temporary file cleanup is more robust than deleteOnExit(), improving test hygiene and preventing potential resource leaks.

Low
  • Update

@heni02 heni02 self-requested a review January 8, 2026 08:08
@heni02 heni02 merged commit 17e207c into matrixorigin:main Jan 8, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants