Skip to content

add size limited pymaps and pylists#530

Merged
boulter merged 6 commits into
masterfrom
add-size-limited-pymaps-and-pylists
Nov 13, 2020
Merged

add size limited pymaps and pylists#530
boulter merged 6 commits into
masterfrom
add-size-limited-pymaps-and-pylists

Conversation

@boulter

@boulter boulter commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

User-provided templates should not be allowed to use unlimited resources. This is a failsafe that adds a limit the maximum size of lists and maps. The default is no (practical) limit.

This is similar to #137

@mattcoley mattcoley left a comment

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.

Throwing the OutputTooBigException in the LengthLimitingStringBuilder has been a bit of a mess trying to handle that runtime exception in multiple places. Would it make better sense to truncate the collection to keep it under the limit and add a warning? This would protect against the situation where data being pulled in gets continually larger until it trips the limit which could break templates rather than just truncate them.

@boulter

boulter commented Nov 10, 2020

Copy link
Copy Markdown
Contributor Author

This throws an IndexOutOfRangeException which extends InterpretException, so may be already handled. OutputTooBigException extends RuntimeException directly. You could argue that IndexOutOfRangeException is not quite right and maybe I could add another exception which also extends InterpretException.

@jasmith-hs jasmith-hs 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.

I like this idea. If it's wrapped in an ELException, it would be handled here by adding a warning-level template error


@Override
public boolean addAll(int index, Collection<?> elements) {
if (size() + elements.size() >= maxSize) {

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.

Integer overflow might be a concern. It should be safer to do size() >= maxSize - elements.size()

@boulter boulter Nov 12, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we will run out of memory long before this if a collection has 2B items in it!

Comment thread src/main/java/com/hubspot/jinjava/objects/collections/SizeLimitingPyList.java Outdated
@boulter

boulter commented Nov 13, 2020

Copy link
Copy Markdown
Contributor Author

@mattcoley I decided to create a new CollectionTooBigException because IndexOutOfRangeException was not appropriate and this is a pretty severe error. I don't want to just ignore it and continue because it could end up creating thousands of error messages. These limits are really high, so I don't think this will come up very often, but it should save the renderer when it does.

@jasmith-hs

Copy link
Copy Markdown
Contributor

What if it's a warning when it's at like 90% capacity to help prevent the situation that Matt described

@boulter

boulter commented Nov 13, 2020

Copy link
Copy Markdown
Contributor Author

What if it's a warning when it's at like 90% capacity to help prevent the situation that Matt described

Done. Check it out.


public class SizeLimitingPyList extends PyList implements PyWrapper {
private int maxSize;
private boolean hasWarned;

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.

Nice

@boulter boulter merged commit 3892ab9 into master Nov 13, 2020
@boulter boulter deleted the add-size-limited-pymaps-and-pylists branch November 13, 2020 20:15
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