[AIRFLOW-3997] Add getter to Variable that returns None instead of throwing #4819

kristiany · 2019-03-02T11:36:24Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-3997
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
- In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal(AIP).

Description

Here are some details about my PR, including screenshots of any UI changes:

Fix default so None works as default value for the get method.

This will not change existing regular functions in the Variable class (unless the redundant default_var=None is used). If
variable foo doesn't exist:

foo = Variable.get("foo")
    -> KeyError

For passing default_var=None to get, None is returned instead:

foo = Variable.get("foo", default_var=None)
if foo is None:
    handle_missing_foo()

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Test None as default, and throw when no default is provided
Test the current behaviour for missing variables for the get function to raise a KeyError
Use None as default value in the setdefault method

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
- All the public functions and the classes in the PR contain docstrings that explain what it does

Describe the default_var parameter with a None value in the doc, in the get variable examples section. Also describe the current behaviour for get raising an error when a variable is missing.

Code Quality

Passes flake8

RosterIn · 2019-03-03T09:20:00Z

I think get_soft() or try_get() might be a better name than optional()
The try_get() idea is taken from the CAST function of SQL:
https://database.guide/cast-vs-try_cast-in-sql-server-whats-the-difference/
which has very similar functionality to what this PR is about.

kristiany · 2019-03-03T11:13:41Z

@RosterIn Agree. try_get is a better name 👍

EDIT:
Changed function name from optional to try_get and also updated the doc change in this PR, as well as the PR body.

OmerJog · 2019-03-05T07:25:47Z

You have an error in your code:

3) ERROR: test_get_non_existing_var_optionally_should_return_none (tests.CoreTest)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/core.py line 725 in test_get_non_existing_var_optionally_should_return_none
      self.assertIsNone(Variable.try_get("thisIdDoesNotExist", default_var=''))
    airflow/utils/db.py line 73 in wrapper
      return func(*args, **kwargs)
   TypeError: try_get() got an unexpected keyword argument 'default_var'

kristiany · 2019-03-05T18:55:55Z

@OmerJog Sorry, fixed now.

Struggling a bit with running the tests locally...

ashb · 2019-03-05T21:12:15Z

airflow/models/__init__.py

+        try:
+            return cls.get(key, deserialize_json=deserialize_json, session=session)
+        except KeyError:
+            return None


I'd like to do this without an extra method. What I propose is:

NO_DEFAULT_SENTINEL = object() def get(cls, key, default_var=NO_DEFAULT_SENTINEL, deserialize_json=False, session=None): obj = session.query(cls).filter(cls.key == key).first() if obj is None: if default_var is not NO_DEFAULT_SENTINEL: return default_var ...

👍 I like this. However this will change the current behaviour when default_var is set to None explicitly, but maybe that is not something people do? 🤷‍♂️

I think it's probably a rare-enough case that it won't affect many people.

Still, we should add a note to UPDATING.md saying something along the lines of "if you want the old behaviour of raising when the variable is not found, ensure default_var is not passed"

ashb · 2019-03-05T21:14:37Z

Could you update your commit message subject to "[AIRFLOW-3997] Add getter to Variable that returns None instead of throwing" please?

kristiany · 2019-03-10T13:25:28Z

PR re-done and body edited with using the existing get method instead and using a value to indicate when no default parameter was provided, as suggested by @ashb

Can't find the UPDATING.md file for describing the change in bahaviour 🤔

OmerJog · 2019-03-10T13:30:47Z

@kristiany
https://github.com/apache/airflow/blob/master/UPDATING.md

…stead of throwing This will not change existing regular functions in the `Variable` class. If variable `foo` doesn't exist: ``` foo = Variable.get("foo") -> KeyError ``` For passing `default_var=None` to get, `None` is returned instead: ``` foo = Variable.get("foo", default_var=None) if foo is None: handle_missing_foo() ``` * Fix default so `None` works as default value for the get method * Test `None` as default, and throwing when no default is provided * Describe the default_var parameter with a `None` value in the doc, in the get variable examples section. Also describe the current behavior for `get` of raising an error when a variable is missing * Use `None` as default value in the `setdefault` method * Add description of the behaviour change in UPDATING.md

kristiany · 2019-03-10T14:08:09Z

Added a description to UPDATING.md as well. Thanks @OmerJog

codecov-io · 2019-03-10T14:50:46Z

Codecov Report

Merging #4819 into master will increase coverage by 0.26%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4819      +/-   ##
==========================================
+ Coverage    75.3%   75.56%   +0.26%     
==========================================
  Files         450      451       +1     
  Lines       29023    29099      +76     
==========================================
+ Hits        21855    21990     +135     
+ Misses       7168     7109      -59

Impacted Files	Coverage Δ
airflow/models/__init__.py	`92.97% <0%> (+0.06%)`	⬆️
airflow/models/connection.py	`64.07% <0%> (-1.18%)`	⬇️
airflow/www/views.py	`76.35% <0%> (ø)`	⬆️
airflow/contrib/hooks/grpc_hook.py	`91.8% <0%> (ø)`
airflow/sensors/base_sensor_operator.py	`98.18% <0%> (+0.22%)`	⬆️
airflow/contrib/operators/aws_athena_operator.py	`70.45% <0%> (+0.68%)`	⬆️
airflow/contrib/operators/ssh_operator.py	`83.54% <0%> (+1.26%)`	⬆️
airflow/hooks/dbapi_hook.py	`87.9% <0%> (+8.87%)`	⬆️
airflow/utils/db.py	`90.38% <0%> (+51.92%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e220ac5...d4fb542. Read the comment docs.

… found (#4819) This will not change existing regular functions in the `Variable` class. If variable `foo` doesn't exist: ``` foo = Variable.get("foo") -> KeyError ``` For passing `default_var=None` to get, `None` is returned instead: ``` foo = Variable.get("foo", default_var=None) if foo is None: handle_missing_foo() ```

… found (apache#4819) This will not change existing regular functions in the `Variable` class. If variable `foo` doesn't exist: ``` foo = Variable.get("foo") -> KeyError ``` For passing `default_var=None` to get, `None` is returned instead: ``` foo = Variable.get("foo", default_var=None) if foo is None: handle_missing_foo() ```

kristiany force-pushed the optional-variable-get-accessor branch from 4e2f000 to 871f599 Compare March 3, 2019 11:22

kristiany force-pushed the optional-variable-get-accessor branch from 871f599 to 3050f44 Compare March 5, 2019 18:54

ashb reviewed Mar 5, 2019

View reviewed changes

ashb changed the title ~~[AIRFLOW-3997] Add accessor for optional variables~~ [AIRFLOW-3997] Add getter to Variable that returns None instead of throwing Mar 5, 2019

kristiany force-pushed the optional-variable-get-accessor branch 3 times, most recently from 5c61090 to 5879bc7 Compare March 10, 2019 13:18

kristiany force-pushed the optional-variable-get-accessor branch from 5879bc7 to a57297c Compare March 10, 2019 14:07

Fokko approved these changes Mar 10, 2019

View reviewed changes

Update UPDATING.md

d4fb542

ashb merged commit 781a82f into apache:master Mar 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-3997] Add getter to Variable that returns None instead of throwing #4819

[AIRFLOW-3997] Add getter to Variable that returns None instead of throwing #4819

kristiany commented Mar 2, 2019 •

edited

Loading

RosterIn commented Mar 3, 2019

kristiany commented Mar 3, 2019 •

edited

Loading

OmerJog commented Mar 5, 2019

kristiany commented Mar 5, 2019 •

edited

Loading

ashb Mar 5, 2019 •

edited

Loading

kristiany Mar 7, 2019

ashb Mar 7, 2019

ashb commented Mar 5, 2019

kristiany commented Mar 10, 2019

OmerJog commented Mar 10, 2019

kristiany commented Mar 10, 2019

codecov-io commented Mar 10, 2019 •

edited

Loading

[AIRFLOW-3997] Add getter to Variable that returns None instead of throwing #4819

[AIRFLOW-3997] Add getter to Variable that returns None instead of throwing #4819

Conversation

kristiany commented Mar 2, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

RosterIn commented Mar 3, 2019

kristiany commented Mar 3, 2019 • edited Loading

OmerJog commented Mar 5, 2019

kristiany commented Mar 5, 2019 • edited Loading

ashb Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

kristiany Mar 7, 2019

Choose a reason for hiding this comment

ashb Mar 7, 2019

Choose a reason for hiding this comment

ashb commented Mar 5, 2019

kristiany commented Mar 10, 2019

OmerJog commented Mar 10, 2019

kristiany commented Mar 10, 2019

codecov-io commented Mar 10, 2019 • edited Loading

Codecov Report

kristiany commented Mar 2, 2019 •

edited

Loading

kristiany commented Mar 3, 2019 •

edited

Loading

kristiany commented Mar 5, 2019 •

edited

Loading

ashb Mar 5, 2019 •

edited

Loading

codecov-io commented Mar 10, 2019 •

edited

Loading