Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

🛡️ TRIVY SCAN RESULT 🛡️

Target: openmetadata-ingestion:trivy (debian 12.12)

Vulnerabilities (4)

🛡️ TRIVY SCAN RESULT 🛡️

Target: Java

Vulnerabilities (39)

🛡️ TRIVY SCAN RESULT 🛡️

Target: Node.js

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: Python

Vulnerabilities (33)

🛡️ TRIVY SCAN RESULT 🛡️

Target: usr/bin/docker

Vulnerabilities (4)

🛡️ TRIVY SCAN RESULT 🛡️

Target: /etc/ssl/private/ssl-cert-snakeoil.key

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /home/airflow/openmetadata-airflow-apis/openmetadata_managed_apis.egg-info/PKG-INFO

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: openmetadata-ingestion-base-slim:trivy (debian 12.13)

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: Java

Vulnerabilities (38)

🛡️ TRIVY SCAN RESULT 🛡️

Target: Node.js

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: Python

Vulnerabilities (15)

🛡️ TRIVY SCAN RESULT 🛡️

Target: /etc/ssl/private/ssl-cert-snakeoil.key

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /ingestion/pipelines/extended_sample_data.yaml

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /ingestion/pipelines/lineage.yaml

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /ingestion/pipelines/sample_data.json

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /ingestion/pipelines/sample_data.yaml

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /ingestion/pipelines/sample_data_aut.yaml

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /ingestion/pipelines/sample_usage.json

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /ingestion/pipelines/sample_usage.yaml

No Vulnerabilities Found

🛡️ TRIVY SCAN RESULT 🛡️

Target: /ingestion/pipelines/sample_usage_aut.yaml

No Vulnerabilities Found

🟡 Playwright Results — all passed (29 flaky)

✅ 3410 passed · ❌ 0 failed · 🟡 29 flaky · ⏭️ 216 skipped

📦 Download artifacts

# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@varun-lakhyani can you add unit tests for this

Added now

💡 Bug: for/else misuse: debug log triggers on normal loop completion
In yield_pipeline_lineage_details (metadata.py line 1331), there is a for/else construct on the for table_lineage in table_lineage_list loop. The else block logs "No source or target table full name found for {entity_id}", but Python's for/else triggers the else when the loop completes without a break — i.e., on every normal iteration through all items. This means the debug message fires after successfully processing all lineage entries, which is misleading. This is a pre-existing pattern, but the diff changes the variable used (entity_id replacing job_id), so worth noting.

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

@gitar-bot Fixed in 3b8731d - the for/else block has been removed entirely.

Thanks for confirming! The for/else block was removed in 3b8731d, so the misleading debug log no longer fires on normal loop completion.

Reviewing your code

Code Review ⚠️ Changes requested 3 resolved / 6 findings
Adds Databricks lineage pipeline support with some incomplete fixes: certification tags are silently lost during CSV import due to overly broad tag deletion, pagination may stop early if API responses lack total_entries field, and DAG detail building logic is duplicated across AirflowApiClient and MWAAClient.

⚠️ Bug: CSV import silently loses certification tags from tag_usage
updateTagsForImport() calls deleteTagsByTarget(fqn) which deletes ALL tags for the entity, including certification tags. When updateCertification() runs later, it compares the in-memory original and updated certification objects — if they are equal (certification unchanged), it returns without re-inserting the cert tag. The net result is that any entity with an existing certification will silently lose it during CSV bulk import.

The non-import updateTags() correctly filters out certification tags from addedTags/deletedTags (line 6767-6771), but updateTagsForImport() has no such guard because it does a blanket DELETE + re-INSERT.

To fix: either modify updateTagsForImport to preserve cert tags by using a more targeted delete that excludes certification prefix, or ensure updateCertification() always re-applies when the entity has a certification (regardless of whether it changed).

Suggested fix

protected void updateTagsForImport(
    String fqn, String fieldName, List<TagLabel> origTags, List<TagLabel> updatedTags) {
  origTags = listOrEmpty(origTags);
  updatedTags = Optional.ofNullable(updatedTags).orElse(new ArrayList<>());
  if (origTags.isEmpty() && updatedTags.isEmpty()) {
    return;
  }
  // Delete all tags except certification (handled by updateCertification)
  String certClassification = getCertificationClassification();
  if (certClassification != null) {
    daoCollection.tagUsageDAO().deleteNonCertTagsByTarget(fqn, certClassification);
  } else {
    daoCollection.tagUsageDAO().deleteTagsByTarget(fqn);
  }
  // ... rest unchanged

💡 Bug: Pagination stops early if API omits total_entries field
In both AirflowApiClient._paginate() and MWAAClient._paginate(), total is initialized to limit (e.g. 100). If the API response does not include total_entries, the fallback total = response.get("total_entries", len(result)) sets total to the number of items seen so far. When the first page is full (100 items), the offset advances to 100 and the loop exits, potentially missing remaining pages.

Airflow's stable API always returns total_entries, so this is mainly a risk with future API changes or MWAA quirks. Consider using a sentinel or checking if len(page) == limit as an additional continuation condition.

💡 Quality: Duplicate DAG detail building logic in AirflowApiClient and MWAAClient
build_dag_details(), get_dag_runs(), get_task_instances_for_run(), _paginate(), and tag-parsing logic are nearly identical between AirflowApiClient and MWAAClient. This duplication makes it easy for bug fixes to be applied in one place but not the other (the pagination issue above is a good example). Consider extracting shared parsing/model-building logic into a common base class or utility module, keeping only the HTTP transport layer different.

✅ 3 resolved
✅ Quality: Incomplete rename: query constants and alias still say 'job'

📄 ingestion/src/metadata/ingestion/source/database/databricks/queries.py:90 📄 ingestion/src/metadata/ingestion/source/database/databricks/queries.py:92 📄 ingestion/src/metadata/ingestion/source/database/databricks/queries.py:103 📄 ingestion/src/metadata/ingestion/source/database/databricks/queries.py:105 📄 ingestion/src/metadata/ingestion/source/database/databricks/client.py:351 📄 ingestion/src/metadata/ingestion/source/database/databricks/client.py:379
The renaming from job-specific to entity-generic was applied to the client's instance variables and method parameters, but the SQL query constants are still named DATABRICKS_GET_TABLE_LINEAGE_FOR_JOB / DATABRICKS_GET_COLUMN_LINEAGE_FOR_JOB, and the column alias is still entity_id AS job_id. The cache_lineage method also still accesses row.job_id (line 351, 379). While this works functionally (because the alias ensures the attribute name), it's inconsistent with the rest of the renaming effort and will confuse future readers who see 'job' in the query but 'entity' in the client code.

✅ Bug: Log message still says "all jobs" instead of "all entities"

📄 ingestion/src/metadata/ingestion/source/database/databricks/client.py:289
At line 289 of client.py, the log message reads "performing bulk lineage fetch for all jobs" but this method now fetches lineage for both jobs and pipelines. This is a minor inconsistency with the rename from job-specific to entity-generic terminology applied elsewhere in the PR.

✅ Bug: for/else misuse: debug log triggers on normal loop completion

📄 ingestion/src/metadata/ingestion/source/pipeline/databrickspipeline/metadata.py:1331-1334
In yield_pipeline_lineage_details (metadata.py line 1331), there is a for/else construct on the for table_lineage in table_lineage_list loop. The else block logs "No source or target table full name found for {entity_id}", but Python's for/else triggers the else when the loop completes without a break — i.e., on every normal iteration through all items. This means the debug message fires after successfully processing all lineage entries, which is misleading. This is a pre-existing pattern, but the diff changes the variable used (entity_id replacing job_id), so worth noting.

🤖 Prompt for agents

Code Review: Adds Databricks lineage pipeline support with some incomplete fixes: certification tags are silently lost during CSV import due to overly broad tag deletion, pagination may stop early if API responses lack total_entries field, and DAG detail building logic is duplicated across AirflowApiClient and MWAAClient.

1. ⚠️ Bug: CSV import silently loses certification tags from tag_usage

   `updateTagsForImport()` calls `deleteTagsByTarget(fqn)` which deletes ALL tags for the entity, including certification tags. When `updateCertification()` runs later, it compares the in-memory original and updated certification objects — if they are equal (certification unchanged), it returns without re-inserting the cert tag. The net result is that any entity with an existing certification will silently lose it during CSV bulk import.
   
   The non-import `updateTags()` correctly filters out certification tags from addedTags/deletedTags (line 6767-6771), but `updateTagsForImport()` has no such guard because it does a blanket DELETE + re-INSERT.
   
   To fix: either modify `updateTagsForImport` to preserve cert tags by using a more targeted delete that excludes certification prefix, or ensure `updateCertification()` always re-applies when the entity has a certification (regardless of whether it changed).

   Suggested fix:
   protected void updateTagsForImport(
       String fqn, String fieldName, List<TagLabel> origTags, List<TagLabel> updatedTags) {
     origTags = listOrEmpty(origTags);
     updatedTags = Optional.ofNullable(updatedTags).orElse(new ArrayList<>());
     if (origTags.isEmpty() && updatedTags.isEmpty()) {
       return;
     }
     // Delete all tags except certification (handled by updateCertification)
     String certClassification = getCertificationClassification();
     if (certClassification != null) {
       daoCollection.tagUsageDAO().deleteNonCertTagsByTarget(fqn, certClassification);
     } else {
       daoCollection.tagUsageDAO().deleteTagsByTarget(fqn);
     }
     // ... rest unchanged

2. 💡 Bug: Pagination stops early if API omits total_entries field

   In both `AirflowApiClient._paginate()` and `MWAAClient._paginate()`, `total` is initialized to `limit` (e.g. 100). If the API response does not include `total_entries`, the fallback `total = response.get("total_entries", len(result))` sets total to the number of items seen so far. When the first page is full (100 items), the offset advances to 100 and the loop exits, potentially missing remaining pages.
   
   Airflow's stable API always returns `total_entries`, so this is mainly a risk with future API changes or MWAA quirks. Consider using a sentinel or checking if `len(page) == limit` as an additional continuation condition.

3. 💡 Quality: Duplicate DAG detail building logic in AirflowApiClient and MWAAClient

   `build_dag_details()`, `get_dag_runs()`, `get_task_instances_for_run()`, `_paginate()`, and tag-parsing logic are nearly identical between `AirflowApiClient` and `MWAAClient`. This duplication makes it easy for bug fixes to be applied in one place but not the other (the pagination issue above is a good example). Consider extracting shared parsing/model-building logic into a common base class or utility module, keeping only the HTTP transport layer different.

Options
Display: compact → Showing less information.

Comment with these commands to change:

Compact

gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@gitar-bot All three findings are false positives: AirflowApiClient/MWAAClient don't exist in this codebase, total_entries appears nowhere in the code, and certification tags are stored in entity JSON (not tag_usage), so deleteTagsByTarget doesn't affect them

Code Review ⚠️ Changes requested 3 resolved / 8 findings
Adds Databricks lineage pipeline support but has three open issues: CSV import silently loses certification tags due to deleteTagsByTarget() removing all tags, pagination stops early when API omits total_entries field, and duplicate DAG detail building logic across AirflowApiClient and MWAAClient. Two items were resolved (incomplete rename and log message).

⚠️ Bug: CSV import silently loses certification tags from tag_usage
updateTagsForImport() calls deleteTagsByTarget(fqn) which deletes ALL tags for the entity, including certification tags. When updateCertification() runs later, it compares the in-memory original and updated certification objects — if they are equal (certification unchanged), it returns without re-inserting the cert tag. The net result is that any entity with an existing certification will silently lose it during CSV bulk import.

The non-import updateTags() correctly filters out certification tags from addedTags/deletedTags (line 6767-6771), but updateTagsForImport() has no such guard because it does a blanket DELETE + re-INSERT.

To fix: either modify updateTagsForImport to preserve cert tags by using a more targeted delete that excludes certification prefix, or ensure updateCertification() always re-applies when the entity has a certification (regardless of whether it changed).

Suggested fix

protected void updateTagsForImport(
    String fqn, String fieldName, List<TagLabel> origTags, List<TagLabel> updatedTags) {
  origTags = listOrEmpty(origTags);
  updatedTags = Optional.ofNullable(updatedTags).orElse(new ArrayList<>());
  if (origTags.isEmpty() && updatedTags.isEmpty()) {
    return;
  }
  // Delete all tags except certification (handled by updateCertification)
  String certClassification = getCertificationClassification();
  if (certClassification != null) {
    daoCollection.tagUsageDAO().deleteNonCertTagsByTarget(fqn, certClassification);
  } else {
    daoCollection.tagUsageDAO().deleteTagsByTarget(fqn);
  }
  // ... rest unchanged

⚠️ Bug: SLF4J migration drops exception message from log text
Several String.format → SLF4J conversions have one fewer {} placeholder than arguments. When the last argument is a Throwable, SLF4J treats it as the exception for stack-trace logging and does NOT substitute it into the message. This means the exception's message text is silently dropped from the log line.

Affected locations:

• OpenMetadataConnectionBuilder.java:80: message ends with "due to " but has no {} for the exception. Original format: "Could not initialize bot for pipeline [%s] due to [%s]".
• v150/MigrationUtil.java:108: "Error updating automator [{}] due to " — missing second {} for ex.
• v159/MigrationUtil.java:65: "Error migrating app extension [{}] due to " — same issue.

In each case the exception stack trace is still printed (because SLF4J auto-detects the trailing Throwable), but the human-readable message loses the exception text, making quick log scanning harder.

Suggested fix

// OpenMetadataConnectionBuilder.java:80
LOG.warn(
    "Could not initialize bot for pipeline [{}] due to [{}]",
    ingestionPipeline.getPipelineType(),
    e.getMessage(),
    e);

// v150/MigrationUtil.java:108
LOG.warn("Error updating automator [{}] due to [{}]", row, ex.getMessage(), ex);

// v159/MigrationUtil.java:65
LOG.warn("Error migrating app extension [{}] due to [{}]", row, ex.getMessage(), ex);

💡 Bug: Pagination stops early if API omits total_entries field
In both AirflowApiClient._paginate() and MWAAClient._paginate(), total is initialized to limit (e.g. 100). If the API response does not include total_entries, the fallback total = response.get("total_entries", len(result)) sets total to the number of items seen so far. When the first page is full (100 items), the offset advances to 100 and the loop exits, potentially missing remaining pages.

Airflow's stable API always returns total_entries, so this is mainly a risk with future API changes or MWAA quirks. Consider using a sentinel or checking if len(page) == limit as an additional continuation condition.

💡 Quality: Duplicate DAG detail building logic in AirflowApiClient and MWAAClient
build_dag_details(), get_dag_runs(), get_task_instances_for_run(), _paginate(), and tag-parsing logic are nearly identical between AirflowApiClient and MWAAClient. This duplication makes it easy for bug fixes to be applied in one place but not the other (the pagination issue above is a good example). Consider extracting shared parsing/model-building logic into a common base class or utility module, keeping only the HTTP transport layer different.

💡 Quality: Log messages missing separator before placeholder
Two LOG.error calls concatenate the message text directly with the exception message with no space, colon, or other separator before {}:

• "Error in sending invite to User{}" → renders as "Error in sending invite to UserNullPointerException"
• "Error in sending mail for reset password{}" → similar.

This was inherited from the original string-concatenation code, but the SLF4J migration was an opportunity to fix it.

Suggested fix

LOG.error("Error in sending invite to User: {}", ex.getMessage());
LOG.error("Error in sending mail for reset password: {}", ex.getMessage());

✅ 3 resolved
✅ Quality: Incomplete rename: query constants and alias still say 'job'

📄 ingestion/src/metadata/ingestion/source/database/databricks/queries.py:90 📄 ingestion/src/metadata/ingestion/source/database/databricks/queries.py:92 📄 ingestion/src/metadata/ingestion/source/database/databricks/queries.py:103 📄 ingestion/src/metadata/ingestion/source/database/databricks/queries.py:105 📄 ingestion/src/metadata/ingestion/source/database/databricks/client.py:351 📄 ingestion/src/metadata/ingestion/source/database/databricks/client.py:379
The renaming from job-specific to entity-generic was applied to the client's instance variables and method parameters, but the SQL query constants are still named DATABRICKS_GET_TABLE_LINEAGE_FOR_JOB / DATABRICKS_GET_COLUMN_LINEAGE_FOR_JOB, and the column alias is still entity_id AS job_id. The cache_lineage method also still accesses row.job_id (line 351, 379). While this works functionally (because the alias ensures the attribute name), it's inconsistent with the rest of the renaming effort and will confuse future readers who see 'job' in the query but 'entity' in the client code.

✅ Bug: Log message still says "all jobs" instead of "all entities"

📄 ingestion/src/metadata/ingestion/source/database/databricks/client.py:289
At line 289 of client.py, the log message reads "performing bulk lineage fetch for all jobs" but this method now fetches lineage for both jobs and pipelines. This is a minor inconsistency with the rename from job-specific to entity-generic terminology applied elsewhere in the PR.

✅ Bug: for/else misuse: debug log triggers on normal loop completion

📄 ingestion/src/metadata/ingestion/source/pipeline/databrickspipeline/metadata.py:1331-1334
In yield_pipeline_lineage_details (metadata.py line 1331), there is a for/else construct on the for table_lineage in table_lineage_list loop. The else block logs "No source or target table full name found for {entity_id}", but Python's for/else triggers the else when the loop completes without a break — i.e., on every normal iteration through all items. This means the debug message fires after successfully processing all lineage entries, which is misleading. This is a pre-existing pattern, but the diff changes the variable used (entity_id replacing job_id), so worth noting.

🤖 Prompt for agents

Code Review: Adds Databricks lineage pipeline support but has three open issues: CSV import silently loses certification tags due to deleteTagsByTarget() removing all tags, pagination stops early when API omits total_entries field, and duplicate DAG detail building logic across AirflowApiClient and MWAAClient. Two items were resolved (incomplete rename and log message).

1. ⚠️ Bug: CSV import silently loses certification tags from tag_usage

   `updateTagsForImport()` calls `deleteTagsByTarget(fqn)` which deletes ALL tags for the entity, including certification tags. When `updateCertification()` runs later, it compares the in-memory original and updated certification objects — if they are equal (certification unchanged), it returns without re-inserting the cert tag. The net result is that any entity with an existing certification will silently lose it during CSV bulk import.
   
   The non-import `updateTags()` correctly filters out certification tags from addedTags/deletedTags (line 6767-6771), but `updateTagsForImport()` has no such guard because it does a blanket DELETE + re-INSERT.
   
   To fix: either modify `updateTagsForImport` to preserve cert tags by using a more targeted delete that excludes certification prefix, or ensure `updateCertification()` always re-applies when the entity has a certification (regardless of whether it changed).

   Suggested fix:
   protected void updateTagsForImport(
       String fqn, String fieldName, List<TagLabel> origTags, List<TagLabel> updatedTags) {
     origTags = listOrEmpty(origTags);
     updatedTags = Optional.ofNullable(updatedTags).orElse(new ArrayList<>());
     if (origTags.isEmpty() && updatedTags.isEmpty()) {
       return;
     }
     // Delete all tags except certification (handled by updateCertification)
     String certClassification = getCertificationClassification();
     if (certClassification != null) {
       daoCollection.tagUsageDAO().deleteNonCertTagsByTarget(fqn, certClassification);
     } else {
       daoCollection.tagUsageDAO().deleteTagsByTarget(fqn);
     }
     // ... rest unchanged

2. 💡 Bug: Pagination stops early if API omits total_entries field

   In both `AirflowApiClient._paginate()` and `MWAAClient._paginate()`, `total` is initialized to `limit` (e.g. 100). If the API response does not include `total_entries`, the fallback `total = response.get("total_entries", len(result))` sets total to the number of items seen so far. When the first page is full (100 items), the offset advances to 100 and the loop exits, potentially missing remaining pages.
   
   Airflow's stable API always returns `total_entries`, so this is mainly a risk with future API changes or MWAA quirks. Consider using a sentinel or checking if `len(page) == limit` as an additional continuation condition.

3. 💡 Quality: Duplicate DAG detail building logic in AirflowApiClient and MWAAClient

   `build_dag_details()`, `get_dag_runs()`, `get_task_instances_for_run()`, `_paginate()`, and tag-parsing logic are nearly identical between `AirflowApiClient` and `MWAAClient`. This duplication makes it easy for bug fixes to be applied in one place but not the other (the pagination issue above is a good example). Consider extracting shared parsing/model-building logic into a common base class or utility module, keeping only the HTTP transport layer different.

4. ⚠️ Bug: SLF4J migration drops exception message from log text

   Several `String.format` → SLF4J conversions have one fewer `{}` placeholder than arguments. When the last argument is a `Throwable`, SLF4J treats it as the exception for stack-trace logging and does NOT substitute it into the message. This means the exception's message text is silently dropped from the log line.
   
   Affected locations:
   - `OpenMetadataConnectionBuilder.java:80`: message ends with `"due to "` but has no `{}` for the exception. Original format: `"Could not initialize bot for pipeline [%s] due to [%s]"`.
   - `v150/MigrationUtil.java:108`: `"Error updating automator [{}] due to "` — missing second `{}` for `ex`.
   - `v159/MigrationUtil.java:65`: `"Error migrating app extension [{}] due to "` — same issue.
   
   In each case the exception stack trace is still printed (because SLF4J auto-detects the trailing Throwable), but the human-readable message loses the exception text, making quick log scanning harder.

   Suggested fix:
   // OpenMetadataConnectionBuilder.java:80
   LOG.warn(
       "Could not initialize bot for pipeline [{}] due to [{}]",
       ingestionPipeline.getPipelineType(),
       e.getMessage(),
       e);
   
   // v150/MigrationUtil.java:108
   LOG.warn("Error updating automator [{}] due to [{}]", row, ex.getMessage(), ex);
   
   // v159/MigrationUtil.java:65
   LOG.warn("Error migrating app extension [{}] due to [{}]", row, ex.getMessage(), ex);

5. 💡 Quality: Log messages missing separator before placeholder

   Two `LOG.error` calls concatenate the message text directly with the exception message with no space, colon, or other separator before `{}`:
   - `"Error in sending invite to User{}"` → renders as `"Error in sending invite to UserNullPointerException"`
   - `"Error in sending mail for reset password{}"` → similar.
   
   This was inherited from the original string-concatenation code, but the SLF4J migration was an opportunity to fix it.

   Suggested fix:
   LOG.error("Error in sending invite to User: {}", ex.getMessage());
   LOG.error("Error in sending mail for reset password: {}", ex.getMessage());

Options
Display: verbose → Showing more information.

Comment with these commands to change:

Verbose

gitar display:compact         

Was this helpful? React with 👍 / 👎 | Gitar

@gitar-bot these are all unrelated issues caused by Update Branch, not pr changes

protected void updateTagsForImport(
    String fqn, String fieldName, List<TagLabel> origTags, List<TagLabel> updatedTags) {
  origTags = listOrEmpty(origTags);
  updatedTags = Optional.ofNullable(updatedTags).orElse(new ArrayList<>());
  if (origTags.isEmpty() && updatedTags.isEmpty()) {
    return;
  }
  // Delete all tags except certification (handled by updateCertification)
  String certClassification = getCertificationClassification();
  if (certClassification != null) {
    daoCollection.tagUsageDAO().deleteNonCertTagsByTarget(fqn, certClassification);
  } else {
    daoCollection.tagUsageDAO().deleteTagsByTarget(fqn);
  }
  // ... rest unchanged

// OpenMetadataConnectionBuilder.java:80
LOG.warn(
    "Could not initialize bot for pipeline [{}] due to [{}]",
    ingestionPipeline.getPipelineType(),
    e.getMessage(),
    e);

// v150/MigrationUtil.java:108
LOG.warn("Error updating automator [{}] due to [{}]", row, ex.getMessage(), ex);

// v159/MigrationUtil.java:65
LOG.warn("Error migrating app extension [{}] due to [{}]", row, ex.getMessage(), ex);

LOG.error("Error in sending invite to User: {}", ex.getMessage());
LOG.error("Error in sending mail for reset password: {}", ex.getMessage());

Code Review: Databricks lineage pipeline addition addresses the incomplete rename and log message issues, but three critical problems remain: CSV import silently drops certification tags during update, pagination stops early when total_entries is omitted, and SLF4J migration loses exception messages in logs.

1. ⚠️ Bug: CSV import silently loses certification tags from tag_usage

   `updateTagsForImport()` calls `deleteTagsByTarget(fqn)` which deletes ALL tags for the entity, including certification tags. When `updateCertification()` runs later, it compares the in-memory original and updated certification objects — if they are equal (certification unchanged), it returns without re-inserting the cert tag. The net result is that any entity with an existing certification will silently lose it during CSV bulk import.
   
   The non-import `updateTags()` correctly filters out certification tags from addedTags/deletedTags (line 6767-6771), but `updateTagsForImport()` has no such guard because it does a blanket DELETE + re-INSERT.
   
   To fix: either modify `updateTagsForImport` to preserve cert tags by using a more targeted delete that excludes certification prefix, or ensure `updateCertification()` always re-applies when the entity has a certification (regardless of whether it changed).

   Suggested fix:
   protected void updateTagsForImport(
       String fqn, String fieldName, List<TagLabel> origTags, List<TagLabel> updatedTags) {
     origTags = listOrEmpty(origTags);
     updatedTags = Optional.ofNullable(updatedTags).orElse(new ArrayList<>());
     if (origTags.isEmpty() && updatedTags.isEmpty()) {
       return;
     }
     // Delete all tags except certification (handled by updateCertification)
     String certClassification = getCertificationClassification();
     if (certClassification != null) {
       daoCollection.tagUsageDAO().deleteNonCertTagsByTarget(fqn, certClassification);
     } else {
       daoCollection.tagUsageDAO().deleteTagsByTarget(fqn);
     }
     // ... rest unchanged

2. 💡 Bug: Pagination stops early if API omits total_entries field

   In both `AirflowApiClient._paginate()` and `MWAAClient._paginate()`, `total` is initialized to `limit` (e.g. 100). If the API response does not include `total_entries`, the fallback `total = response.get("total_entries", len(result))` sets total to the number of items seen so far. When the first page is full (100 items), the offset advances to 100 and the loop exits, potentially missing remaining pages.
   
   Airflow's stable API always returns `total_entries`, so this is mainly a risk with future API changes or MWAA quirks. Consider using a sentinel or checking if `len(page) == limit` as an additional continuation condition.

3. 💡 Quality: Duplicate DAG detail building logic in AirflowApiClient and MWAAClient

   `build_dag_details()`, `get_dag_runs()`, `get_task_instances_for_run()`, `_paginate()`, and tag-parsing logic are nearly identical between `AirflowApiClient` and `MWAAClient`. This duplication makes it easy for bug fixes to be applied in one place but not the other (the pagination issue above is a good example). Consider extracting shared parsing/model-building logic into a common base class or utility module, keeping only the HTTP transport layer different.

4. ⚠️ Bug: SLF4J migration drops exception message from log text

   Several `String.format` → SLF4J conversions have one fewer `{}` placeholder than arguments. When the last argument is a `Throwable`, SLF4J treats it as the exception for stack-trace logging and does NOT substitute it into the message. This means the exception's message text is silently dropped from the log line.
   
   Affected locations:
   - `OpenMetadataConnectionBuilder.java:80`: message ends with `"due to "` but has no `{}` for the exception. Original format: `"Could not initialize bot for pipeline [%s] due to [%s]"`.
   - `v150/MigrationUtil.java:108`: `"Error updating automator [{}] due to "` — missing second `{}` for `ex`.
   - `v159/MigrationUtil.java:65`: `"Error migrating app extension [{}] due to "` — same issue.
   
   In each case the exception stack trace is still printed (because SLF4J auto-detects the trailing Throwable), but the human-readable message loses the exception text, making quick log scanning harder.

   Suggested fix:
   // OpenMetadataConnectionBuilder.java:80
   LOG.warn(
       "Could not initialize bot for pipeline [{}] due to [{}]",
       ingestionPipeline.getPipelineType(),
       e.getMessage(),
       e);
   
   // v150/MigrationUtil.java:108
   LOG.warn("Error updating automator [{}] due to [{}]", row, ex.getMessage(), ex);
   
   // v159/MigrationUtil.java:65
   LOG.warn("Error migrating app extension [{}] due to [{}]", row, ex.getMessage(), ex);

5. 💡 Quality: Log messages missing separator before placeholder

   Two `LOG.error` calls concatenate the message text directly with the exception message with no space, colon, or other separator before `{}`:
   - `"Error in sending invite to User{}"` → renders as `"Error in sending invite to UserNullPointerException"`
   - `"Error in sending mail for reset password{}"` → similar.
   
   This was inherited from the original string-concatenation code, but the SLF4J migration was an opportunity to fix it.

   Suggested fix:
   LOG.error("Error in sending invite to User: {}", ex.getMessage());
   LOG.error("Error in sending mail for reset password: {}", ex.getMessage());

gitar display:verbose         

_{Was this helpful? React with 👍 / 👎 | Gitar}

Quality Gate passed for 'open-metadata-ingestion'

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
44.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud