cobalt-org / liquid-rust

Liquid templating for Rust
docs.rs/liquid
MIT License
463 stars 78 forks source link

Allow static analysis of performance #299

Open salman-ca opened 5 years ago

salman-ca commented 5 years ago

Just a few general questions on liquid-rust.

  1. would you say this implementation provides the same level of safety as the ruby version? i.e. since an end user can create their templates, they won't be able to do anything dangerous.

  2. Is there a way to introspect the liquid document. I believe in the ruby version you can see how many if statements, loops etc. the template has (this is probably to analyze the general complexity of the template - or limit the # of certain tags).

Great work!

epage commented 5 years ago
  1. I'd say this implementation is safer. The ruby version blurs the line between liquid and ruby and they have to go through extra hoops to ensure it is safe. The Rust version does not have any escape hatches like that.

  2. As of right now, there is not a way to introspect the document. Currently, the document gets turned into Box<dyn SomeTrait> where the implementations of the trait aren't exposed such that you can't even dynamic cast them. I'd be interested to know what the use case would be for us to consider opening this up which also help us know what would be the best way to open it up.

salman-ca commented 5 years ago

Regarding #2, the use case is as follows. If you allow the end user to create their own templates, they might write very inefficient templates such as abuse of looping statements etc. If you can introspect the types and # of statements, you could generate some kind of a usage score on the template (in addition to rendering the template - but introspection could prevent you from evening rendering if it is flagged as such).

That is the use case I am thinking about.

epage commented 5 years ago

So a static analysis profiler, interesting. I'll have to give this some thought on the best way to handle it.

On a related note, we also need to take care of https://github.com/cobalt-org/liquid-rust/issues/300.

salman-ca commented 5 years ago

Not sure if it is related to #300 but I know the liquid ruby also has an option where it will render the template and ignore any errors it encounters for some of the tags etc.

epage commented 5 years ago

Not sure if it is related to #300 but I know the liquid ruby also has an option where it will render the template and ignore any errors it encounters for some of the tags etc.

Yeah, currently liquid-rust is aiming to act as if strict, strict_variables, and strict_filters are all enabled.

I get the feeling they regret how lax everything is

It is recommended that you enable :strict or :warn mode on new apps to stop invalid templates from being created. It is also recommended that you use it in the template editors of existing apps to give editors better error messages.

From https://github.com/Shopify/liquid

Personally, I prefer the strictness to help catch issues in my templates (rather than relying on hand inspecting every feature). Are you bringing this up because you have a use case where you'd want things more lax? Could you describe it?

salman-ca commented 5 years ago

I understand what you are saying and I agree with you after thinking about it. Thanks.

chipsenkbeil commented 3 years ago

Personally, I prefer the strictness to help catch issues in my templates (rather than relying on hand inspecting every feature). Are you bringing this up because you have a use case where you'd want things more lax? Could you describe it?

@epage coming from https://github.com/cobalt-org/cobalt.rs/pull/866, I think I might have hit a case where a more lax support is available. That or cobalt's template parsing needs to be specially-handled for vimwiki.

In vimwiki, a code block looks like this:

{{{python
def my_func():
    print('hello world!')
}}}

This causes liquid to fail:

Info: caused by Failed to render content for index.html
Info: caused by liquid:  --> 8:3
  |
8 | {{{python␊
  |   ^---
  |
  = expected Value

So either cobalt would need to disable liquid in wiki files or the liquid parser would need to support ignoring errors like this. Thoughts?

epage commented 3 years ago

@chipsenkbeil please create separate issues for each of your requests.

RyanAmos commented 1 year ago

Not sure if it is related to #300 but I know the liquid ruby also has an option where it will render the template and ignore any errors it encounters for some of the tags etc.

Yeah, currently liquid-rust is aiming to act as if strict, strict_variables, and strict_filters are all enabled.

I get the feeling they regret how lax everything is

It is recommended that you enable :strict or :warn mode on new apps to stop invalid templates from being created. It is also recommended that you use it in the template editors of existing apps to give editors better error messages.

From https://github.com/Shopify/liquid

Personally, I prefer the strictness to help catch issues in my templates (rather than relying on hand inspecting every feature). Are you bringing this up because you have a use case where you'd want things more lax? Could you describe it?

@epage I have a use-case where it's necessary for us to reduce the rendering strictness, specifically for variables. We allow users to define their own templates and store them. Those templates are then used later to render values with global variables provided by the customers' data which may or may not have all of the variables present. Our current implementation just renders those missing variables as an empty string.

I've tried multiple work arounds to get around this, some of which were fairly hacked together, but they all had edge cases that weren't handled properly.

I'm curious if you're open to the idea of adding a "lax" rendering mode, at least for variables?

epage commented 1 year ago

@RyanAmos if you would like to discuss changing the strictness, we should move that out into a separate issue as this has a different focus.