Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Validation for PK Mode Against PK Fields and Unit Tests #335

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

Joel-hanson
Copy link
Contributor

This PR introduces validation logic for the primary key mode (pk.mode) against the primary key fields (pk.fields) in the JdbcSinkConfig class, ensuring that the configurations adhere to the documented requirements. Additionally, unit tests have been added to verify the correctness of the validation logic.

Changes:

  • Implemented validatePKModeAgainstPKFields method to validate the primary key mode against the primary key fields.
  • Added checks for various combinations of pk.mode and pk.fields to ensure configurations are valid according to the documentation.

@Joel-hanson Joel-hanson marked this pull request as ready for review June 21, 2024 13:19
@Joel-hanson Joel-hanson requested review from a team as code owners June 21, 2024 13:19

final Config config = new JdbcSinkConnector().validate(props);

assertFalse(config.configValues().stream().allMatch(cv -> cv.errorMessages().size() > 0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This asserts that there's at least one property that didn't have an error message. I'm guessing we actually want to assert that no properties have error messages?

Suggested change
assertFalse(config.configValues().stream().allMatch(cv -> cv.errorMessages().size() > 0));
assertFalse(config.configValues().stream().anyMatch(cv -> cv.errorMessages().size() > 0));

Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Joel-hanson, this looks fantastic! Only have one small comment about a test case, everything else LGTM. If the suggestion makes sense, you can just amend the single current commit and force push, then we can merge.

Signed-off-by: Joel Hanson <[email protected]>
Signed-off-by: Joel Hanson <[email protected]>
@Joel-hanson
Copy link
Contributor Author

@C0urante you are right and I have added in that change.

@C0urante C0urante merged commit b0a104b into Aiven-Open:master Jul 2, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants