Skip to content

Conversation

@TeddyCr
Copy link
Collaborator

@TeddyCr TeddyCr commented Jan 26, 2026

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Fixed CSV parsing bug:
    • Test case parameter import failed when values contained semicolons (e.g., SQL queries ending with ;)
    • Changed delimiter from ; to , in TestCaseRepository.serializeParameterValues()
  • New parsing utilities:
    • RestUtil.normalizeQuotes() converts Unicode smart quotes to ASCII quotes
    • RestUtil.extractJsonObjects() extracts JSON objects using state machine brace matching
    • RestUtil.findMatchingBrace() handles nested structures, escape sequences, and quoted strings
  • Comprehensive test coverage:
    • Added 23 unit tests in RestUtilTest.java covering quote normalization, brace matching, and real-world SQL scenarios

This will update automatically on new commits.


aji-aju
aji-aju previously approved these changes Jan 26, 2026
@TeddyCr TeddyCr merged commit c911166 into open-metadata:main Jan 26, 2026
11 of 21 checks passed
@gitar-bot
Copy link

gitar-bot bot commented Jan 26, 2026

Code Review ⚠️ Changes requested 0 resolved / 3 findings

Solid fix for CSV import parsing with robust JSON extraction. Two edge case issues: null handling in normalizeQuotes() and single-quote support in brace matching should be addressed.

⚠️ Edge Case: normalizeQuotes() does not handle null input

📄 openmetadata-service/src/main/java/org/openmetadata/service/util/RestUtil.java

The normalizeQuotes() method does not check for null input and will throw a NullPointerException if called with a null argument. While the current caller in parseParameterValues() trims the input first (which already handles the null case at a higher level), this utility method is public and could be called from other contexts.

Impact: If another part of the codebase calls RestUtil.normalizeQuotes(null), it will crash with NPE.

Suggested fix:

public static String normalizeQuotes(String input) {
  if (input == null) {
    return null;
  }
  return input
      .replace('\u201c', '"')
      .replace('\u201d', '"')
      .replace('\u2018', '\'')
      .replace('\u2019', '\'');
}

Alternatively, add a @NonNull annotation and document this behavior.

⚠️ Bug: findMatchingBrace() doesn't handle single quotes in JSON

📄 openmetadata-service/src/main/java/org/openmetadata/service/util/RestUtil.java

The findMatchingBrace() method only handles double quotes (") for string boundaries but ignores single quotes ('). While standard JSON uses double quotes, the normalizeQuotes() method in the same class converts Unicode single quotes (\u2018, \u2019) to ASCII single quotes ('). If the input contains values with single quotes used as string delimiters (which can happen with some non-standard JSON parsers or after quote normalization), the brace matching could fail.

For example, with input {'key': 'value {x}'}:

  • The method would see { at position 9 inside the string and incorrectly increase depth
  • This could lead to incorrect JSON extraction

Impact: Edge case where single-quoted JSON strings with braces inside could be incorrectly parsed.

Suggested fix:
Track both single and double quote string boundaries:

if (c == '"' || c == '\'') {
  if (!inString) {
    inString = true;
    stringChar = c;
  } else if (c == stringChar) {
    inString = false;
  }
  continue;
}

Alternatively, document that this method only handles double-quoted JSON strings (which is the JSON standard).

💡 Bug: Delimiter change may break import of previously exported CSVs

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java

The serialization delimiter was changed from semicolon (;) to comma (,). While the new extractJsonObjects() parser can handle both delimiters (as shown in tests), this creates a subtle inconsistency:

  1. Old CSV exports used semicolon: {...};{...}
  2. New CSV exports use comma: {...},{...}

The new parser handles both correctly, so backward compatibility for importing is preserved. However, if the system uses round-trip data comparison (export → import → export), the output format will change, which could cause confusion or issues in automated workflows that compare CSV files byte-for-byte.

Impact: Low - parsing works correctly for both formats. This is more of a documentation concern.

Recommendation: Add a comment explaining the format change and that both delimiters are supported for import, or consider keeping semicolon as the delimiter for consistency with existing exports.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ingestion safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants