feat: Support configuring expression in Canvas form #272

igarashitm · 2023-11-02T00:57:41Z

Fixes: #270

Just a mockup for now, plumbing TBD

2023-11-01.20-51-47.mp4

igarashitm · 2023-11-08T21:07:54Z

Implementation is done including dialect parsing. I'll add tests and then should be ready to go.

2023-11-08.16-01-18.mp4

igarashitm · 2023-11-08T22:05:58Z

One known bug: the language xtokenize has an array property namespace, which breaks this expression editor ATM
we'd need to fix the catalog-schema converter for that
https://github.com/KaotoIO/kaoto-next/pull/272/files#diff-2d877e44faedc2698396d57993c8d9d9a404d30029b306414175166ecbd0f5e4R64

Issue: #311

Fixes: KaotoIO#270

lordrip

This is great @igarashitm, a huge step forward 💪 🙌

lordrip · 2023-11-10T09:47:38Z

packages/ui/src/components/Visualization/Canvas/CanvasForm.tsx

  return schema?.schema === undefined ? null : (
    <ErrorBoundary fallback={<p>This node cannot be configured yet</p>}>
      <Title headingLevel="h1">{componentName}</Title>
-
+      {isExpressionAwareStep && <ExpressionEditor selectedNode={props.selectedNode} />}


[curious-question]

Couldn't this be added in the CustomAutoField component instead?

Could be possible for this expression property to collide with another one from another resource?

If the latter is true, would be beneficial to have the isExpressionAwareStep logic inside of the camel-route-visual-entity?

Since this is done outside of the uniforms, CustomAutoField doesn't participate

Can you ellaborate?

I'd like to wait a bit to see how Integration and Kamelet kind is implemented, as this logic will be used for those as well

Could it be incorporated into the CustomAutoField workflow? This is not a blocking issue in any case

I meant what would happen if a Pipe for instance has an expression property? I guess we're gonna see the expression field anyway.

That sounds fair 👍

lordrip · 2023-11-10T09:48:14Z

packages/ui/src/components/MetadataEditor/PipeErrorHandlerEditor.test.tsx

@@ -1,7 +1,7 @@
 import { PipeErrorHandlerEditor } from './PipeErrorHandlerEditor';
 import { within } from '@testing-library/dom';
 import { fireEvent, render, screen } from '@testing-library/react';
-import { pipeErrorHandlerJson } from '../../stubs/PipeErrorHandler';
+import * as pipeErrorHandlerSchema from '@kaoto-next/camel-catalog/PipeErrorHandler.json';


Great this is finally working 🎉

lordrip · 2023-11-10T09:49:33Z

packages/ui/src/components/Visualization/Canvas/ExpressionEditor.test.tsx

+      useSchemasStore.setState({
+        schemas: {
+          camelYamlDsl: {
+            name: 'camelYamlDsl',
+            tags: ['camel'],
+            version: '1.0.0',
+            uri: '',
+            schema: yamlDslSchema as unknown as Record<string, unknown>,
+          },
+        },
+      });


[heads-up]: It's possible that in a future iteration, the useSchemasStore will be removed in favor of a react provider, to handle schemas easier in consumers, like the VSCode extension.

lordrip · 2023-11-10T10:02:01Z

packages/ui/src/components/Visualization/Canvas/expression.service.ts

+  /**
+   * Get the language catalog map from the Camel catalog. Since "language" language is not in the languages Camel
+   *  catalog, it is extracted from the YAML DSL schema.
+   * @param yamlDslSchema
+   */
+  static getLanguageMap(yamlDslSchema: Record<string, unknown>): Record<string, ICamelLanguageDefinition> {
+    const catalog = { ...CamelCatalogService.getLanguageMap() };
+
+    // "language" for custom language is not in the Camel catalog. Create from YAML DSL schema and Add here.
+    const customDefinition = ExpressionService.createCustomLanguageDefinition(yamlDslSchema);
+    if (customDefinition) {
+      catalog['language'] = customDefinition;
+    }
+
+    // "file" language is merged with "simple" language and it doesn't exist in YAML DSL schema. Remove it here.
+    delete catalog['file'];
+
+    return catalog;
+  }
+
+  private static createCustomLanguageDefinition(yamlDslSchema: Record<string, unknown>) {
+    const definitions = (yamlDslSchema.items as Record<string, unknown>)?.definitions as Record<string, unknown>;
+    const customLanguageSchema =
+      definitions && (definitions['org.apache.camel.model.language.LanguageExpression'] as Record<string, unknown>);
+    if (!customLanguageSchema) return;
+
+    const properties = Object.entries(customLanguageSchema.properties as Record<string, unknown>).reduce(
+      (acc, [key, value]) => {
+        acc[key] = {
+          index: Object.keys(acc).length,
+          required: (customLanguageSchema.required as string[]).includes(key),
+          ...(value as Record<string, unknown>),
+        } as ICamelLanguageProperty;
+        return acc;
+      },
+      {} as Record<string, ICamelLanguageProperty>,
+    );
+    return {
+      language: {
+        kind: CatalogKind.Language,
+        name: 'language',
+        title: customLanguageSchema.title,
+        description: customLanguageSchema.description,
+        deprecated: false,
+        modelName: 'language',
+      } as ICamelLanguageModel,
+      properties: properties,
+    } as ICamelLanguageDefinition;
+  }
+


[curious-question]: Does this process mutate over time? If not, wouldn't be preferable to run it just once at boot time instead? Perhaps around when we build the catalogs.

Sounds like a plan, filed an issue - #316

lordrip · 2023-11-10T10:02:54Z

packages/ui/src/components/Visualization/Canvas/expression.service.ts

+  /**
+   * Set the expression model to the parent step model object. This method uses the most verbose dialect, i.e.
+   * ```yaml
+   * - setBody:
+   *     expression:
+   *       simple:
+   *         expression: ${body}
+   *         trim: true
+   * ```


This is great, thanks for providing the example

mmm, could be possible to point also in the example, what the params represent based on the YAML example?

Do you mean we want an explanation for e.g. trim parameter in this comment?

Ah, no, I meant the function parameters.

@param parentModel @param languageModelName @param newExpressionModel

For instance, are those parameters related to the YAML example? do they correspond to a particular field? if that's the case, can we point out which ones?

ahh OK, that makes sense, I'll follow up

lordrip · 2023-11-10T10:24:58Z

I would like to know your opinion about the following image:

Is it necessary for the expression field to be expandable? It feels a bit different than the rest of the form.
Given that we're offering the id field as well, should we also provide a default ID as well, as we do for other nodes?

lordrip · 2023-11-10T10:32:01Z

Another thing that I noticed, if you start with the default route

- route:
    from:
      uri: timer:template
      parameters:
        period: "1000"
      steps:
        - log:
            message: template message
    id: route-1073

notice how the log processor doesn't have an expression property defined, then you open the log processor to configure it, there's no expression field either

but returning to the source code, it looks like an expression property gets added 🤔

have you seen that in the past?

igarashitm · 2023-11-10T11:22:37Z

I would like to know your opinion about the following image:

Is it necessary for the expression field to be expandable? It feels a bit different than the rest of the form.

I would think so, since some of the language have many parameters and push step parameters out of the screen.
I agree that the styling for this expression editor looks different from the rest of the form, it's better to align at some point

Given that we're offering the id field as well, should we also provide a default ID as well, as we do for other nodes?

Filed an issue - #317

igarashitm · 2023-11-10T11:26:05Z

notice how the log processor doesn't have an expression property defined, then you open the log processor to configure it, there's no expression field either

but returning to the source code, it looks like an expression property gets added 🤔

have you seen that in the past?

Since log step doesn't have expression parameter, it shouldn't be added, can you file a bug issue and assign it to me?

lordrip · 2023-11-10T11:55:31Z

I filed an issue for the expression field that sometimes is added unintentionally (at least I think unintentionally): #319, it doesn't happen always and I haven't found a reliable to reproduce it.

lordrip · 2023-11-10T11:56:19Z

Considering that we have all the feedback in dedicated issues, I'm gonna merge this PR 🎉 , thanks once again Tomo

igarashitm force-pushed the 270 branch from c7239e2 to 24e61d6 Compare November 8, 2023 20:20

igarashitm mentioned this pull request Nov 8, 2023

Language xtokenize crashes #311

Closed

igarashitm force-pushed the 270 branch from 24e61d6 to 29a822c Compare November 10, 2023 03:14

igarashitm marked this pull request as ready for review November 10, 2023 03:15

igarashitm requested a review from a team November 10, 2023 03:15

feat: Support configuring expression in Canvas form

1d37514

Fixes: KaotoIO#270

igarashitm force-pushed the 270 branch from 29a822c to 1d37514 Compare November 10, 2023 03:23

lordrip approved these changes Nov 10, 2023

View reviewed changes

igarashitm mentioned this pull request Nov 10, 2023

Assign auto-generated ID for expression #317

Open

igarashitm mentioned this pull request Nov 10, 2023

Move custom language catalog into @kaoto-next/camel-catalog #316

Closed

lordrip mentioned this pull request Nov 10, 2023

[BUG] Steps sometimes receive an expression property when it's not applicable #319

Closed

lordrip merged commit 0711250 into KaotoIO:main Nov 10, 2023
10 checks passed

igarashitm mentioned this pull request Nov 10, 2023

chore: Add a bit more TSDoc comments #322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support configuring expression in Canvas form #272

feat: Support configuring expression in Canvas form #272

igarashitm commented Nov 2, 2023 •

edited

Loading

igarashitm commented Nov 8, 2023 •

edited

Loading

igarashitm commented Nov 8, 2023 •

edited

Loading

lordrip left a comment

lordrip Nov 10, 2023

igarashitm Nov 10, 2023

lordrip Nov 10, 2023

lordrip Nov 10, 2023

lordrip Nov 10, 2023

lordrip Nov 10, 2023

igarashitm Nov 10, 2023

lordrip Nov 10, 2023

lordrip Nov 10, 2023

igarashitm Nov 10, 2023

lordrip Nov 10, 2023

igarashitm Nov 10, 2023

lordrip commented Nov 10, 2023

lordrip commented Nov 10, 2023

igarashitm commented Nov 10, 2023 •

edited

Loading

igarashitm commented Nov 10, 2023

lordrip commented Nov 10, 2023

lordrip commented Nov 10, 2023

feat: Support configuring expression in Canvas form #272

feat: Support configuring expression in Canvas form #272

Conversation

igarashitm commented Nov 2, 2023 • edited Loading

igarashitm commented Nov 8, 2023 • edited Loading

igarashitm commented Nov 8, 2023 • edited Loading

lordrip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordrip commented Nov 10, 2023

lordrip commented Nov 10, 2023

igarashitm commented Nov 10, 2023 • edited Loading

igarashitm commented Nov 10, 2023

lordrip commented Nov 10, 2023

lordrip commented Nov 10, 2023

igarashitm commented Nov 2, 2023 •

edited

Loading

igarashitm commented Nov 8, 2023 •

edited

Loading

igarashitm commented Nov 8, 2023 •

edited

Loading

igarashitm commented Nov 10, 2023 •

edited

Loading