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

fix: Ensure strict typing for input defaults to prevent runtime errors #5604

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Malaydewangan09
Copy link
Contributor

What changes are being made and why?

This PR addresses the issue (#5590) where untyped input defaults could lead to runtime errors. By switching to a strongly typed generic parameter for inputs, we ensure that defaults are properly validated during schema generation, thereby reducing unexpected behaviors at execution time.

closes #5590

@yuri1969
Copy link
Contributor

@Malaydewangan09 Hi, here my QA feedback:

  1. It is still possible to save the invalid flow mentioned in the issue.
  2. The JSON Schema for the TIME input type changed to time. It no longer accepts a value, such as 10:11:12. It requires a time zone info, such as 10:11:12Z:
- id: time
  type: TIME
  # ERROR: 'String is not a RFC3339 time.'
  defaults: "10:11:12"
  required: false

@Malaydewangan09
Copy link
Contributor Author

Malaydewangan09 commented Oct 23, 2024

Hey @yuri1969
Thanks!
So when the validate API is triggered
We have to the default value checks outside the scope of Jackson mapper right ?
Because for case

id: myflow
namespace: company.team
inputs:
  - id: intDefaultsInStrInput
    type: STRING
    validator: '\d+'
    defaults: 100
    required: false

tasks:
  - id: date
    type: io.kestra.plugin.core.debug.Return
    format: "{{taskrun.startDate}}"

Jackson will not throw error ?

@Malaydewangan09
Copy link
Contributor Author

Hey @yuri1969 Thanks! So when the validate API is triggered, We have to do the default value checks outside the scope of Jackson Mapper, right ? Because for case

id: myflow
namespace: company.team
inputs:
  - id: intDefaultsInStrInput
    type: STRING
    validator: '\d+'
    defaults: 100
    required: false

tasks:
  - id: date
    type: io.kestra.plugin.core.debug.Return
    format: "{{taskrun.startDate}}"

Jackson will not throw an error??

Can we have a custom deserializer for the default value?

@yuri1969
Copy link
Contributor

@Malaydewangan09 I'm not really familiar with advanced Jackson usage, sorry 😐

@Malaydewangan09
Copy link
Contributor Author

Malaydewangan09 commented Oct 24, 2024

@MilosPaunovic @loicmathieu can you help me with this?
Thanks!

@loicmathieu
Copy link
Member

@Malaydewangan09 I need to take some time to investigate.

@yuri1969 as I understand it, we previously accept non-standard LocalTime string representation, and with the change it would not work anymore. I need to dig but their may not be any way to fix that. But I'm worry about the breaking change here.

@yuri1969
Copy link
Contributor

@loicmathieu The way to keep the current format might be to somehow assign a custom JSON Schema "type" for the TIME defaults field... This way it would be possible to drop the JSON Schema time format and define a simple pattern matching the current format, such as values 10:11:12, instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To review
Development

Successfully merging this pull request may close these issues.

Flow's untyped input defaults can lead to runtime errors
3 participants