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

asegal-hs · 2023-11-06T17:30:40Z

Currently, Jinjava provides filters for truncating HTML. The purpose of these is to shorten or summarize rich text. They correctly handle HTML so that tags are not left hanging open. However, they do not accommodate text that contains raw HubL as well as HTML. The purpose of this PR is to allow summaries of objects that contain raw HubL text.

This also includes a filter that closes HTML tags that may have been left hanging open.

Example usage in pseudocode:

user.name = Alice
text = "<p> Hello, {% user.name %} </p>"
rendered = text | render(13) // <p> Hello, Ali
closed = renderer | closehtml //<p> Hello, Ali</p>

This serves as an alternate option to the truncatehtml filter that will do the same thing to raw rich text but also correctly render HubL included in it.

jasmith-hs

These seem like good features to include

jasmith-hs · 2023-11-06T17:42:52Z

src/main/java/com/hubspot/jinjava/lib/filter/CloseHtmlFilter.java

+
+  @Override
+  public String getName() {
+    return "render";


Suggested change

return "render";

return "closehtml";

jasmith-hs · 2023-11-06T17:43:16Z

src/main/java/com/hubspot/jinjava/lib/filter/RenderFilter.java

+    if (args.length > 0) {
+      /*
+       This means a render limit length has been provided.
+       Here we begin a left to right render where we add to an HTML string until the length reaches a certain limit.


Yes, not necessarily an HTML string, but the functionality makes sense

…ML nodes

jasmith-hs

Looks good, just a few points that we should address before merging

jasmith-hs · 2023-11-16T12:58:43Z

src/main/java/com/hubspot/jinjava/lib/filter/RenderFilter.java

@@ -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));


We should cap the limit to config.getMaxOutputSize()

And we should add some nicer error handling here if we can't coerce the String to a Long

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.

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.

Only clamp if the max output size is > 0

jasmith-hs · 2023-11-16T12:59:24Z

src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java

-  public String render(Node root, boolean processExtendRoots) {
-    OutputList output = new OutputList(config.getMaxOutputSize());
-
+  public String render(Node root, boolean processExtendRoots, long renderLimit) {


This doesn't need to be public

It's easier to increase visibility later than to decrease it

jasmith-hs · 2023-11-16T13:00:44Z

src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java

+  public String render(Node root, long renderLimit) {
+    return render(root, true, renderLimit);


Don't need to add this method

jasmith-hs · 2023-11-16T13:02:47Z

src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java

  }

  /**
-   * 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)


Don't need to change this, also it's not the same as that since it passes the max output size from the config

… args

jasmith-hs · 2023-11-17T17:20:09Z

src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java

+    long safeRenderSize = (config.getMaxOutputSize() == 0)
+      ? renderLimit
+      : Math.min(renderLimit, config.getMaxOutputSize());
+    OutputList output = new OutputList(safeRenderSize);


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

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

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.

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()

jasmith-hs · 2023-11-21T19:14:12Z

src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java

+  private long clampOutputSizeSafely(long providedLimit) {
+    long configMaxOutput = config.getMaxOutputSize();
+
+    if (configMaxOutput == 0) {
+      return providedLimit;
+    }
+
+    if (providedLimit <= 0) {
+      return configMaxOutput;
+    }
+
+    return Math.min(providedLimit, configMaxOutput);
+  }


Looks good, can we just get some tests for this logic?

Done, thanks a lot Jack. To avoid repeating tests or changing signatures I moved this into a small static utils class which I tested on it's own. Let me know if you'd rather I just make it public in the interpreter or add more tests to the render method as a whole and take it back to being private.

I did like this a bit for simplicity though.

Stubs out filters

8fcd5ef

asegal-hs requested a review from jasmith-hs November 6, 2023 17:30

jasmith-hs reviewed Nov 6, 2023

View reviewed changes

asegal-hs added 11 commits November 6, 2023 13:08

Turned on prettier and changed filter name

bfb78f5

Stubbed out if case statement

87e6a0e

Using max output param to clamp outputs

baae46e

Calls new render limit method from filter

0a82efb

Basic functionality working with two tests

0e4692a

Added two more tests

be44efa

Removed close html filter because limited rendering works on whole HT…

630768f

…ML nodes

Added back closehtml filter and test case that demonstrates it

16fb43f

Added close html filter implementation with Jsoup

daf2c84

Using document traversal to close HTML tags

b4c604f

Removed references to safelist for Jsoup

09f051b

asegal-hs requested a review from jasmith-hs November 15, 2023 19:08

jasmith-hs requested changes Nov 16, 2023

View reviewed changes

asegal-hs added 2 commits November 16, 2023 13:19

Removed unnecessary method and formalized default case for non-number…

38b50a5

… args

Clamp output size to config max if not 0

4e2e4a1

asegal-hs requested a review from jasmith-hs November 17, 2023 15:17

jasmith-hs requested changes Nov 17, 2023

View reviewed changes

Added safer limit clamping function

254d5fc

jasmith-hs reviewed Nov 21, 2023

View reviewed changes

Extracted clamp to helper class with tests and fixed negative config bug

10b4ff6

jasmith-hs approved these changes Nov 21, 2023

View reviewed changes

asegal-hs merged commit bf66df2 into master Nov 27, 2023
4 checks passed

asegal-hs deleted the capped-rendering branch November 27, 2023 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

asegal-hs commented Nov 6, 2023

jasmith-hs left a comment

jasmith-hs Nov 6, 2023

jasmith-hs Nov 6, 2023

jasmith-hs left a comment

jasmith-hs Nov 16, 2023

jasmith-hs Nov 16, 2023

asegal-hs Nov 16, 2023

asegal-hs Nov 16, 2023

jasmith-hs Nov 16, 2023

jasmith-hs Nov 16, 2023

jasmith-hs Nov 16, 2023

jasmith-hs Nov 16, 2023

jasmith-hs Nov 16, 2023

jasmith-hs Nov 17, 2023

jasmith-hs Nov 17, 2023

asegal-hs Nov 17, 2023

jasmith-hs Nov 17, 2023

jasmith-hs Nov 21, 2023

asegal-hs Nov 21, 2023

		public String render(Node root, long renderLimit) {
		return render(root, true, renderLimit);

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

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

Conversation

asegal-hs commented Nov 6, 2023

jasmith-hs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmith-hs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment