Skip to content

Remove unnecessary call to keys() method on dictionaries #34260

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.

Sign up for GitHub

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

Merged
uranusjr merged 3 commits into from
Sep 14, 2023
Merged

Remove unnecessary call to keys() method on dictionaries #34260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d29e89
Remove unnecessary call to keys() method on dictionaries
pankajkoti Sep 10, 2023
0473312
Merge branch 'main' into remove-unnecessary-keys-call
pankajkoti Sep 13, 2023
5fc664b
Apply @potiuk's suggestion for raising RuntimeError when body is None
pankajkoti Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion airflow/providers/google/cloud/hooks/bigquery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ def create_external_table(
],
"googleSheetsOptions": ["skipLeadingRows"],
}
if source_format in src_fmt_to_param_mapping.keys():
if source_format in src_fmt_to_param_mapping:
valid_configs = src_fmt_to_configs_mapping[src_fmt_to_param_mapping[source_format]]
src_fmt_configs = _validate_src_fmt_configs(
source_format, src_fmt_configs, valid_configs, backward_compatibility_configs
Expand Down
2 changes: 1 addition & 1 deletion airflow/providers/google/cloud/transfers/gcs_to_bigquery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ def _create_external_table(self):
],
"googleSheetsOptions": ["skipLeadingRows"],
}
if self.source_format in src_fmt_to_param_mapping.keys():
if self.source_format in src_fmt_to_param_mapping:
valid_configs = src_fmt_to_configs_mapping[src_fmt_to_param_mapping[self.source_format]]
self.src_fmt_configs = self._validate_src_fmt_configs(
self.source_format, self.src_fmt_configs, valid_configs, backward_compatibility_configs
Expand Down
6 changes: 4 additions & 2 deletions airflow/providers/google/cloud/utils/field_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ def _validate_dict(self, children_validation_specs: dict, full_field_path: str,
validation_spec=child_validation_spec, dictionary_to_validate=value, parent=full_field_path
)
all_dict_keys = {spec["name"] for spec in children_validation_specs}
for field_name in value.keys():
for field_name in value:
if field_name not in all_dict_keys:
self.log.warning(
"The field '%s' is in the body, but is not specified in the "
Expand Down Expand Up @@ -421,6 +421,8 @@ def validate(self, body_to_validate: dict) -> None:
:param body_to_validate: body that must follow the specification
:return: None
"""
if body_to_validate is None:

@uranusjr uranusjr Sep 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The function’s type annotation should probably be updated to cover the None case.

@pankajkoti pankajkoti Sep 13, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am slightly confused if it would make sense to update the type annotation to include None as an acceptable type only to raise an error. 🤔

@uranusjr uranusjr Sep 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah that’s a good point

raise RuntimeError("The body to validate is `None`. Please provide a dictionary to validate.")
try:
for validation_spec in self._validation_specs:
self._validate_field(validation_spec=validation_spec, dictionary_to_validate=body_to_validate)
Expand All @@ -441,7 +443,7 @@ def validate(self, body_to_validate: dict) -> None:
if nested_union_spec.get("type") != "union"
and nested_union_spec.get("api_version") != self._api_version
)
for field_name in body_to_validate.keys():
for field_name in body_to_validate:

@Taragolis Taragolis Sep 10, 2023

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.

Everything fine in refactoring itself, for me a bit confusing original test which was written years ago

airflow/tests/providers/google/cloud/utils/test_field_validator.py

Lines 37 to 44 in dd4804e

def test_validate_should_fail_if_body_is_none(self):
specification = []
body = None
validator = GcpBodyFieldValidator(specification, "v1")
with pytest.raises(AttributeError):
validator.validate(body)

I not familiar with such body in GCP, but I guess if we expect that only dict is valid here then we need to fix a bit of logic from raise TypeError in case if got not a dict, because it also might confuse end users if they got Attribute error

@hussein-awala hussein-awala Sep 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand that it fails because None is not iterable, but the question is how None.keys() didn't fail before 🤔

@potiuk potiuk Sep 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

>>> x = None
>>> x.keys()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'NoneType' object has no attribute 'keys'
>>>

@potiuk potiuk Sep 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you will never get None in reality - this was really a defensive test. And AttributeError was just side effect of using keys() on None.

I think the right solution is:

Add this:

if body_to_validate is None:
   raise RuntimeError("The body passed here should never be `None`)

and expect the RuntimeError in this test.

if field_name not in all_field_names:
self.log.warning(
"The field '%s' is in the body, but is not specified in the "
Expand Down
2 changes: 1 addition & 1 deletion airflow/ti_deps/deps/trigger_rule_dep.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ def _iter_upstream_conditions(relevant_tasks: dict) -> Iterator[ColumnOperators]
return
# Otherwise we need to figure out which map indexes are depended on
# for each upstream by the current task instance.
for upstream_id in relevant_tasks.keys():
for upstream_id in relevant_tasks:
map_indexes = _get_relevant_upstream_map_indexes(upstream_id)
if map_indexes is None: # All tis of this upstream are dependencies.
yield (TaskInstance.task_id == upstream_id)
Expand Down
2 changes: 1 addition & 1 deletion airflow/utils/process_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ def patch_environ(new_env_variables: dict[str, str]) -> Generator[None, None, No
After leaving the context, it restores its original state.
:param new_env_variables: Environment variables to set
"""
current_env_state = {key: os.environ.get(key) for key in new_env_variables.keys()}
current_env_state = {key: os.environ.get(key) for key in new_env_variables}
os.environ.update(new_env_variables)
try:
yield
Expand Down
4 changes: 3 additions & 1 deletion tests/providers/google/cloud/utils/test_field_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ def test_validate_should_fail_if_body_is_none(self):

validator = GcpBodyFieldValidator(specification, "v1")

with pytest.raises(AttributeError):
with pytest.raises(
RuntimeError, match="The body to validate is `None`. Please provide a dictionary to validate."
):
validator.validate(body)

def test_validate_should_fail_if_specification_is_none(self):
Expand Down