fix(graphql/upsertIngestionSource): Validate cron schedule; parse error in CLI

[asikowitz]

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)
• Tests for the changes have been added/updated (if applicable)
• Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
• For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

Summary by CodeRabbit

• New Features

  • Integrated Spring framework support for enhanced application context management.
  • Added robust error handling for GraphQL query executions, improving user feedback during failures.
  • Introduced flexible error formatting options in GraphQL query execution.

• Bug Fixes

  • Enhanced validation for cron intervals and timezones to prevent erroneous configurations.

• Tests

  • Added new test cases for handling invalid cron expressions and timezones, improving test coverage and reliability.

[coderabbitai]

Walkthrough

The recent changes enhance the datahub-graphql-core project by integrating Spring's context features and improving the authorization, validation, and error handling mechanisms within key components. The introduction of new test cases and error management in CLI functions bolsters the robustness and reliability of the application, ensuring clearer feedback for users and better management of edge cases.

Changes

File	Change Summary
datahub-graphql-core/build.gradle	Added springContext dependency to leverage Spring's context features, enhancing dependency management and application wiring.
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/...	Enhanced authorization and validation logic in UpsertIngestionSourceResolver. Introduced adjustCronInterval method, improved error handling for URNs, and added cron/timezone validation.
datahub-graphql-core/src/test/java/com/linkedin/datahub/graphql/...	Refactored test cases for better clarity. Added tests for invalid cron and timezone inputs to ensure proper exception handling in UpsertIngestionSourceResolver.
metadata-ingestion/src/datahub/cli/ingest_cli.py	Improved error handling in the deploy function to manage GraphQL query execution errors, providing clearer feedback and handling failures gracefully.
metadata-ingestion/src/datahub/ingestion/graph/client.py	Introduced format_exception parameter in execute_graphql to allow flexible error reporting, either in formatted or raw form.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant GraphQL
    participant Client
    User->>CLI: Execute Deployment
    CLI->>GraphQL: Execute Query
    alt Error Occurs
        GraphQL-->>CLI: GraphError
        CLI->>CLI: Parse Error Message
        CLI-->>User: Display Error
    else Success
        GraphQL-->>CLI: Success Response
        CLI-->>User: Display Confirmation
    end

Loading

Poem

🐇 In a world of code, so bright and fair,
Where rabbits code with utmost care,
New features hop in, spry and quick,
Enhancing functions with a flick!
With spring and tests, our project sings,
A joyful dance of techy things! 🎉

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

[asikowitz]

Moved everything but ingestProposal out of the async portion... rest should be really fast and makes testing simpler

[asikowitz]

Parsing graphql errors seems super annoying... this is just to display an error to the user and has a blanket try / catch so think this is safe to put in

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 01b3461 and a38b2a3.

Files selected for processing (5)

• datahub-graphql-core/build.gradle (1 hunks)
• datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ingest/source/UpsertIngestionSourceResolver.java (3 hunks)
• datahub-graphql-core/src/test/java/com/linkedin/datahub/graphql/resolvers/ingest/source/UpsertIngestionSourceResolverTest.java (3 hunks)
• metadata-ingestion/src/datahub/cli/ingest_cli.py (2 hunks)
• metadata-ingestion/src/datahub/ingestion/graph/client.py (2 hunks)

Additional comments not posted (14)

datahub-graphql-core/build.gradle (1)

25-25: Dependency Addition: Ensure necessity and compatibility.

The springContext dependency has been added. Verify that this addition is necessary and compatible with the existing project dependencies.

Verification successful

---

Dependency Addition: Ensure necessity and compatibility.

The springContext dependency has been added, and its usage is confirmed across multiple files in the codebase. To ensure compatibility, verify that there are no conflicts with other dependencies.

• Files with Spring context usage:
  • metadata-service/openapi-entity-servlet/src/test/java/io/datahubproject/openapi/v2/delegates/EntityApiDelegateImplTest.java
  • metadata-service/war/src/main/java/com/linkedin/gms/CommonApplicationConfig.java
  • metadata-service/war/src/main/java/com/linkedin/gms/servlet/SchemaRegistryServletConfig.java
  • (and others as listed in the provided context)

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the necessity and compatibility of the `springContext` dependency.

# Test: Check for usage of Spring context features in the codebase. Expect: At least one usage.
rg --type java -A 5 'import org.springframework.context'

Length of output: 222113

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ingest/source/UpsertIngestionSourceResolver.java (6)

28-37: Review Added Imports: Ensure necessity and correctness.

The new imports for DateTimeException, ZoneId, Objects, Optional, UUID, CompletableFuture, Collectors, Slf4j, and CronExpression are added. Verify that these imports are necessary and used correctly in the code.

---

64-85: Error Handling and Proposal Creation: Verify correctness.

The error handling for malformed URNs and proposal creation is added. Ensure that errors are correctly handled and proposals are correctly created.

---

53-56: Authorization Check: Verify correctness.

The authorization check is added at the beginning of the get method. Ensure that unauthorized access is correctly handled.

Verification successful

---

Authorization Check Verified:

The authorization check using IngestionAuthUtils.canManageIngestion is consistently applied across multiple resolver classes, including UpsertIngestionSourceResolver. Unauthorized access is correctly handled by throwing an AuthorizationException.

• UpsertIngestionSourceResolver.java: Lines 53-56

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the authorization check.

# Test: Check for usage of `IngestionAuthUtils.canManageIngestion` method. Expect: Correct handling of unauthorized access.
rg --type java -A 5 'IngestionAuthUtils.canManageIngestion'

Length of output: 11380

---

163-173: New Method: Verify correctness of adjustCronInterval.

The new method adjustCronInterval is added. Ensure that the method correctly adjusts the cron interval.

Verification successful

---

Verified correctness of adjustCronInterval method.

The adjustCronInterval method correctly adjusts 5-part cron expressions to 6-part expressions by adding a leading zero, ensuring compatibility with the Spring library. The method is used in contexts where the adjusted cron interval is validated, confirming its correctness.

• IngestionScheduler.java
• UpsertIngestionSourceResolver.java

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the new method `adjustCronInterval`.

# Test: Check for usage of `adjustCronInterval` method. Expect: Correct adjustment of cron interval.
rg --type java -A 5 'adjustCronInterval'

Length of output: 4087

---

57-59: Input Handling: Verify correctness.

The handling of ingestionSourceUrn and input is added. Ensure that the inputs are correctly processed and validated.

Verification successful

---

Input Handling: Verify correctness.

The handling of ingestionSourceUrn and input is added. Ensure that the inputs are correctly processed and validated.

• Check if the inputs (ingestionSourceUrn and input) are validated after being fetched and bound.
• Ensure there is proper error handling in case of invalid inputs.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the input handling.

# Test: Check for usage of `environment.getArgument` method. Expect: Correct processing and validation of inputs.
rg --type java -A 5 'environment.getArgument'

Length of output: 181724

---

140-155: Validation Checks: Verify correctness.

The validation checks for cron interval and timezone are added. Ensure that the inputs are correctly validated.

Verification successful

---

Validation Checks: Verified correctness.

The validation checks for cron interval and timezone in the UpsertIngestionSourceResolver.java file are correctly implemented. The adjustCronInterval method ensures that a 5-character cron interval is adjusted to a 6-character format, which is necessary for compatibility with the Spring library.

• The CronExpression.parse method is used to validate the modified cron interval.
• The ZoneId.of method is used to validate the timezone input.
• The adjustCronInterval method correctly handles the conversion of 5-character cron intervals to 6-character intervals.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the validation checks.

# Test: Check for usage of `CronExpression.parse` and `ZoneId.of` methods. Expect: Correct validation of inputs.
rg --type java -A 5 'CronExpression.parse'
rg --type java -A 5 'ZoneId.of'

Length of output: 2992

---

Script:

#!/bin/bash
# Description: Verify the implementation of the `adjustCronInterval` method.

# Search for the `adjustCronInterval` method definition and its usages.
ast-grep --lang java --pattern 'String adjustCronInterval($_) { $$$ }'

Length of output: 72

---

Script:

#!/bin/bash
# Description: Verify the implementation and usage of the `adjustCronInterval` method.

# Search for the `adjustCronInterval` method definition.
rg --type java 'String adjustCronInterval'

# Search for any usages of the `adjustCronInterval` method.
rg --type java 'adjustCronInterval'

Length of output: 1159

---

Script:

#!/bin/bash
# Description: Extract the implementation details of the `adjustCronInterval` method in `UpsertIngestionSourceResolver.java`.

# Extract the implementation of the `adjustCronInterval` method.
rg --type java -A 10 'private String adjustCronInterval' datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ingest/source/UpsertIngestionSourceResolver.java

Length of output: 665

datahub-graphql-core/src/test/java/com/linkedin/datahub/graphql/resolvers/ingest/source/UpsertIngestionSourceResolverTest.java (4)

112-135: New Test Case: Verify correctness of testUpsertWithInvalidCron.

The new test case testUpsertWithInvalidCron is added. Ensure that the test case correctly validates the behavior of the resolver with invalid cron expressions.

---

137-160: New Test Case: Verify correctness of testUpsertWithInvalidTimezone.

The new test case testUpsertWithInvalidTimezone is added. Ensure that the test case correctly validates the behavior of the resolver with invalid timezones.

---

26-36: Refactored Code: Verify correctness and maintainability.

The code is refactored to encapsulate UpdateIngestionSourceInput instantiation into makeInput method. Ensure that the refactored code improves readability and maintainability without introducing issues.

---

Line range hint 38-110:
Existing Test Cases: Verify correctness.

The existing test cases testGetSuccess, testGetUnauthorized, and testGetEntityClientException are present. Ensure that the test cases are correctly updated to reflect the changes in the resolver.

metadata-ingestion/src/datahub/cli/ingest_cli.py (1)

375-387: Enhanced error handling for GraphQL query execution.

The added try block improves robustness by handling GraphError exceptions. If parsing the error message into JSON format fails, a generic error message is displayed. This ensures that errors related to GraphQL execution are managed gracefully, providing clear feedback to the user.

metadata-ingestion/src/datahub/ingestion/graph/client.py (2)

1136-1136: New parameter format_exception added to execute_graphql function.

The new parameter format_exception enhances the flexibility of error reporting by allowing callers to choose between a formatted error message or a raw error response.

---

1153-1156: Updated error handling logic in execute_graphql function.

The error handling logic now checks the value of format_exception. If True, the error message is formatted to include the string "Error executing graphql query" followed by the actual errors. If False, the error is raised without additional formatting, directly presenting the errors from the result.

[hsheth2]

ingestion changes seem fine

[hsheth2]

the "right" way to do this would be to create a GraphqlError that has a .errors attribute

[hsheth2]

it would have a nice __str__ method, but would still let us introspect the raw error info

[chriscollins3456]

makes sense