Fix: ensure bool(check_query_exists) returns True or False

[johncmerfeld]

closes: #43977

---

It is possible for bool(check_query_exists) to return None. This can break Xcom Jinja templates that retrieve null values from the airflow database using a LazySelectSequence

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

• Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
• Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: [email protected]
  Slack: https://s.apache.org/airflow-slack

[potiuk]

Could you please add a unit test covering this case?

[johncmerfeld]

Could you please add a unit test covering this case?

I'd love to; I'm just a little unsure where it would go. I couldn't find any module that tests the behavior of LazySelectSequence directly. I'd appreciate any advice you have on this!

[potiuk]

Could you please add a unit test covering this case?

I'd love to; I'm just a little unsure where it would go. I couldn't find any module that tests the behavior of LazySelectSequence directly. I'd appreciate any advice you have on this!

Maybe add one then - following the same structure in tests as you have for the source

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 suggestion.

[johncmerfeld]

Maybe add one then - following the same structure in tests as you have for the source

Sure. Since it's an abstract class, I'm thinking I'll have to create a mock implementation for the test. I'll also need to mock check_query_exists so that it returns None .Something like:

class LazySelectSequenceInstance(LazySelectSequence[Any]):
    ... # implement required methods

@mock.patch("check_query_exists", lambda x, y: None):
def test_lazy_select_sequence():
    lss = LazySelectSequenceInstance(...)
    assert bool(lss) == False

Does this make sense?

[Lee-W]

Maybe add one then - following the same structure in tests as you have for the source

Sure. Since it's an abstract class, I'm thinking I'll have to create a mock implementation for the test. I'll also need to mock check_query_exists so that it returns None .Something like:

class LazySelectSequenceInstance(LazySelectSequence[Any]):
    ... # implement required methods

@mock.patch("check_query_exists", lambda x, y: None):
def test_lazy_select_sequence():
    lss = LazySelectSequenceInstance(...)
    assert bool(lss) == False

Does this make sense?

Sounds reasonable. We already have a mock class here

airflow/tests/serialization/test_serialized_objects.py

Line 170 in f5abe50

class MockLazySelectSequence(LazySelectSequence):

probably we could do something similar here as well?

[johncmerfeld]

@Lee-W I'm struggling to get unit tests running locally with virtualenv but all the CI checks appear to be passing.

EDIT: Ahh okay I see the tests don't actually run until there is an approval. Alas I'm unable to run the tests when building locally on MacOS or ubuntu. Not sure I have the hardware needed to run the docker version of the tests

[Lee-W]

@Lee-W I'm struggling to get unit tests running locally with virtualenv but all the CI checks appear to be passing.

EDIT: Ahh okay I see the tests don't actually run until there is an approval. Alas I'm unable to run the tests when building locally on MacOS or ubuntu. Not sure I have the hardware needed to run the docker version of the tests

for local test https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst is the thing you might want to try 🙂

[uranusjr]

This doesn’t test anything… I believe it passes even without the change.

[johncmerfeld]

You're right. I've fixed the test and it is now meaningful. Without the bool() cast in check_query_exists the test fails

[Lee-W]

Will this fail in the original case?

assert not bool(None)

seems to work fine

[johncmerfeld]

Correct, bool(None) returns False. However, the problem is that prior to this change, bool(lss) returns None, which results in TypeError: __bool__ should return bool, returned NoneType, which I'm seeing in production.

[johncmerfeld]

So really this is a case where either the assertion is true or an exception is raised. Perhaps there is a more explicit way to test this, or I could add a comment. Whatever you think is best!

[Lee-W]

Ah, got it

I think assert bool(lss) is False would probably be better🤔

[johncmerfeld]

Yeah I think you're right. It's much clearer