Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(canvas): Validate nodes upon rendering #711

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

lordrip
Copy link
Member

@lordrip lordrip commented Jan 23, 2024

Context

Currently, we're validating the canvas' nodes when editing the Form, using the original model. This causes that in some cases, we get a false positive result since a parameter might have been defined using the URI field.

Changes

This commit aims to validate each node upon rendering, meaning that the node will be validated whenever the node is rendered, which happens the first time a route is presented or when the node is selected or deselected.

In addition to that, we're parsing the node definition so we can account for the URI field when applicable.

How to test

Using the following YAML, you should see that only the missing-parameters nodes are highlighted as warning

- route:
    id: SqlBeansRoute
    from:
      uri: timer
      steps:
        - to:
            uri: sql
        - to:
            uri: log:test
- beans:
    - name: mysqlDataSource
      type: io.kaoto.MysqlDataSource
      properties:
        port: "3306"
        url: jdbc:mysql://localhost/test
        username: dbuser
        password: dbpass
        driver-class-name: com.mysql.jdbc.Driver
    - name: mysqlPostgreSqlSource
      type: io.kaoto.MysqlPostgreSqlSource
      properties:
        port: "5432"
        url: jdbc:postgresql://localhost/test
        username: dbuser
        password: dbpass
        driver-class-name: org.postgresql.Driver
- from:
    uri: timer:timer-1?period=5000&delay=5&synchronous=true
    steps: []
    
# camel-k: language=yaml

# Write your routes here, for example:
- from:
    uri: "jms:cheese"
    steps:
      - log: "${body}"
      
- route:
    id: route-7827
    from:
      uri: kamelet:chuck-norris-source
      steps:
        - log: ${body}
        - to:
            description: azz
            uri: kamelet:kafka-not-secured-sink?topic=foobar&bootstrapServers=localhost
            parameters: {}          

fix: #697
fix: #674

@lordrip lordrip force-pushed the feat/validate-node-upon-rendering branch from 31991a8 to bba452a Compare January 23, 2024 13:47
@igarashitm
Copy link
Contributor

It seems this change disabled the validation update for form change event, it needs to close and reopen the node to update the validation status.

@lordrip
Copy link
Member Author

lordrip commented Jan 23, 2024

It seems this change disabled the validation update for form change event, it needs to close and reopen the node to update the validation status.

Yes, it's working like the description field.

@lordrip lordrip force-pushed the feat/validate-node-upon-rendering branch from bba452a to 0651e8c Compare January 23, 2024 13:57
@lordrip
Copy link
Member Author

lordrip commented Jan 23, 2024

@igarashitm can we use the validator for Pipes as well?

@igarashitm
Copy link
Contributor

The reason I added the listen-notify was to update the validation status as soon as user input the required field. I'd prefer that behavior.

@lhein
Copy link
Contributor

lhein commented Jan 23, 2024

One thing to consider: If we have updates while typing from source to graph, users would rightfully expect also the opposite direction to work. I am not a big fan of this tbh. For me it would be enough, if we update on Save.

@igarashitm
Copy link
Contributor

@lhein

One thing to consider: If we have updates while typing from source to graph, users would rightfully expect also the opposite direction to work. I am not a big fan of this tbh. For me it would be enough, if we update on Save.

Do you mean to wait updating the status until vscode save event is triggered, but not even when form is closed and re-opened? by typing from source to graph, do you mean typing in the config form, but not typing in the source code editor?

@igarashitm
Copy link
Contributor

igarashitm commented Jan 23, 2024

Another option is to introduce a refresh button somewhere, maybe start with clicking the node status icon, but I'd prefer more explicit one eventually. What I don't like is that, in order to update the node status, I need to close and reopen which is indirect actions for the node status update.

validate button in the config form?

Update

I quickly tried onStatusDecoratorClick() to update the status, but once validation passes, the status decorator disappear as it changes to 'default' which doesn't have a status icon. maybe better to hook on something else.

@lordrip
Copy link
Member Author

lordrip commented Jan 23, 2024

@lhein

One thing to consider: If we have updates while typing from source to graph, users would rightfully expect also the opposite direction to work. I am not a big fan of this tbh. For me it would be enough, if we update on Save.

Do you mean to wait updating the status until vscode save event is triggered, but not even when form is closed and re-opened? by typing from source to graph, do you mean typing in the config form, but not typing in the source code editor?

For clarification:

There are a few Update events:
a) Right after an edit happens in the Form (current behavior from main branch)
b) After the form is closed
c) When hitting Ctrl+S in VSCode (we could say this one is special since we don't have it in the online version)

@igarashitm
Copy link
Contributor

Also in the future, we will do inline validation in the config form itself I assume. The node status would be better in sync with the form validations. If we do the form validations with user action like a validate button, then node status icon would. IMHO We have to avoid the node status and config form validation being out of sync.

@lhein
Copy link
Contributor

lhein commented Jan 24, 2024

It seems I was not clear enough in my statement. What I meant is that when typing something in the source code (like the description parameter) I wouldn't expect the changes to be visible in the Canvas (in this case the label of the node would change). What I would expect is that once I save the Source changes, then the Canvas gets updated.

Below video hopefully shows in a better way what I mean. Here I change the description in the code and the node label changes while I type. If on the other hand I go to the canvas config form and change the description there, then it is not changing in the source while typing and also not after I am done and closing the form. It needs a SAVE to persist the changes to the source. This is not consistent and behaving the same in both directions and thats why I say we should only update canvas elements from source changes, once the source changes are saved.

sourceCanvasSync

This btw. also leads to not so nice behaviors / issues like the one below...

SyncOnTypeIssue

Currently, we're validating the canvas' nodes when editing the Form,
using the original model. This casuses that in some cases, we get a
false positive result since a parameter might have been defined using
the URI field.

This commit aims to validate each node upon rendering, meaning that the
node will be validated whenever the node is rendered, which happens the
first time when a route is presented or when the node is selected or
deselected.

In addition to that, we're parsing the node definition so we could
account for the URI field when applicable.

fix: KaotoIO#697
@lordrip
Copy link
Member Author

lordrip commented Jan 24, 2024

As discussed, we're moving forward with this PR behavior so we can close the TP1 release and create additional issues to address the individual concerns:

  1. Keep form and node validation status in sync
  2. Form changes should reflect on the canvas

@lordrip lordrip force-pushed the feat/validate-node-upon-rendering branch from 0651e8c to 19c3cda Compare January 24, 2024 14:47
@lordrip lordrip merged commit ead5f7b into KaotoIO:main Jan 24, 2024
10 checks passed
@lordrip lordrip deleted the feat/validate-node-upon-rendering branch January 24, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants