-
Notifications
You must be signed in to change notification settings - Fork 169
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
Changes from 12 commits
8fcd5ef
bfb78f5
87e6a0e
baae46e
0a82efb
0e4692a
be44efa
630768f
16fb43f
daf2c84
b4c604f
09f051b
38b50a5
4e2e4a1
254d5fc
10b4ff6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -259,18 +259,46 @@ 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); | ||
} | ||
|
||
/** | ||
* Render the given root node, processing extend parents. Equivalent to render(root, true) | ||
* Render the given root node, processing extend parents. Equivalent to render(root, true, -1) | ||
* | ||
* @param root | ||
* node to render | ||
* @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, -1). | ||
* @param root | ||
* node to render | ||
* @param processExtendRoots | ||
* @return | ||
*/ | ||
public String render(Node root, boolean processExtendRoots) { | ||
return render(root, processExtendRoots, config.getMaxOutputSize()); | ||
} | ||
|
||
/** | ||
* Render the given root node to a certain limit, processing extend parents. | ||
* @param root | ||
* node to render | ||
* @param renderLimit | ||
* the number of characters in response text | ||
* @return | ||
*/ | ||
public String render(Node root, long renderLimit) { | ||
return render(root, true, renderLimit); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need to add this method |
||
} | ||
|
||
/** | ||
|
@@ -280,11 +308,12 @@ 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()); | ||
|
||
public String render(Node root, boolean processExtendRoots, long renderLimit) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't need to be public There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's easier to increase visibility later than to decrease it |
||
OutputList output = new OutputList(renderLimit); | ||
for (Node node : root.getChildren()) { | ||
lineNumber = node.getLineNumber(); | ||
position = node.getStartPosition(); | ||
|
@@ -340,8 +369,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<>(); | ||
|
@@ -406,6 +435,7 @@ public String render(Node root, boolean processExtendRoots) { | |
} | ||
|
||
resolveBlockStubs(output); | ||
|
||
if (ignoredOutput.length() > 0) { | ||
return ( | ||
EagerReconstructionUtils.labelWithNotes( | ||
|
@@ -421,7 +451,6 @@ public String render(Node root, boolean processExtendRoots) { | |
output.getValue() | ||
); | ||
} | ||
|
||
return output.getValue(); | ||
} | ||
|
||
|
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(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
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", | ||
|
@@ -24,6 +25,10 @@ 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, 0)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should cap the limit to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we should add some nicer error handling here if we can't coerce the String to a Long There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what I thought, except default max output size is 0 (unlimited I assume?) so clamping it always goes lower than the provided value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just changed the default value to explicitly take on whatever is in the config. We could instead just switch to the other case or throw an error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only clamp if the max output size is > 0 |
||
} | ||
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>"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to change this, also it's not the same as that since it passes the max output size from the config