Skip to content

Avoid re-registration if member not found in UnsafeMemberAccessStrategy#783

Merged
sebastienros merged 1 commit intosebastienros:mainfrom
lahma:check-type-registration
Apr 21, 2025
Merged

Avoid re-registration if member not found in UnsafeMemberAccessStrategy#783
sebastienros merged 1 commit intosebastienros:mainfrom
lahma:check-type-registration

Conversation

@lahma
Copy link
Copy Markdown
Collaborator

@lahma lahma commented Apr 21, 2025

UnsafeMemberAccessStrategy will always call Register again if member was not found. It's enough to have invalid reference in Liquid template for this endless re-registration to kick in.

@sebastienros sebastienros merged commit 89fcd8c into sebastienros:main Apr 21, 2025
1 check passed
@lahma lahma deleted the check-type-registration branch April 21, 2025 16:45
@lahma
Copy link
Copy Markdown
Collaborator Author

lahma commented Apr 21, 2025

I think this solution is not yet perfect because if you have nonexistent member in a concrete type that doesn't change its shape (be ware of dictionary, dynamic etc) the result could be cached as shared undefined accessor.

@sebastienros
Copy link
Copy Markdown
Owner

Do you mean there is a bug in your PR or before it?

@lahma
Copy link
Copy Markdown
Collaborator Author

lahma commented Apr 21, 2025

To my understanding, it's currently a feature. The path goes to base.GetAccessor(type, name) which then finds its way to GetUnlikely and traverses interfaces etc and eventually returns null again. With fixed types/shapes this should be a definite answer and cacheable, but of course you could create a DoS by generating properties that do not exist and get cached...

@sebastienros
Copy link
Copy Markdown
Owner

Right, I think I am now caching it in the other branch I mentioned.

@sebastienros
Copy link
Copy Markdown
Owner

BTW, totally off-topic. The IncludeStatement caches the inner template file only if the value of the expression is the same. For instance {% include my_var %} is supported, and my_var contains the name of the template. The include statement instance as a single nullable ref to the parsed template, and so the template is cached if the variable has the same value during two consecutive evaluations.

Thoughts:

  • one value is too small for this kind of usage, if this is a variable there is definitely more than one and then there is useless processing.
  • unlimited values (dictionary) could become a DOS vector if the variable was built from outside data. But at the same time it's only cached if the file actually exists (coming from a FileProvider) so in practice there should only be a defined number of templates. In theory though these templates could be dynamic (cms?) and grow indefinitely.

What kind of structure/strategy would be appropriate here? LRU with a size limit? Weak dictionary?

@lahma
Copy link
Copy Markdown
Collaborator Author

lahma commented Apr 21, 2025

What kind of structure/strategy would be appropriate here? LRU with a size limit? Weak dictionary?

I don't have definitive answer, like always.. Maybe for "cache miss" data it would make sense to cache a fixed set of data - for example last 10 misses that are bound to context (like Type), that could ensure that it's not unbounded. Weak dictionaries are a bit hard as they need to have something to attach to and in these cases it might not be obvious what's the GC root - I might be wrong as I really haven't thought about the problem that much / or have context.

@sebastienros
Copy link
Copy Markdown
Owner

I will ask AI, it knows everything

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.

2 participants