Fix: ensure bool(LazySelectSequence) returns True or False

[johncmerfeld]

closes: #43977

---

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

[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?

[Lee-W]

After a second, though, shouldn't we just handle it in check_query_exists? That function states it should return bool