fix: handle null LiteralMap in RemoteLiteralMapViewer

[kanterov]

TL;DR

Handle null LiteralMap in RemoteLiteralMapViewer

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

Not sure I completely understand how proto objects translate to JS objects. There are cases when fullInputs and fullOutputs are equal to null. Pull request adds a null check that complements the existing undefined check.

[schottra]

If you're handling both cases, a coercing equality against null will do it and is a common pattern in this repository.
if (map != null) { ... }

Generally when checking protobuf fields which are not primitive values, I use this form of check.

[schottra]

@kanterov
Sorry you got caught by this. You're correct that it's a quirk of the way protobuf objects are deserialized. You can look into the details in the flyteidl generated JS code. But that file is huge and github can't render it, so here's the relevant code for the response object that results in this null value:

admin.WorkflowExecutionGetDataResponse = (function() {

            /**
             * Properties of a WorkflowExecutionGetDataResponse.
             * @memberof flyteidl.admin
             * @interface IWorkflowExecutionGetDataResponse
             * @property {flyteidl.admin.IUrlBlob|null} [outputs] WorkflowExecutionGetDataResponse outputs
             * @property {flyteidl.admin.IUrlBlob|null} [inputs] WorkflowExecutionGetDataResponse inputs
             * @property {flyteidl.core.ILiteralMap|null} [fullInputs] WorkflowExecutionGetDataResponse fullInputs
             * @property {flyteidl.core.ILiteralMap|null} [fullOutputs] WorkflowExecutionGetDataResponse fullOutputs
             */

            /**
             * Constructs a new WorkflowExecutionGetDataResponse.
             * @memberof flyteidl.admin
             * @classdesc Represents a WorkflowExecutionGetDataResponse.
             * @implements IWorkflowExecutionGetDataResponse
             * @constructor
             * @param {flyteidl.admin.IWorkflowExecutionGetDataResponse=} [properties] Properties to set
             */
            function WorkflowExecutionGetDataResponse(properties) {
                if (properties)
                    for (let keys = Object.keys(properties), i = 0; i < keys.length; ++i)
                        if (properties[keys[i]] != null)
                            this[keys[i]] = properties[keys[i]];
            }

            /**
             * WorkflowExecutionGetDataResponse outputs.
             * @member {flyteidl.admin.IUrlBlob|null|undefined} outputs
             * @memberof flyteidl.admin.WorkflowExecutionGetDataResponse
             * @instance
             */
            WorkflowExecutionGetDataResponse.prototype.outputs = null;

            /**
             * WorkflowExecutionGetDataResponse inputs.
             * @member {flyteidl.admin.IUrlBlob|null|undefined} inputs
             * @memberof flyteidl.admin.WorkflowExecutionGetDataResponse
             * @instance
             */
            WorkflowExecutionGetDataResponse.prototype.inputs = null;

            /**
             * WorkflowExecutionGetDataResponse fullInputs.
             * @member {flyteidl.core.ILiteralMap|null|undefined} fullInputs
             * @memberof flyteidl.admin.WorkflowExecutionGetDataResponse
             * @instance
             */
            WorkflowExecutionGetDataResponse.prototype.fullInputs = null;
/* ... */

The generated code to debug protobuf messages uses prototype inheritance to handle values which are absent in the protobuf message. It follows specific conventions for each data type, some of which cause problems if they aren't handled correctly.
For instance, booleans default to false (not undefined) if they weren't set.
Nested objects will default to null instead of undefined.
Integers and enums will default to 0.

I've debated potentially writing a transform layer that makes the models a little more reliable and less error-prone.

[schottra]

@kanterov Looks like there is a type issue with one of the storybook stories due to updating ExecutionData to have non-optional properties. I'm happy to merge the PR once that is fixed!

[codecov-io]

Codecov Report

Merging #117 (4ef0ead) into master (12c486f) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #117   +/-   ##
=======================================
  Coverage   70.60%   70.60%           
=======================================
  Files         387      387           
  Lines        6531     6531           
  Branches     1048     1048           
=======================================
  Hits         4611     4611           
  Misses       1920     1920           

Impacted Files	Coverage Δ
src/models/Execution/api.ts	95.00% <ø> (ø)
src/components/Literals/RemoteLiteralMapViewer.tsx	94.44% <100.00%> (ø)

---

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 d6a35b3...4ef0ead. Read the comment docs.

[kanterov]

@schottra thanks for your help, I fixed it, didn't have enough time yesterday :)

[service-github-lyft-semantic-release]

🎉 This PR is included in version 0.17.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀