Skip to content

Commit

Permalink
fix(): prerender variables (#2588)
Browse files Browse the repository at this point in the history 
* fix(): prerender variables

* test(): fix test

* fix(): review changes

* fix(): review changes

* fix(core): return optional of object instad of empty() when no type match
  • Loading branch information
@Skraye
Skraye committed Nov 30, 2023
1 parent 61db3cc commit 2e1c373
Show file tree
Hide file tree
Showing 7 changed files with 74 additions and 31 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/io/kestra/core/runners/RunContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ public InputStream getTaskStateFile(String state, String name, Boolean isNamespa
URI uri = URI.create(this.taskStateFilePathPrefix(state, isNamespace, useTaskRun));
URI resolve = uri.resolve(uri.getPath() + "/" + name);

return this.storageInterface.get(getTenantId(), resolve);
return this.storageInterface.get(getTenantId(), resolve);
}

public URI putTaskStateFile(byte[] content, String state, String name) throws IOException {
Expand Down
83 changes: 59 additions & 24 deletions core/src/main/java/io/kestra/core/runners/VariableRenderer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@
import com.github.jknack.handlebars.HandlebarsException;
import com.github.jknack.handlebars.Template;
import com.github.jknack.handlebars.helper.*;
import io.pebbletemplates.pebble.PebbleEngine;
import io.pebbletemplates.pebble.error.AttributeNotFoundException;
import io.pebbletemplates.pebble.error.PebbleException;
import io.pebbletemplates.pebble.extension.AbstractExtension;
import io.pebbletemplates.pebble.template.PebbleTemplate;
import io.kestra.core.exceptions.IllegalVariableEvaluationException;
import io.kestra.core.runners.handlebars.VariableRendererPlugins;
import io.kestra.core.runners.handlebars.helpers.*;
Expand All @@ -18,6 +13,15 @@
import io.kestra.core.runners.pebble.PebbleLruCache;
import io.micronaut.context.ApplicationContext;
import io.micronaut.context.annotation.ConfigurationProperties;
import io.micronaut.core.annotation.Nullable;
import io.pebbletemplates.pebble.PebbleEngine;
import io.pebbletemplates.pebble.error.AttributeNotFoundException;
import io.pebbletemplates.pebble.error.PebbleException;
import io.pebbletemplates.pebble.extension.AbstractExtension;
import io.pebbletemplates.pebble.template.PebbleTemplate;
import jakarta.inject.Inject;
import jakarta.inject.Singleton;
import lombok.Getter;

import java.io.IOException;
import java.io.StringWriter;
Expand All @@ -26,11 +30,6 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import io.micronaut.core.annotation.Nullable;
import jakarta.inject.Inject;
import jakarta.inject.Singleton;
import lombok.Getter;

@Singleton
public class VariableRenderer {
private static final Pattern RAW_PATTERN = Pattern.compile("\\{%[-]*\\s*raw\\s*[-]*%\\}(.*?)\\{%[-]*\\s*endraw\\s*[-]*%\\}");
Expand Down Expand Up @@ -93,6 +92,10 @@ public VariableRenderer(ApplicationContext applicationContext, @Nullable Variabl
}

public String recursiveRender(String inline, Map<String, Object> variables) throws IllegalVariableEvaluationException {
return recursiveRender(inline, variables, true);
}

public String recursiveRender(String inline, Map<String, Object> variables, Boolean preprocessVariables) throws IllegalVariableEvaluationException {
if (inline == null) {
return null;
}
Expand All @@ -111,6 +114,11 @@ public String recursiveRender(String inline, Map<String, Object> variables) thro
return uuid;
});

// pre-process variables
if (preprocessVariables) {
variables = this.preprocessVariables(variables);
}

boolean isSame = false;
String current = "";
PebbleTemplate compiledTemplate;
Expand All @@ -131,28 +139,43 @@ public String recursiveRender(String inline, Map<String, Object> variables) thro
}

try {
Template template = handlebars.compileInline(currentTemplate);
Template template = handlebars.compileInline(currentTemplate);
current = template.apply(variables);
} catch (HandlebarsException | IOException hbE) {
throw new IllegalVariableEvaluationException(
"Pebble evaluation failed with '" + e.getMessage() + "' " +
"and Handlebars fallback failed also with '" + hbE.getMessage() + "'" ,
"Pebble evaluation failed with '" + e.getMessage() + "' " +
"and Handlebars fallback failed also with '" + hbE.getMessage() + "'",
e
);
}
}


isSame = currentTemplate.equals(current);
currentTemplate = current;
}

// post-process raw tags
for(var entry: replacers.entrySet()) {
for (var entry : replacers.entrySet()) {
current = current.replace(entry.getKey(), entry.getValue());
}
return current;
}

public Map<String, Object> preprocessVariables(Map<String, Object> variables) throws IllegalVariableEvaluationException {
Map<String, Object> currentVariables = variables;
Map<String, Object> previousVariables;
boolean isSame = false;

while (!isSame) {
previousVariables = currentVariables;
currentVariables = this.render(currentVariables, variables, false);
isSame = previousVariables.equals(currentVariables);
}

return currentVariables;
}

public IllegalVariableEvaluationException properPebbleException(PebbleException e) {
if (e instanceof AttributeNotFoundException current) {
return new IllegalVariableEvaluationException(
Expand All @@ -166,17 +189,27 @@ public IllegalVariableEvaluationException properPebbleException(PebbleException
return new IllegalVariableEvaluationException(e);
}

// By default, render() will render variables
// so we keep the previous behavior
public String render(String inline, Map<String, Object> variables) throws IllegalVariableEvaluationException {
return this.recursiveRender(inline, variables);
return this.recursiveRender(inline, variables, true);
}

public String render(String inline, Map<String, Object> variables, boolean preprocessVariables) throws IllegalVariableEvaluationException {
return this.recursiveRender(inline, variables, preprocessVariables);
}

// Default behavior
public Map<String, Object> render(Map<String, Object> in, Map<String, Object> variables) throws IllegalVariableEvaluationException {
return render(in, variables, true);
}

public Map<String, Object> render(Map<String, Object> in, Map<String, Object> variables, boolean preprocessVariables) throws IllegalVariableEvaluationException {
Map<String, Object> map = new HashMap<>();

for (Map.Entry<String, Object> r : in.entrySet()) {
String key = this.render(r.getKey(), variables);
Object value = renderObject(r.getValue(), variables).orElse(r.getValue());
String key = this.render(r.getKey(), variables, preprocessVariables);
Object value = renderObject(r.getValue(), variables, preprocessVariables).orElse(r.getValue());

map.putIfAbsent(
key,
Expand All @@ -188,23 +221,25 @@ public Map<String, Object> render(Map<String, Object> in, Map<String, Object> va
}

@SuppressWarnings({"unchecked", "rawtypes"})
private Optional<Object> renderObject(Object object, Map<String, Object> variables) throws IllegalVariableEvaluationException {
private Optional<Object> renderObject(Object object, Map<String, Object> variables, boolean preprocessVariables) throws IllegalVariableEvaluationException {
if (object instanceof Map) {
return Optional.of(this.render((Map) object, variables));
return Optional.of(this.render((Map) object, variables, preprocessVariables));
} else if (object instanceof Collection) {
return Optional.of(this.renderList((List) object, variables));
return Optional.of(this.renderList((List) object, variables, preprocessVariables));
} else if (object instanceof String) {
return Optional.of(this.render((String) object, variables));
return Optional.of(this.render((String) object, variables, preprocessVariables));
} else if (object == null) {
return Optional.empty();
}

return Optional.empty();
return Optional.of(object);
}

public List<Object> renderList(List<Object> list, Map<String, Object> variables) throws IllegalVariableEvaluationException {
private List<Object> renderList(List<Object> list, Map<String, Object> variables, boolean preprocessVariables) throws IllegalVariableEvaluationException {
List<Object> result = new ArrayList<>();

for (Object inline : list) {
this.renderObject(inline, variables)
this.renderObject(inline, variables, preprocessVariables)
.ifPresent(result::add);
}

Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/io/kestra/core/models/hierarchies/FlowGraphTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ void simple() throws IllegalVariableEvaluationException {
Flow flow = this.parse("flows/valids/return.yaml");
FlowGraph flowGraph = GraphUtils.flowGraph(flow, null);

assertThat(flowGraph.getNodes().size(), is(5));
assertThat(flowGraph.getEdges().size(), is(4));
assertThat(flowGraph.getNodes().size(), is(6));
assertThat(flowGraph.getEdges().size(), is(5));
assertThat(flowGraph.getClusters().size(), is(0));

assertThat(((AbstractGraphTask) flowGraph.getNodes().get(2)).getTask().getId(), is("date"));
Expand Down
4 changes: 3 additions & 1 deletion core/src/test/java/io/kestra/core/runners/RunContextTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.concurrent.TimeoutException;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.*;

@Property(name = "kestra.tasks.tmp-dir.path", value = "/tmp/sub/dir/tmp/")
Expand Down Expand Up @@ -112,14 +113,15 @@ void inputsLarge() throws TimeoutException {
void variables() throws TimeoutException {
Execution execution = runnerUtils.runOne(null, "io.kestra.tests", "return");

assertThat(execution.getTaskRunList(), hasSize(3));
assertThat(execution.getTaskRunList(), hasSize(4));

assertThat(
ZonedDateTime.from(ZonedDateTime.parse((String) execution.getTaskRunList().get(0).getOutputs().get("value"))),
ZonedDateTimeMatchers.within(10, ChronoUnit.SECONDS, ZonedDateTime.now())
);
assertThat(execution.getTaskRunList().get(1).getOutputs().get("value"), is("task-id"));
assertThat(execution.getTaskRunList().get(2).getOutputs().get("value"), is("return"));
assertThat((String) execution.getTaskRunList().get(3).getOutputs().get("value"), containsString("toto"));
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/io/kestra/core/runners/pebble/PebbleVariableRendererTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
import io.kestra.core.runners.VariableRenderer;
import io.kestra.core.utils.Rethrow;
import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
import jakarta.inject.Inject;
import org.junit.jupiter.api.Test;

import java.time.ZonedDateTime;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import jakarta.inject.Inject;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.*;
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/resources/flows/templates/with-template.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ inputs:

variables:
var-1: var-1
var-2: '{{ var-1 }}'
var-2: "{{ 'var-1' }}"

taskDefaults:
- type: io.kestra.core.tasks.log.Log
Expand Down
8 changes: 7 additions & 1 deletion core/src/test/resources/flows/valids/return.yaml
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
id: return
namespace: io.kestra.tests

variables:
period: "{{ schedule.date ?? execution.startDate }}"

tasks:
- id: date
type: io.kestra.core.tasks.debugs.Return
Expand All @@ -10,4 +13,7 @@ tasks:
format: "{{task.id}}"
- id: flow-id
type: io.kestra.core.tasks.debugs.Return
format: "{{flow.id}}"
format: "{{flow.id}}"
- id: variables
type: io.kestra.core.tasks.debugs.Return
format: "{{ vars.period | replace({':': 'toto'}) }}"

0 comments on commit 2e1c373

Please sign in to comment.