feat: introduce validateFields method to SourceConnectionConfig and implement validation for all source configurations#3956
Conversation
pratickchokhani
force-pushed
the
shard-config-validate-fields
branch
from
eb6bb70 to
29e854d
Compare
…mplement validation for all source configurations
pratickchokhani
force-pushed
the
shard-config-validate-fields
branch
from
29e854d to
6c65bdc
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (37.50%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3956 +/- ##
============================================
+ Coverage 55.64% 63.32% +7.67%
+ Complexity 6731 2455 -4276
============================================
Files 1117 528 -589
Lines 68313 30636 -37677
Branches 7696 3409 -4287
============================================
- Hits 38012 19399 -18613
+ Misses 27829 10210 -17619
+ Partials 2472 1027 -1445
🚀 New features to boost your workflow:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a standardized validation mechanism for source connection configurations within the migration framework. By defining a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
pratickchokhani
requested review from
bharadwaj-aditya
and removed request for
darshan-sj and
rohitwali
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a validateFields method to the SourceConnectionConfig interface and implements validation logic across various shard and connection configuration classes, including Shard, AstraConnectionConfig, CassandraConnectionConfig, and JdbcShardConfig. It also updates SourceConfigParser to trigger these validations and adds corresponding unit tests. The review feedback points out that because the parser deserializes nulls to empty strings, the validation checks in Shard and AstraConnectionConfig should be updated to check for both null and empty values to prevent invalid configurations from bypassing validation.
| throw new IllegalArgumentException("namespace cannot be null"); | ||
| } | ||
| if (secretManagerUri == null) { | ||
| throw new IllegalArgumentException("secretManagerUri cannot be null"); |
There was a problem hiding this comment.
secret manager or password can be null. It is possible to pass only one of them.
Can you please verify the logic here ?
There was a problem hiding this comment.
We have integration tests for both secretManagerUri and password. Unless someone sets the value as null manually in the JSON or HOCON string, these values will not be null.
Hence, this check is added.
There was a problem hiding this comment.
2 questions
- Is there a scenario where someone would want to set these fields explicitly as null and not empty strings? Maybe for password ?
- Does the integration test handle the scenario where all 4 combinations of these 2 parameters are set ?
| * Validates that the source configuration is valid. Throws IllegalArgumentException if the | ||
| * configuration is invalid. | ||
| */ | ||
| void validateFields(); |
There was a problem hiding this comment.
If the validation is implemented at the source, then it can be private.
This method should not be required.
There was a problem hiding this comment.
This only validates the null checks and basic checks. It is called by the SourceConfigParser before returning the config.
Interface fields cannot be private.
There was a problem hiding this comment.
If validate is called in each of the flows right after reading the config, then this validation is not really achieving anything. Especially if the code of what to validate is within the source implementation.
The value of the validate would be if it lies outside of the source implementation. Can you think of a scenario where the source specific validation can not be done while reading the source config ?
2 participants
No description provided.