Repository: kestra-io/kestra
Issue: Invalid props in taskDefaults doesn't show validation error (save looks unresponsive)
Labels: bug

### Expected Behavior

When we provide wrong taskDefaults we want to have a feedback of which property is wrong to be able to correct them

### Actual Behaviour

Save still in purple but is unresponsive and provide no feedback about the error (see #1610)

### Steps To Reproduce

_No response_

### Environment Information

- Kestra Version:
- Operating System (OS / Docker / Kubernetes):
- Java Version (If not docker):


### Example flow

_No response_

Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/core/src/main/java/io/kestra/core/serializers/YamlFlowParser.java b/core/src/main/java/io/kestra/core/serializers/YamlFlowParser.java
index 3f2e4e4523..a321458fc7 100644
--- a/core/src/main/java/io/kestra/core/serializers/YamlFlowParser.java
+++ b/core/src/main/java/io/kestra/core/serializers/YamlFlowParser.java
@@ -20,6 +20,7 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.util.Collections;
+import java.util.Map;
 import java.util.Set;
 
 @Singleton
@@ -41,6 +42,18 @@ public <T> T parse(String input, Class<T> cls) {
         return readFlow(mapper, input, cls, type(cls));
     }
 
+    public <T> T parse(Map<String, Object> input, Class<T> cls) {
+        try {
+            return mapper.convertValue(input, cls);
+        } catch (IllegalArgumentException e) {
+            if(e.getCause() instanceof JsonProcessingException jsonProcessingException) {
+                jsonProcessingExceptionHandler(input, type(cls), jsonProcessingException);
+            }
+
+            throw e;
+        }
+    }
+
     private static <T> String type(Class<T> cls) {
         return cls.getSimpleName().toLowerCase();
     }
@@ -78,59 +91,66 @@ private <T> T readFlow(ObjectMapper mapper, String input, Class<T> objectClass,
         try {
             return mapper.readValue(input, objectClass);
         } catch (JsonProcessingException e) {
-            if (e.getCause() instanceof ConstraintViolationException constraintViolationException) {
-                throw constraintViolationException;
-            }
-            else if (e instanceof InvalidTypeIdException invalidTypeIdException) {
-                // This error is thrown when a non-existing task is used
-                throw new ConstraintViolationException(
-                    "Invalid type: " + invalidTypeIdException.getTypeId(),
-                    Set.of(
-                        ManualConstraintViolation.of(
-                            "Invalid type: " + invalidTypeIdException.getTypeId(),
-                            input,
-                            String.class,
-                            invalidTypeIdException.getPathReference(),
-                            null
-                        ),
-                        ManualConstraintViolation.of(
-                            e.getMessage(),
-                            input,
-                            String.class,
-                            invalidTypeIdException.getPathReference(),
-                            null
-                        )
+            jsonProcessingExceptionHandler(input, resource, e);
+        }
+
+        return null;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> void jsonProcessingExceptionHandler(T target, String resource, JsonProcessingException e) throws ConstraintViolationException {
+        if (e.getCause() instanceof ConstraintViolationException constraintViolationException) {
+            throw constraintViolationException;
+        }
+        else if (e instanceof InvalidTypeIdException invalidTypeIdException) {
+            // This error is thrown when a non-existing task is used
+            throw new ConstraintViolationException(
+                "Invalid type: " + invalidTypeIdException.getTypeId(),
+                Set.of(
+                    ManualConstraintViolation.of(
+                        "Invalid type: " + invalidTypeIdException.getTypeId(),
+                        target,
+                        (Class<T>) target.getClass(),
+                        invalidTypeIdException.getPathReference(),
+                        null
+                    ),
+                    ManualConstraintViolation.of(
+                        e.getMessage(),
+                        target,
+                        (Class<T>) target.getClass(),
+                        invalidTypeIdException.getPathReference(),
+                        null
                     )
-                );
-            }
-            else if (e instanceof UnrecognizedPropertyException unrecognizedPropertyException) {
-                var message = unrecognizedPropertyException.getOriginalMessage() + unrecognizedPropertyException.getMessageSuffix();
-                throw new ConstraintViolationException(
-                    message,
-                    Collections.singleton(
-                        ManualConstraintViolation.of(
-                            e.getCause() == null ? message : message + "\nCaused by: " + e.getCause().getMessage(),
-                            input,
-                            String.class,
-                            unrecognizedPropertyException.getPathReference(),
-                            null
-                        )
-                    ));
-            }
-            else {
-                throw new ConstraintViolationException(
-                    "Illegal "+ resource +" yaml: " + e.getMessage(),
-                    Collections.singleton(
-                        ManualConstraintViolation.of(
-                            e.getCause() == null ? e.getMessage() : e.getMessage() + "\nCaused by: " + e.getCause().getMessage(),
-                            input,
-                            String.class,
-                            "flow",
-                            null
-                        )
+                )
+            );
+        }
+        else if (e instanceof UnrecognizedPropertyException unrecognizedPropertyException) {
+            var message = unrecognizedPropertyException.getOriginalMessage() + unrecognizedPropertyException.getMessageSuffix();
+            throw new ConstraintViolationException(
+                message,
+                Collections.singleton(
+                    ManualConstraintViolation.of(
+                        e.getCause() == null ? message : message + "\nCaused by: " + e.getCause().getMessage(),
+                        target,
+                        (Class<T>) target.getClass(),
+                        unrecognizedPropertyException.getPathReference(),
+                        null
                     )
-                );
-            }
+                ));
+        }
+        else {
+            throw new ConstraintViolationException(
+                "Illegal "+ resource +" yaml: " + e.getMessage(),
+                Collections.singleton(
+                    ManualConstraintViolation.of(
+                        e.getCause() == null ? e.getMessage() : e.getMessage() + "\nCaused by: " + e.getCause().getMessage(),
+                        target,
+                        (Class<T>) target.getClass(),
+                        "flow",
+                        null
+                    )
+                )
+            );
         }
     }
 }
diff --git a/core/src/main/java/io/kestra/core/services/TaskDefaultService.java b/core/src/main/java/io/kestra/core/services/TaskDefaultService.java
index ba387749de..b8b9fe3b21 100644
--- a/core/src/main/java/io/kestra/core/services/TaskDefaultService.java
+++ b/core/src/main/java/io/kestra/core/services/TaskDefaultService.java
@@ -10,6 +10,7 @@
 import io.kestra.core.queues.QueueInterface;
 import io.kestra.core.runners.RunContextLogger;
 import io.kestra.core.serializers.JacksonMapper;
+import io.kestra.core.serializers.YamlFlowParser;
 import io.kestra.core.utils.MapUtils;
 import io.micronaut.core.annotation.Nullable;
 import org.slf4j.Logger;
@@ -28,6 +29,9 @@ public class TaskDefaultService {
     @Inject
     protected TaskGlobalDefaultConfiguration globalDefault;
 
+    @Inject
+    protected YamlFlowParser yamlFlowParser;
+
     @Inject
     @Named(QueueFactoryInterface.WORKERTASKLOG_NAMED)
     protected QueueInterface<LogEntry> logQueue;
@@ -116,7 +120,7 @@ public Flow injectDefaults(Flow flow) throws ConstraintViolationException {
             flowAsMap.put("taskDefaults", taskDefaults);
         }
 
-        return JacksonMapper.toMap(flowAsMap, Flow.class);
+        return yamlFlowParser.parse(flowAsMap, Flow.class);
     }
 
     private static Object recursiveDefaults(Object object, Map<String, List<TaskDefault>> defaults) {


Test patch:
diff --git a/core/src/test/java/io/kestra/core/runners/TaskDefaultsCaseTest.java b/core/src/test/java/io/kestra/core/runners/TaskDefaultsCaseTest.java
index 379b54d037..57e1dc513d 100644
--- a/core/src/test/java/io/kestra/core/runners/TaskDefaultsCaseTest.java
+++ b/core/src/test/java/io/kestra/core/runners/TaskDefaultsCaseTest.java
@@ -12,35 +12,45 @@
 import io.kestra.core.models.tasks.Task;
 import io.kestra.core.queues.QueueFactoryInterface;
 import io.kestra.core.queues.QueueInterface;
+import io.kestra.core.repositories.FlowRepositoryInterface;
 import io.kestra.core.services.GraphService;
+import io.kestra.core.services.TaskDefaultService;
 import io.kestra.core.utils.TestsUtils;
+import jakarta.inject.Inject;
+import jakarta.inject.Named;
+import jakarta.inject.Singleton;
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.NoArgsConstructor;
 import lombok.ToString;
 import lombok.experimental.SuperBuilder;
 
+import javax.validation.ConstraintViolationException;
+import javax.validation.Valid;
+import javax.validation.constraints.NotEmpty;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Objects;
+import java.util.Optional;
 import java.util.concurrent.TimeoutException;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
-import jakarta.inject.Inject;
-import jakarta.inject.Named;
-import jakarta.inject.Singleton;
-import javax.validation.Valid;
-import javax.validation.constraints.NotEmpty;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 @Singleton
 public class TaskDefaultsCaseTest {
     @Inject
     private RunnerUtils runnerUtils;
 
+    @Inject
+    private TaskDefaultService taskDefaultService;
+
+    @Inject
+    private FlowRepositoryInterface flowRepository;
+
     @Inject
     @Named(QueueFactoryInterface.WORKERTASKLOG_NAMED)
     private QueueInterface<LogEntry> logQueue;
@@ -74,6 +84,13 @@ public void invalidTaskDefaults() throws TimeoutException {
         assertThat(execution.getTaskRunList(), hasSize(1));
         LogEntry matchingLog = TestsUtils.awaitLog(logs, log -> log.getMessage().contains("Unrecognized field \"invalid\""));
         assertThat(matchingLog, notNullValue());
+
+        ConstraintViolationException constraintViolationException = assertThrows(ConstraintViolationException.class, () -> taskDefaultService.injectDefaults(flowRepository
+            .findById("io.kestra.tests", "invalid-task-defaults", Optional.empty())
+            .orElseThrow()));
+
+        assertThat(constraintViolationException.getConstraintViolations().size(), is(1));
+        assertThat(constraintViolationException.getMessage(), containsString("Unrecognized field \"invalid\""));
     }
 
     @SuperBuilder
@@ -117,7 +134,7 @@ public List<Task> allChildTasks() {
         }
 
         @Override
-        public List<ResolvedTask> childTasks(RunContext runContext, TaskRun parentTaskRun) throws IllegalVariableEvaluationException {
+        public List<ResolvedTask> childTasks(RunContext runContext, TaskRun parentTaskRun) {
             return FlowableUtils.resolveTasks(this.tasks, parentTaskRun);
         }