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

Fix bug in prune_dict where empty dict and list would be removed even in strict mode #32573

Merged
merged 9 commits into from
Jul 19, 2023

Conversation

vandonr-amz
Copy link
Contributor

@vandonr-amz vandonr-amz commented Jul 12, 2023

Strict mode is supposed to remove only None values, but this was not applied to lists and dict.

also prepare deprecation for the provider-specific method in amazon package doing the same thing.

As explained in code comments, we cannot replace it right now because of the bug fixed here, making behavior different.

@ferruzzi
Copy link
Contributor

Interesting, I wonder if those failing Lambda tests were written wrong or if these two functions are somehow not actually functionally identical.

airflow/providers/amazon/aws/utils/__init__.py Outdated Show resolved Hide resolved
@vandonr-amz
Copy link
Contributor Author

Interesting, I wonder if those failing Lambda tests were written wrong or if these two functions are somehow not actually functionally identical.

Yeah same, I'll take a look

@vandonr-amz
Copy link
Contributor Author

Interesting, I wonder if those failing Lambda tests were written wrong or if these two functions are somehow not actually functionally identical.

Yeah same, I'll take a look

turns out they are not equivalent because of

if new_val:
new_dict[k] = new_val

Which removes empty lists and dictionaries even if mode=strict, despite the doc saying it'll only remove None values in that case :/

@vandonr-amz
Copy link
Contributor Author

I think we have 2 options,

  1. "fix" that code, but this changes the behavior for every usage, not sure of the consequences
  2. close this PR and keep our own helper function

@ferruzzi
Copy link
Contributor

why_not_both.gif

I'd say the code should be fixed to match the docs in that case, but also I get that that isn't what you signed up for here so.... I guess drop this for now and maybe create an Issue pointing out the discrepancy in prune_dict()?

@eladkal
Copy link
Contributor

eladkal commented Jul 14, 2023

I think we have 2 options,

  1. "fix" that code, but this changes the behavior for every usage, not sure of the consequences
  2. close this PR and keep our own helper function

You can fix the core helper function.
Change the provider to import the new core helper if airflow >=2.7.0 and fall back to local helper otherwise.
Once providers minimum version is 2.7.0 we can get rid of the local helper (assuming its also deprecated as well)

@vandonr-amz vandonr-amz changed the title remove provider specific helper in favor of equivalent one in core Fix bug in prune_dict where dict and list would be removed even in strict mode Jul 14, 2023
@vandonr-amz vandonr-amz changed the title Fix bug in prune_dict where dict and list would be removed even in strict mode Fix bug in prune_dict where empty dict and list would be removed even in strict mode Jul 14, 2023
@vandonr-amz
Copy link
Contributor Author

@eladkal you requested changes earlier, I believe they were addressed, can you re-review ?

@vincbeck vincbeck merged commit bbd2902 into apache:main Jul 19, 2023
1 check passed
@vandonr-amz vandonr-amz deleted the vandonr/cleaning branch July 19, 2023 20:43
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Aug 2, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers area:secrets provider:amazon-aws AWS/Amazon - related issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants