Skip to content

Add max string length to Jinjava config.#275

Merged
mattcoley merged 5 commits into
masterfrom
add-max-string-length
Jan 17, 2019
Merged

Add max string length to Jinjava config.#275
mattcoley merged 5 commits into
masterfrom
add-max-string-length

Conversation

@mattcoley

Copy link
Copy Markdown
Collaborator

Adds a max string length to the config. As a test, I have used this setting to limit the output of the Join filter, however it can be used in other filters as well if the approach looks fine.

@mattcoley mattcoley requested review from boulter and hs-lsong January 17, 2019 18:04
@codecov-io

codecov-io commented Jan 17, 2019

Copy link
Copy Markdown

Codecov Report

Merging #275 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #275      +/-   ##
============================================
+ Coverage     72.19%   72.29%   +0.09%     
- Complexity     1475     1477       +2     
============================================
  Files           231      231              
  Lines          4571     4587      +16     
  Branches        731      732       +1     
============================================
+ Hits           3300     3316      +16     
  Misses         1016     1016              
  Partials        255      255
Impacted Files Coverage Δ Complexity Δ
...c/main/java/com/hubspot/jinjava/JinjavaConfig.java 85% <100%> (+1%) 19 <4> (+1) ⬆️
...ava/com/hubspot/jinjava/lib/filter/JoinFilter.java 96.42% <100%> (+2.31%) 7 <5> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83cf48f...f55d9c0. Read the comment docs.

} catch (OutputTooBigException ex) {
interpreter.addError(new TemplateError(ErrorType.WARNING,
ErrorReason.OTHER,
ErrorItem.OTHER,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ErrorItem.FILTER

}
stringBuilder.append(Objects.toString(val, ""));
} catch (OutputTooBigException ex) {
interpreter.addError(new TemplateError(ErrorType.WARNING,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems FATAL.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not necessarily FATAL IMO since it will still output a string, just shorter than expected.

}

vals.add(Objects.toString(val, ""));
try {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can also simple sum the length of each string and compare to the limit. It may look simpler. Up to you.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That's how the modified string builder works and throws the exception once the length > limit. I didn't see the need to duplicate the code we already have in there.

@boulter boulter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

anywhere else we can apply this? ~ operator?

}

@Test
public void itLimitsString() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: itLimitsStringLength

@mattcoley

Copy link
Copy Markdown
Collaborator Author

Yes I am planning adding this elsewhere once the approach is approved.


RenderResult result = jinjava.renderForResult("{{ [1, 2, 3, 4, 5]|join('|') }}", new HashMap<String, Object>());
assertThat(result.getOutput()).isEqualTo("1|2|3");
assertThat(result.getErrors().size()).isEqualTo(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it would be good to verify that error is what you expect.

@mattcoley mattcoley merged commit ca7f589 into master Jan 17, 2019
@mattcoley mattcoley deleted the add-max-string-length branch January 17, 2019 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants