Skip to content

Conversation

@IceS2
Copy link
Contributor

@IceS2 IceS2 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

  • Bug fix:
    • Escapes single quotes in AlertUtil.convertInputListToString() using SQL-standard doubling convention (''')
  • New test file:
    • AlertUtilTest.java with 9 test methods covering edge cases including apostrophes in strings (e.g., "Jake's test bundle")
  • Security improvement:
    • Prevents syntax errors and potential SQL injection vulnerabilities in alert filtering rules and message formatting

This will update automatically on new commits.


@gitar-bot
Copy link

gitar-bot bot commented Jan 26, 2026

🔍 CI failure analysis for a96cc1f: Eight CI failures analyzed: Maven tests (1 failure + 3 errors), Python 3.10 & 3.11 (2 failures each), Playwright shards 2,3,4,5,6 (64 failures, 45 flaky passed). All unrelated to this PR's backend Java alert utility changes.

Complete CI Failures Summary - All Eight Jobs

Overall Statistics

Maven Tests (NEW):

  • maven-postgresql-ci: 1 failure, 3 errors, 7,836 tests run

Python Tests (2 versions):

  • py-run-tests (3.10): 2 failed, 531 passed
  • py-run-tests (3.11): 2 failed (SAME tests), 531 passed

Playwright Tests (5 shards):

  • Total failures: 64 across 5 shards
  • Total flaky: 45 - ALL passed on retry ✅
  • Total passed: 2,600

Grand Total: 72 failures (1+3 Maven + 4 Python + 64 Playwright)
Grand Total Flaky: 45 (all passed on retry) ✅


NEW Failure: maven-postgresql-ci

Test Results

Tests run: 7836
Failures: 1
Errors: 3
Skipped: 701
Build time: 2.74 hours

Specific Failures

1. Test Failure:

AppsResourceTest.post_trigger_app_200:369->assertAppRanAfterTriggerWithStatus:452
Max retries exceeded polling for eventual assert

2. Test Error (403 Forbidden):

GlossaryTermResourceTest.testGlossaryTermCsvImportSameCSV:4494->deleteEntity:5104
HttpResponse status code: 403, reason phrase: Error reading response:
User 'admin' is not a reviewer

3. AWS Credentials Errors (3 tests):

AwsCredentialsUtilTest.testBuildCredentialsProviderWithEmptyCredentials:77
AwsCredentialsUtilTest.testBuildCredentialsProviderWithNoCredentials:52
AwsCredentialsUtilTest.testBuildCredentialsProviderWithOnlyAccessKey:64

IllegalArgumentException: AWS credentials not configured.
Either provide accessKeyId/secretAccessKey or set enabled=true to use IAM authentication.

Root Cause

UNRELATED to this PR. This PR only modifies:

  • AlertUtil.convertInputListToString() (alert utility string escaping)
  • AlertUtilTest.java (unit tests)

The failures are in:

  1. AppsResourceTest: App trigger polling timeout
  2. GlossaryTermResourceTest: Permission error (user admin not a reviewer)
  3. AwsCredentialsUtilTest: AWS credentials configuration tests

None of these components use or depend on alert utility string escaping.

Analysis

AppsResourceTest failure:

  • Polling timeout suggests async app execution issues
  • Could be performance problem or test timeout too short

GlossaryTermResourceTest error:

  • Permission/authorization issue
  • Admin user not recognized as reviewer for CSV import cleanup
  • Possible recent change to reviewer validation logic

AwsCredentialsUtilTest errors:

  • Tests validating AWS credential configuration edge cases
  • Tests expect specific error handling behavior
  • Possible recent change to AWS credentials validation logic

Summary - All CI Failures

All 72 failures (1+3 Maven + 4 Python + 64 Playwright) are UNRELATED to this PR.

This PR modifies:

  • Backend: AlertUtil.convertInputListToString() - single quote escaping
  • Backend: AlertUtilTest.java - unit tests

Failures span completely different components:

  1. Maven backend tests: Apps, glossary terms, AWS credentials
  2. Python ingestion: Auto-classification, S3 connector
  3. Frontend Playwright: UI tests (visibility, timeouts, assertions)

None of these have any code path connection to alert utility string escaping.

Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Good security fix for SQL single quote escaping with comprehensive tests. The previous finding about potential NPE from null list elements remains unaddressed.

💡 Edge Case: Potential NullPointerException if list contains null elements

📄 openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertUtil.java:363

The code handles null lists and empty lists, but if the list contains a null element (e.g., List.of("value1", null)), calling .replace("'", "''") on a null string will throw a NullPointerException.

While this may be an existing issue not introduced by this PR, the escaping logic makes it more visible. Consider adding a null check for individual elements:

String value = valueList.get(i);
if (value != null) {
    result.append(",'").append(value.replace("'", "''")).append("'");
}

Alternatively, add a test case to document expected behavior for null elements in the list. This is a minor edge case since List.of() doesn't allow null values, but other List implementations do.

Rules ✅ All requirements met

Gitar Rules

Summary Enhancement: PR summary already added and remains accurate

2 rules not applicable. Show all rules by commenting gitar display:verbose.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

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.

3 participants