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

Introduces a limited length render filter and HTML tag close filter #1128

Merged
merged 16 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,11 @@ public String renderFlat(String template) {
* @return rendered result
*/
public String render(String template) {
return render(parse(template), true);
return render(template, config.getMaxOutputSize());
}

public String render(String template, long renderLimit) {
return render(parse(template), true, renderLimit);
}

/**
Expand All @@ -270,7 +274,19 @@ public String render(String template) {
* @return rendered result
*/
public String render(Node root) {
return render(root, true);
return render(root, true, config.getMaxOutputSize());
}

/**
* Render the given root node with an option to process extend parents.
* Equivalent to render(root, processExtendRoots).
* @param root
* node to render
* @param processExtendRoots
* @return
*/
public String render(Node root, boolean processExtendRoots) {
return render(root, processExtendRoots, config.getMaxOutputSize());
}

/**
Expand All @@ -280,11 +296,15 @@ public String render(Node root) {
* node to render
* @param processExtendRoots
* if true, also render all extend parents
* @param renderLimit
* the number of characters the result may contain
* @return rendered result
*/
public String render(Node root, boolean processExtendRoots) {
OutputList output = new OutputList(config.getMaxOutputSize());

private String render(Node root, boolean processExtendRoots, long renderLimit) {
long safeRenderSize = (config.getMaxOutputSize() == 0)
? renderLimit
: Math.min(renderLimit, config.getMaxOutputSize());
OutputList output = new OutputList(safeRenderSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone could pass a negative or 0-value render limit and bypass the max output size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it would be good to test these different cases for ensuring render limit doesn't allow rendering more than configured

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So by default, this using 0 (unlimited) as the render size makes me wonder what the problem is here. Do we have cases where we modify this config value for limited length rendering? As per my logic here in the filter, if no value/an invalid value is provided, we just pass in 0 anyway, so that to me seems like an easy workaround.

Right now, config.getMaxOutputSize() returns 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If config.getMaxOutputSize() is 100 and renderLimit is 0, then it will bypass the configured limit and render unlimited characters.
For HubL, we are always setting a non-zero value for config.getMaxOutputSize()

for (Node node : root.getChildren()) {
lineNumber = node.getLineNumber();
position = node.getStartPosition();
Expand Down Expand Up @@ -340,8 +360,8 @@ public String render(Node root, boolean processExtendRoots) {
return output.getValue();
}
}
StringBuilder ignoredOutput = new StringBuilder();

StringBuilder ignoredOutput = new StringBuilder();
// render all extend parents, keeping the last as the root output
if (processExtendRoots) {
Set<String> extendPaths = new HashSet<>();
Expand Down Expand Up @@ -406,6 +426,7 @@ public String render(Node root, boolean processExtendRoots) {
}

resolveBlockStubs(output);

if (ignoredOutput.length() > 0) {
return (
EagerReconstructionUtils.labelWithNotes(
Expand All @@ -421,7 +442,6 @@ public String render(Node root, boolean processExtendRoots) {
output.getValue()
);
}

return output.getValue();
}

Expand Down
26 changes: 26 additions & 0 deletions src/main/java/com/hubspot/jinjava/lib/filter/CloseHtmlFilter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.hubspot.jinjava.lib.filter;

import com.hubspot.jinjava.doc.annotations.JinjavaDoc;
import com.hubspot.jinjava.doc.annotations.JinjavaParam;
import com.hubspot.jinjava.doc.annotations.JinjavaSnippet;
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
import java.util.Objects;
import org.jsoup.Jsoup;

@JinjavaDoc(
value = "Closes open HTML tags in a string",
input = @JinjavaParam(value = "s", desc = "String to close", required = true),
snippets = { @JinjavaSnippet(code = "{{ \"<p> Hello, world\"|closehtml }}") }
)
public class CloseHtmlFilter implements Filter {

@Override
public String getName() {
return "closehtml";
}

@Override
public Object filter(Object var, JinjavaInterpreter interpreter, String... args) {
return Jsoup.parseBodyFragment(Objects.toString(var)).body().html();
}
}
12 changes: 12 additions & 0 deletions src/main/java/com/hubspot/jinjava/lib/filter/RenderFilter.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package com.hubspot.jinjava.lib.filter;

import com.hubspot.jinjava.JinjavaConfig;
import com.hubspot.jinjava.doc.annotations.JinjavaDoc;
import com.hubspot.jinjava.doc.annotations.JinjavaParam;
import com.hubspot.jinjava.doc.annotations.JinjavaSnippet;
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
import java.util.Objects;
import org.apache.commons.lang3.math.NumberUtils;

@JinjavaDoc(
value = "Renders a template string early to be used by other filters and functions",
Expand All @@ -24,6 +26,16 @@ public String getName() {

@Override
public Object filter(Object var, JinjavaInterpreter interpreter, String... args) {
if (args.length > 0) {
String firstArg = args[0];
return interpreter.render(
Objects.toString(var),
NumberUtils.toLong(
firstArg,
JinjavaConfig.newBuilder().build().getMaxOutputSize()
)
);
}
return interpreter.render(Objects.toString(var));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.hubspot.jinjava.lib.filter;

import static org.assertj.core.api.Assertions.assertThat;

import com.hubspot.jinjava.BaseInterpretingTest;
import org.junit.Before;
import org.junit.Test;

public class CloseHtmlFilterTest extends BaseInterpretingTest {
CloseHtmlFilter f;

@Before
public void setup() {
f = new CloseHtmlFilter();
}

@Test
public void itClosesTags() {
String openTags = "<p>Hello, world!";
assertThat(f.filter(openTags, interpreter)).isEqualTo("<p>Hello, world!</p>");
}

@Test
public void itIgnoresClosedTags() {
String openTags = "<p>Hello, world!</p>";
assertThat(f.filter(openTags, interpreter)).isEqualTo("<p>Hello, world!</p>");
}

@Test
public void itClosesMultipleTags() {
String openTags = "<h1><p>Hello, world!";
assertThat(f.filter(openTags, interpreter))
.isEqualTo("<h1><p>Hello, world!</p></h1>");
}
}
38 changes: 38 additions & 0 deletions src/test/java/com/hubspot/jinjava/lib/filter/RenderFilterTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,42 @@ public void itRendersObject() {

assertThat(filter.filter(stringToRender, interpreter)).isEqualTo("world");
}

@Test
public void itRendersObjectWithinLimit() {
String stringToRender = "{% if null %}Hello{% else %}world{% endif %}";

assertThat(filter.filter(stringToRender, interpreter, "5")).isEqualTo("world");
}

@Test
public void itDoesNotRenderObjectOverLimit() {
String stringToRender = "{% if null %}Hello{% else %}world{% endif %}";

assertThat(filter.filter(stringToRender, interpreter, "4")).isEqualTo("");
}

@Test
public void itRendersPartialObjectOverLimit() {
String stringToRender = "Hello{% if null %}Hello{% else %}world{% endif %}";

assertThat(filter.filter(stringToRender, interpreter, "7")).isEqualTo("Hello");
}

@Test
public void itCountsHtmlTags() {
String stringToRender = "<p>Hello</p>{% if null %}Hello{% else %}world{% endif %}";

assertThat(filter.filter(stringToRender, interpreter, "15"))
.isEqualTo("<p>Hello</p>");
}

@Test
public void itDoesNotAlwaysCompleteHtmlTags() {
String stringToRender =
"<p> Hello, {% if null %}world{% else %}world!{% endif %} </p>";

assertThat(filter.filter(stringToRender, interpreter, "17"))
.isEqualTo("<p> Hello, world!");
}
}