feat: Pre-emptively decode binary literals (#889)

* Revert "feat: Launch Form Msgpack IDL Supporting (#888)"

This reverts commit d1dc889.

* Pre-emptively decode binary literals

Signed-off-by: Carina Ursu <[email protected]>

* lint fixes

Signed-off-by: Carina Ursu <[email protected]>

* fix literal.helpers.test.ts

Signed-off-by: Carina Ursu <[email protected]>

---------

Signed-off-by: Carina Ursu <[email protected]>

package.json

@@ -56,7 +56,6 @@
   "dependencies": {
     "@commitlint/cli": "^17.3.0",
     "@commitlint/config-conventional": "^17.3.0",
-    "@msgpack/msgpack": "^3.0.0-beta2",
     "@semantic-release/changelog": "^5.0.1",
     "@semantic-release/commit-analyzer": "^8.0.1",
     "@semantic-release/exec": "^6.0.3",

packages/common/package.json

@@ -24,7 +24,7 @@
     "tslib": "^2.4.1"
   },
   "dependencies": {
-    "@flyteorg/flyteidl": "^1.10.7",
+    "@flyteorg/flyteidl": "^1.13.5",
     "@protobuf-ts/runtime": "^2.6.0",
     "@protobuf-ts/runtime-rpc": "^2.6.0"
   },

packages/common/src/flyteidl/google.ts

@@ -0,0 +1,5 @@
+import { google } from '@flyteorg/flyteidl/gen/pb-js/flyteidl';
+
+/** Message classes for google entities */
+
+export default google;

packages/oss-console/package.json

@@ -79,6 +79,7 @@
     "lossless-json": "^1.0.3",
     "memoize-one": "^5.0.0",
     "moment-timezone": "^0.5.28",
+    "msgpackr": "^1.11.2",
     "msw": "^1.3.2",
     "notistack": "^3.0.1",
     "object-hash": "^1.3.1",

packages/oss-console/src/App/index.tsx

@@ -1,4 +1,5 @@
 import '../common/setupProtobuf';
+import '../common/decodeMsgPackLiterals';
 import React, { PropsWithChildren } from 'react';
 import Collapse from '@mui/material/Collapse';
 import { FlyteApiProvider } from '@clients/flyte-api/ApiProvider';

packages/oss-console/src/common/decodeMsgPackLiterals.ts

@@ -0,0 +1,64 @@
+import core from '@clients/common/flyteidl/core';
+import google from '@clients/common/flyteidl/google';
+import $protobuf from 'protobufjs';
+import { unpack } from 'msgpackr';
+import isNil from 'lodash/isNil';
+import entries from 'lodash/entries';
+
+// Convert a JavaScript object to Protobuf Struct
+function convertToStruct(obj: any) {
+  const struct = new google.protobuf.Struct({
+    fields: {},
+  });
+
+  entries(obj).forEach(([key, value]) => {
+    struct.fields[key] = convertToValue(value);
+  });
+
+  return struct;
+}
+
+// Convert values to Protobuf Value type
+function convertToValue(value: any) {
+  const protoValue = new google.protobuf.Value();
+
+  if (Array.isArray(value)) {
+    const listValues = value.map(convertToValue);
+    protoValue.listValue = new google.protobuf.ListValue({ values: listValues });
+  } else if (typeof value === 'object' && value !== null) {
+    protoValue.structValue = convertToStruct(value);
+  } else if (typeof value === 'string') {
+    protoValue.stringValue = value;
+  } else if (typeof value === 'number') {
+    protoValue.numberValue = value;
+  } else if (typeof value === 'boolean') {
+    protoValue.boolValue = value;
+  } else if (value === null) {
+    protoValue.nullValue = google.protobuf.NullValue.NULL_VALUE;
+  }
+
+  return protoValue;
+}
+
+const originalLiteralDecode = core.Literal.decode;
+// Overriding the decode method of Literal to convert msgpack binary literals to protobuf structs
+core.Literal.decode = (reader: $protobuf.Reader | Uint8Array, length?: number) => {
+  const result = originalLiteralDecode(reader, length);
+
+  if (result?.scalar?.binary?.tag === 'msgpack') {
+    // We know that a binary literal with tag 'msgpack' is a STRUCT
+    const value = result?.scalar?.binary?.value;
+    const msgpackResult = isNil(value) ? value : unpack(value);
+    // Convert the msgpack result to a protobuf struct
+    const protobufStruct = convertToStruct(msgpackResult);
+
+    return {
+      metadata: result.metadata,
+      hash: result.hash,
+      scalar: {
+        generic: protobufStruct,
+      },
+    } as core.Literal;
+  }
+  return result;
+};

packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionLabels.tsx

@@ -2,7 +2,7 @@ import React from 'react';
 import Chip from '@mui/material/Chip';
 import makeStyles from '@mui/styles/makeStyles';
 
-type ValuesType = {[p: string]: string};
+type ValuesType = { [p: string]: string };
 interface Props {
   values: ValuesType;
 }
@@ -12,21 +12,20 @@ const useStyles = makeStyles({
     display: 'flex',
     flexWrap: 'wrap',
     width: '100%',
-    maxWidth: '420px'
+    maxWidth: '420px',
   },
   chip: {
     margin: '2px 2px 2px 0',
   },
 });
 
-
-export const ExecutionLabels: React.FC<Props> = ({values}) => {
+export const ExecutionLabels: React.FC<Props> = ({ values }) => {
   const classes = useStyles();
   return (
     <div className={classes.chipContainer}>
       {Object.entries(values).map(([key, value]) => (
         <Chip
-          color='info'
+          color="info"
           key={key}
           label={value ? `${key}: ${value}` : key}
           className={classes.chip}

packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionMetadata.tsx

@@ -71,11 +71,7 @@ export const ExecutionMetadata: React.FC<{}> = () => {
   const workflowId = execution?.closure?.workflowId;
 
   const { labels } = execution.spec;
-  const {
-    referenceExecution,
-    systemMetadata ,
-    parentNodeExecution,
-  } = execution.spec.metadata;
+  const { referenceExecution, systemMetadata, parentNodeExecution } = execution.spec.metadata;
   const cluster = systemMetadata?.executionCluster ?? dashedValueString;
 
   const details: DetailItem[] = [
@@ -130,11 +126,13 @@ export const ExecutionMetadata: React.FC<{}> = () => {
   if (labels != null && labels.values != null) {
     details.push({
       label: ExecutionMetadataLabels.labels,
-      value: (
-        Object.entries(labels.values).length > 0 ? 
-          <ExecutionLabels values={labels.values} /> : dashedValueString
-      )
-    })
+      value:
+        Object.entries(labels.values).length > 0 ? (
+          <ExecutionLabels values={labels.values} />
+        ) : (
+          dashedValueString
+        ),
+    });
   }
 
   return (

packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionNodeURL.tsx

@@ -34,14 +34,14 @@ export const ExecutionNodeURL: React.FC<{
 
   const config =
     // eslint-disable-next-line no-nested-ternary
-    env.CODE_SNIPPET_USE_AUTO_CONFIG === "true"
+    env.CODE_SNIPPET_USE_AUTO_CONFIG === 'true'
       ? 'Config.auto()'
       : isHttps
       ? // https snippet
         `Config.for_endpoint("${window.location.host}")`
       : // http snippet
         `Config.for_endpoint("${window.location.host}", True)`;
-    const code = `from flytekit.remote.remote import FlyteRemote
+  const code = `from flytekit.remote.remote import FlyteRemote
 from flytekit.configuration import Config
 remote = FlyteRemote(
     ${config},

packages/oss-console/src/components/Executions/ExecutionDetails/test/ExecutionLabels.test.tsx

@@ -4,27 +4,31 @@ import '@testing-library/jest-dom';
 import { ExecutionLabels } from '../ExecutionLabels';
 
 jest.mock('@mui/material/Chip', () => (props: any) => (
-  <div data-testid="chip" {...props}>{props.label}</div>
+  <div data-testid="chip" {...props}>
+    {props.label}
+  </div>
 ));
 
 describe('ExecutionLabels', () => {
   it('renders chips with key-value pairs correctly', () => {
     const values = {
       'random/uuid': 'f8b9ff18-4811-4bcc-aefd-4f4ec4de469d',
-      'bar': 'baz',
-      'foo': '',
+      bar: 'baz',
+      foo: '',
     };
 
     render(<ExecutionLabels values={values} />);
 
-    expect(screen.getByText('random/uuid: f8b9ff18-4811-4bcc-aefd-4f4ec4de469d')).toBeInTheDocument();
+    expect(
+      screen.getByText('random/uuid: f8b9ff18-4811-4bcc-aefd-4f4ec4de469d'),
+    ).toBeInTheDocument();
     expect(screen.getByText('bar: baz')).toBeInTheDocument();
     expect(screen.getByText('foo')).toBeInTheDocument();
   });
 
   it('applies correct styles to chip container', () => {
     const values = {
-      'key': 'value',
+      key: 'value',
     };
 
     const { container } = render(<ExecutionLabels values={values} />);
@@ -38,9 +42,9 @@ describe('ExecutionLabels', () => {
 
   it('renders correct number of chips', () => {
     const values = {
-      'key1': 'value1',
-      'key2': 'value2',
-      'key3': 'value3',
+      key1: 'value1',
+      key2: 'value2',
+      key3: 'value3',
     };
 
     render(<ExecutionLabels values={values} />);

packages/oss-console/src/components/Executions/ExecutionDetails/test/ExecutionMetadata.test.tsx

@@ -15,7 +15,7 @@ const durationTestId = `metadata-${ExecutionMetadataLabels.duration}`;
 const interruptibleTestId = `metadata-${ExecutionMetadataLabels.interruptible}`;
 const overwriteCacheTestId = `metadata-${ExecutionMetadataLabels.overwriteCache}`;
 const relatedToTestId = `metadata-${ExecutionMetadataLabels.relatedTo}`;
-const parentNodeExecutionTestId = `metadata-${ExecutionMetadataLabels.parent}`
+const parentNodeExecutionTestId = `metadata-${ExecutionMetadataLabels.parent}`;
 const labelsTestId = `metadata-${ExecutionMetadataLabels.labels}`;
 
 jest.mock('../../../../models/Launch/api', () => ({
@@ -113,15 +113,15 @@ describe('ExecutionMetadata', () => {
   it('shows related to if metadata is available', () => {
     const { getByTestId } = renderMetadata();
     expect(getByTestId(relatedToTestId)).toHaveTextContent('name');
-  })
+  });
 
   it('shows parent execution if metadata is available', () => {
     const { getByTestId } = renderMetadata();
     expect(getByTestId(parentNodeExecutionTestId)).toHaveTextContent('name');
-  })
+  });
 
   it('shows labels if spec has them', () => {
     const { getByTestId } = renderMetadata();
-    expect(getByTestId(labelsTestId)).toHaveTextContent("key: value");
-  })
+    expect(getByTestId(labelsTestId)).toHaveTextContent('key: value');
+  });
 });

packages/oss-console/src/components/Launch/LaunchForm/LaunchFormComponents/StructInput.tsx

@@ -1,8 +1,7 @@
-import React, { FC, useCallback, useEffect, useMemo, useState } from 'react';
+import React, { FC, useCallback, useMemo, useState } from 'react';
 import { Form } from '@rjsf/mui';
 import validator from '@rjsf/validator-ajv8';
 import styled from '@mui/system/styled';
-import * as msgpack from '@msgpack/msgpack';
 import { InputProps } from '../types';
 import { protobufValueToPrimitive, PrimitiveType } from '../inputHelpers/struct';
 import { StyledCard } from './StyledCard';

packages/oss-console/src/components/Launch/LaunchForm/inputHelpers/struct.ts

@@ -1,14 +1,13 @@
 import Protobuf from '@clients/common/flyteidl/protobuf';
 import Core from '@clients/common/flyteidl/core';
-import * as msgpack from '@msgpack/msgpack';
 import { InputType, InputValue } from '../types';
 import { structPath } from './constants';
 import { ConverterInput, InputHelper, InputValidatorParams } from './types';
 import { extractLiteralWithCheck, formatParameterValues } from './utils';
 
 export type PrimitiveType = string | number | boolean | null | object;
 
-function asValueWithKind(value: Protobuf.IValue): Protobuf.Value {
+export function asValueWithKind(value: Protobuf.IValue): Protobuf.Value {
   return value instanceof Protobuf.Value ? value : Protobuf.Value.create(value);
 }
 
@@ -91,25 +90,9 @@ function objectToProtobufStruct(obj: Dictionary<any>): Protobuf.IStruct {
   return { fields };
 }
 
-function parseBinary(binary: Core.IBinary): string {
-  if (!binary.value) {
-    throw new Error('Binary value is empty');
-  }
-
-  if (binary.tag === 'msgpack') {
-    return JSON.stringify(msgpack.decode(binary.value));
-  }
-
-  // unsupported binary type, it might be temporary
-  return '';
-}
-
 function fromLiteral(literal: Core.ILiteral): InputValue {
-  if (literal.scalar?.binary) {
-    return parseBinary(literal.scalar.binary);
-  }
-
   const structValue = extractLiteralWithCheck<Protobuf.IStruct>(literal, structPath);
+
   const finalValue = formatParameterValues(InputType.Struct, protobufStructToObject(structValue));
   return finalValue;
 }