Sitelet https://github.com/GoogleCloudPlatform/cloud-data-quality/pull/170
Skip to content

validate configs before loading into cache - #170

Merged
thinhha merged 2 commits into
mainfrom
rb-error
May 5, 2022
Merged

validate configs before loading into cache#170
thinhha merged 2 commits into
mainfrom
rb-error

Conversation

@thinhha

@thinhha thinhha commented May 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #156

Error message is now:

Traceback (most recent call last):
  File "/home/thinhha_google_com/.cache/bazel/_bazel_thinhha_google_com/b633505a2816fec546811907bc138b6c/execroot/clouddq/bazel-out/k8-fastbuild/bin/clouddq/clouddq.runfiles/clouddq/clouddq/main.py", line 452, in main
    configs_cache = lib.prepare_configs_cache(configs_path=Path(configs_path))
  File "/home/thinhha_google_com/.cache/bazel/_bazel_thinhha_google_com/b633505a2816fec546811907bc138b6c/execroot/clouddq/bazel-out/k8-fastbuild/bin/clouddq/clouddq.runfiles/clouddq/clouddq/lib.py", line 218, in prepare_configs_cache
    configs_cache.load_all_rule_bindings_collection(rule_binding_collection)
  File "/home/thinhha_google_com/.cache/bazel/_bazel_thinhha_google_com/b633505a2816fec546811907bc138b6c/execroot/clouddq/bazel-out/k8-fastbuild/bin/clouddq/clouddq.runfiles/clouddq/clouddq/classes/dq_configs_cache.py", line 182, in load_all_rule_bindings_collection
    raise ValueError(f"Failed to parse Rule Binding with error:\n{e}\n")
ValueError: Failed to parse Rule Binding with error:
Assertion failed on value 'NOT_NULL_SIMPLE'.
Config ID: 4209174C-2568-49D2-8D6D-7ABEAE4435FC, Config arguments:
{'column_id': 'VALUE',
 'entity_uri': 'dataplex://projects/<my-gcp-project-id>/locations/<my-gcp-dataplex-region-id>/lakes/<my-gcp-dataplex-lake-id>/zones/<my-gcp-dataplex-zone-id>/entities/contact_details',
 'id': '4209174C-2568-49D2-8D6D-7ABEAE4435FC',
 'metadata': {'brand': 'one'},
 'row_filter_id': 'NONE',
 'rule_ids': 'NOT_NULL_SIMPLE'}.
Error: Rule Binding ID: '4209174C-2568-49D2-8D6D-7ABEAE4435FC' must have defined value 'rule_ids' of type 'list'.

To prevent error messages due to rule bindings entity_uris validation failing due to missing default arguments from metadata registry defaults (which would lead to a circular dependency), I've added a validate_uri flag to the entity_uris from_dict method which is True by default and set to False only for the purpose of this initial validation.

I also updated type annotations in clouddq/lib.py to the consistent with everywhere else.

@thinhha
thinhha requested review from AmandeepSinghCS and nauh May 2, 2022 16:41
nauh
nauh reviewed May 2, 2022
View reviewed changes
Comment thread clouddq/classes/dq_entity_uri.py
cls: EntityUri,
uri_string: str,
default_configs: dict | None = None,
validate_uri: bool = True,

@nauh nauh May 2, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there any reason that we don't want to validate URI in certain cases?

@thinhha thinhha May 2, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the only scenario so far is for the purpose of this PR: validating that we can successfully parse a Rule Binding configs without assuming that we have already parsed the metadata_registry_defaults arguments which contains information required to successfully validate the entity_uris.

To be clear, this flag was added purely for the purpose of this PR. This seemed like a cleaner way to avoid introducing breaking API changes to existing interfaces.

@thinhha thinhha May 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nauh PTAL please let me know if there's any further question!

To be clear, I had to add the validate_uri argument to prevent an error with uri validation failing due to the 'default_configs' not being available at this point to be included in the URI instantiation.

The error this fixes in the test_cli_unit test is:

Traceback (most recent call last):
  File "/home/thinhha_google_com/.cache/bazel/_bazel_thinhha_google_com/b633505a2816fec546811907bc138b6c/sandbox/linux-sandbox/1/execroot/clouddq/bazel-out/k8-fastbuild/bin/tests/test_cli_unit.runfiles/clouddq/clouddq/classes/dq_configs_cache.py", line 178, in load_all_rule_bindings_collection
    dq_rule_binding.DqRuleBinding.from_dict(
  File "/home/thinhha_google_com/.cache/bazel/_bazel_thinhha_google_com/b633505a2816fec546811907bc138b6c/sandbox/linux-sandbox/1/execroot/clouddq/bazel-out/k8-fastbuild/bin/tests/test_cli_unit.runfiles/clouddq/clouddq/classes/dq_rule_binding.py", line 79, in from_dict
    parsed_entity_uri = EntityUri.from_uri(
  File "/home/thinhha_google_com/.cache/bazel/_bazel_thinhha_google_com/b633505a2816fec546811907bc138b6c/sandbox/linux-sandbox/1/execroot/clouddq/bazel-out/k8-fastbuild/bin/tests/test_cli_unit.runfiles/clouddq/clouddq/classes/dq_entity_uri.py", line 86, in from_uri
    entity_uri.validate()
  File "/home/thinhha_google_com/.cache/bazel/_bazel_thinhha_google_com/b633505a2816fec546811907bc138b6c/sandbox/linux-sandbox/1/execroot/clouddq/bazel-out/k8-fastbuild/bin/tests/test_cli_unit.runfiles/clouddq/clouddq/classes/dq_entity_uri.py", line 112, in validate
    self._validate_dataplex_uri_fields(
  File "/home/thinhha_google_com/.cache/bazel/_bazel_thinhha_google_com/b633505a2816fec546811907bc138b6c/sandbox/linux-sandbox/1/execroot/clouddq/bazel-out/k8-fastbuild/bin/tests/test_cli_unit.runfiles/clouddq/clouddq/classes/dq_entity_uri.py", line 165, in _validate_dataplex_uri_fields
    raise ValueError(
ValueError: Required argument 'projects' not found in  entity_uri: dataplex://zones/<my-gcp-dataplex-zone-id>/entities/asset_bucket.
Either add 'projects' to the URI or specify default Dataplex configs for projects/locations/lakes/zones as part of a YAML 'metadata_default_registries' config, e.g.

metadata_registry_defaults:
  dataplex:
    projects: <my-gcp-project-id>
    locations: <my-gcp-dataplex-region-id>
    lakes: <my-gcp-dataplex-lake-id>
    zones: <my-gcp-dataplex-zone-id>

AmandeepSinghCS
AmandeepSinghCS approved these changes May 3, 2022
View reviewed changes

@AmandeepSinghCS AmandeepSinghCS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Thanks !

@thinhha
thinhha requested a review from nauh May 3, 2022 13:44
@thinhha

thinhha commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

thank you!

@thinhha
thinhha merged commit e859d87 into main May 5, 2022
@thinhha
thinhha deleted the rb-error branch May 5, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nauh nauh nauh approved these changes
+1 more reviewer
@AmandeepSinghCS AmandeepSinghCS AmandeepSinghCS approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enhancement: Better diagnostics for errors.

3 participants

@thinhha @nauh @AmandeepSinghCS