Fix: render non-null falsy XCom entries correctly in UI

[detvdl]

Fixed issues described in #42096, which were introduced in 563bc49

[pierrejeambrun]

Well funny enough I just left a comment here on a very related null/undefined check.

This is a mistake that is common in our codebase when we do ! and !! on falsy values unfortunately. We need to pay extra attention to that.

Also NaN is yet another case to handle (nullish coallescing will not handle that, we might need a common utility function).

Also do you mind explaining why we explicitely try to isNaN the value ? Coercion of isNaN is somewhat surprising, and I' m not sure what we are trying to actually check under the hood. Xcom values are bool, number, obj / null / undefined.

[detvdl]

Also do you mind explaining why we explicitely try to isNaN the value ? Coercion of isNaN is somewhat surprising, and I' m not sure what we are trying to actually check under the hood. Xcom values are bool, number, obj / null / undefined.

isNaN returns True for both undefined or NaN so it was mostly a shortcut to check for both

Perhaps more idiomatic would be to use the nullish coalescing here to more explicitly catch the first 2 cases (null and undefined) and explicitly call isNan on the right hand side?

if (xcom.value ?? Number.isNaN(xcom.value))

From a quick peek at the code I figured both cases could be present exceptionally but I may be wrong, have only looked at the UI rendering for a few minutes.

I pragmatically just tackled this specific issue as soon as I saw it, and got a PR out not to forget 🙂

[pierrejeambrun]

if (xcom.value ?? Number.isNaN(xcom.value))

In that case if we have a value we will enter the loop.

Maybe a simple:

xcom.value === undefined || xcom.value === null

We can always add the || Number.isNaN(xcom.value but I don't think we can actually have a NaN at this stage, but I'm not sure.

[detvdl]

if (xcom.value ?? Number.isNaN(xcom.value))

In that case if we have a value we will enter the loop.

Ah you're right, I hadn't stopped to think about that, good call!

I don't know about the NaN either, let me spend a little more time checking if it can happen and I will come back to this

After a quick peek, I can see that the XCom model value is serialized through SQLAlchemySchema's dump method or similar, which should never produce any value that is then further parsed back to NaN in the jsonParse method shown in 563bc49

There are ways to support NaN in those schemas for serialization (https://marshmallow.readthedocs.io/en/stable/upgrading.html#float-field-takes-a-new-allow-nan-parameter) but it's not the default, and nowhere in this codebase is it explicitly set (nor do I think Airflow uses a version of that dep which supports it)

So you're right @pierrejeambrun , just a plain undefined and null check should be fine and the most clear 🙂

[pierrejeambrun]

Great, thanks for the investigation @detvdl