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

_get_size_of_locals fails for non-hashable values #79

Closed
drewler opened this issue Oct 28, 2022 · 3 comments · Fixed by #80
Closed

_get_size_of_locals fails for non-hashable values #79

drewler opened this issue Oct 28, 2022 · 3 comments · Fixed by #80
Labels
bug Something isn't working

Comments

@drewler
Copy link

drewler commented Oct 28, 2022

When setting resource limits for a Context's by setting a local_namespace_limit, it fails when computing _get_size_of_locals if any of the values is not hashable (eg: a list) because it tries to de-duplicate them by putting them in a set.

liquid/liquid/context.py

Lines 468 to 473 in e123ce0

def _get_size_of_locals(self) -> int:
if not self.env.local_namespace_limit:
return 0
return (
sum(sys.getsizeof(obj, default=1) for obj in set(self.locals.values()))
+ self.local_namespace_size_carry

Code reproducing the issue:

from liquid import Environment

class ResourceLimitedEnvironment(Environment):
    # Limits the number of variables that can be defined within a template.
    local_namespace_limit = 100

template_string = """
{% assign beatles = "John, Paul, George, Ringo" | split: ", " %}

{% for member in beatles %}
  {{ member }}
{% endfor %}
"""

# Works
default_env = Environment()
template = default_env.from_string(template_string)
template.render()

# Doesn't work
resource_limited_env = ResourceLimitedEnvironment()
resource_limited_template = resource_limited_env.from_string(template_string)
resource_limited_template.render()
@jg-rp
Copy link
Owner

jg-rp commented Oct 29, 2022

Hi @drewler,

Thank you for reporting this bug.

Pull request #80 fixes this issue by not not building a set from a render context's local namespace. In the unlikely event that someone is relying on the old, broken behaviour, Context now exposes get_size_of_locals() for potential customization of the local namespace "size" calculation.

This example mimics the broken behaviour.

import sys

from liquid import Environment
from liquid import Context
from liquid.template import BoundTemplate

class MyRenderContext(Context):
    def get_size_of_locals(self) -> int:
        if not self.env.local_namespace_limit:
            return 0
        return (
            sum(sys.getsizeof(obj, default=1) for obj in set(self.locals.values()))
            + self.local_namespace_size_carry
        )


class MyBoundTemplate(BoundTemplate):
    context_class = MyRenderContext


class MyEnvironment(Environment):
    local_namespace_limit = 110  # XXX: very low, for demonstration purposes
    template_class = MyBoundTemplate


env = MyEnvironment()

template = env.from_string(
    "{% assign foo = 'hello' %}"
    "{% assign bar = 'world' %}"
).render()


# raises a LocalNamespaceLimitError
template = env.from_string(
    "{% assign foo = 'hello' %}"
    "{% assign bar = 'world' %}"
    "{% assign baz = '!' %}"
).render()

And this example simply counts the number of items in the namespace.

from liquid import Environment
from liquid import Context
from liquid.template import BoundTemplate

class MyRenderContext(Context):
    def get_size_of_locals(self) -> int:
        if not self.env.local_namespace_limit:
            return 0
        return len(self.locals) + self.local_namespace_size_carry


class MyBoundTemplate(BoundTemplate):
    context_class = MyRenderContext


class MyEnvironment(Environment):
    local_namespace_limit = 2  # XXX: very low, for demonstration purposes
    template_class = MyBoundTemplate


env = MyEnvironment()

template = env.from_string(
    "{% assign foo = 'hello' %}"
    "{% assign bar = 'world' %}"
).render()

# raises a LocalNamespaceLimitError
template = env.from_string(
    "{% assign foo = 'hello' %}"
    "{% assign bar = 'world' %}"
    "{% assign baz = '!' %}"
).render()

@jg-rp jg-rp closed this as completed in #80 Oct 30, 2022
@jg-rp
Copy link
Owner

jg-rp commented Oct 31, 2022

Fixed in version 1.4.7. Now released.

@drewler
Copy link
Author

drewler commented Oct 31, 2022

That was fast. Thank you @jg-rp!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants