Skip to content

Conversation

aaronburtle
Copy link
Contributor

@aaronburtle aaronburtle commented May 3, 2024

Why make this change?

Closes #2184
Closes #2180

What is this change?

When we don't properly initialize the SqlMetadataProvider, certain functions will not behave as intended. This change adds some defensive programming around this fact so that we have more readable exceptions in these cases, as well as restructuring the validation to avoid the potential of such a circumstance.

We refactor the ValidateEntitiesMetadata function into 2 functions, one called ValidateRelationshipConfigCorrectness that validates the relationships in the config without needing to cross reference database metadata, and leaving the remaining validations that do require such metadata within the original function. This validation is then handled by a mix of the initialization of the metadata provider (this init process does some validation), and the call to ValidateRelationships which happens from within ValidateEntitiesMetadata only when the initialization of the metadata provider resulted in no connection errors.

Therefore, the normal code path when doing dab validate will be

  1. Call ValidateRelationshipConfigCorrectness
  2. Call ValidateEntitiesMetadata
    2a. If there were no connection errors Call ValidateRelationships

image

How was this tested?

Manually, against our test suite, and we add a regression test that looks for the correct error messaging when the config that caused the error from the finding of the bug is used.

Sample Request(s)

This can be reprod by using the command `dab validate -c

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

few nits, but looks good!

@seantleonard
Copy link
Contributor

/azp run

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Why do we validate backing columns if we know connection string is incorrect?

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

some additional changes needed.

@aaronburtle
Copy link
Contributor Author

/azp run

@ayush3797
Copy link
Contributor

Might not need this clear:

based on the resolution of previous comments

Copy link
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

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

Some suggestions. LGTM otherwise! Thanks for fixing the dab validate command!

@seantleonard
Copy link
Contributor

/azp run

@seantleonard seantleonard requested a review from Aniruddh25 May 9, 2024 17:25
@seantleonard seantleonard dismissed Aniruddh25’s stale review May 9, 2024 19:12

feedback addressed, removing stale review.

@seantleonard seantleonard merged commit 7804517 into main May 9, 2024
@seantleonard seantleonard deleted the dev/aaronburtle/DabValidateUnhandledException branch May 9, 2024 19:12
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.

[Bug]: Validating entity relationships throws unhandled exception [Bug]: New relationship validations causing unexpected exception being thrown
4 participants