zeripath commented 1 year ago

This proposal is about providing a way to pass a template.FuncMap at template execution overriding specific functions within the template.

When executing a template using one of new methods, then the provided template.FuncMap will be checked first.

Summary

Reasons:

Passing values to templates can only be achieved using the data any parameter and this is suboptimal for contextual rendering elements such as web nonces like csrf, or translation functions.
This is especially problematic when using sub-templates called from within template rendering. The template calling syntax only allows setting a single variable as its data, making passing this contextual rendering functions difficult.
The current suggestion is to use Clone(), Funcs() and then Execute() but this is slow and optimizing this appears very difficult. (#38114) It would require significant changes throughout the code and even if parent templates could be changed to allow inheritance, It would requires cloning every template that could be called in advance.

Advantages:

The proposal is easy to implement (see #54432)
The additional methods are simple to use and represent a very small API change.
The change will allow significant simplification of templates in web services.

Detailed rationale

Sometimes data needs to be rendered differently based on the context it is rendered in.

The two most common examples anti-CSRF tokens and locales.

Although currently these can be added to the data struct provided when rendering - these are not strictly data and it significantly complicates rendering especially when considering subtemplating.

Proposed API

I propose that the following methods are added to text/template.Template and to html/template.Template

text/template:

func (t *Template) ExecuteFuncMap(wr io.Writer, data any, funcs FuncMap) error { ... }

func (t *Template) ExecuteTemplateFuncMap(wr io.Writer, name string, data any, funcs FuncMap) error { ... }

html/template:

func (t *Template) ExecuteFuncMap(wr io.Writer, data any, funcs FuncMap) error { ... }

func (t *Template) ExecuteTemplateFuncMap(wr io.Writer, name string, data interface{}, funcs FuncMap) error {

Calling these methods would execute the template as per Execute and ExecuteTemplate with the only difference being that when a template calls a function the provided funcs template.FuncMap would be checked first. In this way funcs can be considered a map of function overrides for template execution and acts similarly to the calling the Funcs(...) method on the template but would also override functions in templates called by the current executing template.

Examples

Current Status

Let us assume we have a function with the following signature:

func (l *Locale) Tr(key string, args ...any) string { ... }

In this example this function takes a key and provides the translated value for it.

Clearly, this cannot be compiled into shared templates as the locale will be different on a per request basis.

So assuming we don't want to have multiple compiled templates per locale we will need to pass this in as data. Let's say as .Tr.

This leads to:

...
{{.Tr "key" args}}
...

But... what about when ranging over a set of items? Now, . will refer to an item in the range of items so we need use $...

{{range .Items}}
   ...
   {{$.Tr "key" .}}
   ...
{{end}}

If we forget we will get a runtime error. So we won't know that it's a problem until it's run.

But... what if we want to reuse that range block as a subtemplate or even just extract it out because it is becoming unwieldy? Ideally we'd want to do:

{{range .Items}}
   {{template "render-item" .}}
{{end}}

That of course doesn't work because we lose access to $. So now we need to do something horrible like:

{{range .Items}}
   {{template "render-item" dict "Item" . "Tr" $.Tr}} {{/* or "root" $ */}}
{{end}}

and then in our subtemplate refer to .Item and .Tr (or .root.Tr). This means that we need to now know that we're in a subtemplate and every time we call the subtemplate we need to do a similar hack.

Again if we forget to add the correct prefix we'll get a runtime error but only at time of render.

Why isn't `template.Clone()` `template.Funcs()` `template.Execute()` sufficient?

The problem here is multifactorial, Clone() isn't very lightweight and requires locks, Funcs(...) also requires locks and every template would need to be cloned and its funcs updated in case when you execute the template it calls another template.

The referenced issue has been around for over 2 years and it is likely that improving the efficiency of this route significantly is not possible without significant refactors.

Instead, the proposed API makes for a clear and simple implementation with a minimal changeset.

Proposed

The proposed API allows for much simpler templating. The Tr function would be passed in within an override FuncMap e.g.:

err := tmpl.ExecuteFuncMap(wr, data, template.FuncMap(map[string]any{"Tr": locale.Tr})

Then the downstream templates look like:

{{Tr "key" args...}}
{{range .Items}}
    {{Tr "key" .}}
    {{template "item-template" .}}
{{end}}

And the Tr function is available everywhere without having to determine whether it needs a ., a $ or even a $.root. (Or even if it's not been passed in to this subtemplate.)

Compatibility

The proposed functionality is completely backwards compatible.

Implementation

The proposal is easy to implement and has been implemented as #54432.

robpike commented 1 year ago

It isn't clear what you are proposing. Does the argument function map replace the complete set available, does it replace only those in the template, or is it just a place to look first?

You don't say what the function signatures are, either. Please edit the proposal to make it clearer.

zeripath commented 1 year ago

I hope that I've made this clearer for you. PR #54432 hopefully shows the intended API in practice.

zeripath commented 1 year ago

A further optimization in #54432 would be to only check and convert the overlay functions in to a valueFunc when the function is called instead of beforehand.

robpike commented 1 year ago

I hope that I've made this clearer for you. PR #54432 hopefully shows the intended API in practice.

Thanks, that is clear now.

The new names are cumbersome (but could be changed), while thought needs to be given to whether this is the right approach.

zeripath commented 1 year ago

I agree that they're slightly cumbersomely named but they are consistent with naming practices elsewhere in std. Another option could be to make them ExecuteWithFuncMap.

There is genuinely a need to provide funcmaps at time of execution differently from those used at parsing, and whilst such funcmaps are related - certainly the parsing funcmap constrains what types of funcs and the names of funcs - they're not the same.

This is recognised in the current API by having template.Funcs do two different things before and after parsing. This results in Funcs having to have an explicit documentation warning about this different behaviour(!). Internally they're also stored separately. The problem is this method necessarily changes the template - and there is no way to express a desire only change a single execution of a template.

In a lot of ways the naturality of execution funcmaps can be seen in how simple it is to implement. Apart from a small difficulty regarding changing the provided funcmap in to a valuemap there is almost nothing to do to implement it.

Now, how else could this be implemented?

The suggestion of Clone Funcs Execute hasn't worked in the past and isn't going to work. It's too hard to speed up and in a lot of ways it's not the right thing. We don't want to change the template - just this execution of the template.

That leaves the question of is there a way to expose execution state outside of the current functions? Well... The current API means that a template.Template is a lot of somewhat disparate things. It is not simply a single parsed template, but a collection of parsed templates, a parser and an executor all of itself. That's fine and makes for a simple API but at some point it might be sensible to actually separate those things out and expose them individually. Is this the reason to do that? I don't necessarily believe it is. (Certainly any attempt to implement this functionality without the proposed functions would at least result in partial separation out of the executor from the template.) Does this API prevent that or make that migration harder? Again I don't think so. Any separation out is going to have to expose this kind of functionality and it would map nicely on to that.

zeripath commented 1 year ago

I should add that the slight distaste in the naming of the go API here should be balanced against the substantial cumbersomeness the current API is imposing on the template API, (and the impracticality of the suggested workaround).

A codebase using multiple templates is likely to have only one or two places where ExecuteFuncMap is written - compare that to every template having to account for the lack of execution funcmaps, pretending that contextual functions are somehow data and then having to fiddle about trying to remember if it is .Tr, $.Tr, $.root.Tr or even having to go back and change every call of a template to add root in. Whilst one can think of a locale as data to be rendered it is somewhat orthogonal to it and is genuinely a context of a render. (This extends to other things but locale is the simplest case.)

The currently suggested workaround of using Clone() + Funcs() + Execute() is slow and would require cloning all of the templates needed on a per execution basis. The current structure of templates means that can never be quick because the execution funcmaps are stored within the templates themselves and these have to have a concurrency lock because templates can parse themselves. This fundamentally cannot be sped up without significant restructuring of the whole of the text/template package whereas the proposed change is easy to do.

When balancing these factors, although these method names are slightly cumbersome, it is likely that only one of these 4 methods will be used only once or twice in a codebase. Not having them is making templates incredibly cumbersome and error prone. This suggests really that they would be a net reduction in cumbersomeness.

earthboundkid commented 1 year ago

@bep, would this simplify the implementation of Hugo? Being able to call a page function in templates would be nice.

zeripath commented 1 year ago

Dear golang team have there been any further thoughts about this?

bep commented 1 year ago

@carlmjohnson I don't understand this issue very well (I have only skimmed it). In Hugo we have a fork of the execution part of Go's template packages with some small adjustments (we have a script that keep the 2 in sync); one of them is a ExecuteWithContext method, which I think make more sense re. passing contextual data. And yes, it will eventually allow you to do page.Foo once I find time to wire everything ...

zeripath commented 1 year ago

Forking the execution code is part is something I am trying to avoid...

wxiaoguang commented 1 year ago

Is this proposal still under discussion? I agree with zeripath that the customized FuncMap is really helpful for large projects.

Without this approach, I guess the developers should do:

Parse all templates
Each time, Clone a template to new one, and inject new functions (eg: with Context)
The cloned templates could be managed by an object pool, because the Clone seems heavy.

According my test, this approach is not feasible, for a site with 400 templates:

It wastes too much memory , 55M (no clone) vs 3.7G (clone)
It's quite slow, each clone takes ~ 10-20ms, the program startup time increases from nearly zero to 10s.

wxiaoguang commented 1 year ago

@robpike I also think this is the right approach, is there any more concern?

rsc commented 1 year ago

I still believe that #38114 is the path forward here. The fact that no one has had time to do it does not necessarily imply it is difficult or impossible, just low priority.

wxiaoguang commented 1 year ago

The clone idea could be more complex than the ExecFuncMap idea. I have succeeded to get the per-calll func map work by:

Without official support, there are a lot of work to do, because the sub-templates share the internal state:

Make a new html/template/Template for all templates
Add template code to the all template
Freeze the all template, reset its exec func map, it shouldn't execute any template.
When a caller wants to render a template by its name
1. Find the name in all
2. Find all its related sub templates
3. Escape all related templates (just like what the html template package does)
4. Add the escaped parse-trees of related templates into a new (scoped) text/template/Template
5. Add context-related func map into the new (scoped) text template
6. Execute the new (scoped) text template
7. To improve performance, the escaped templates are cached to template sets

rsc commented 1 year ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

bep commented 1 year ago

I have talked about this before, without seeing much enthusiasm, but I'll repeat the bullet points.

Hugo used to do a clone of the full template set for each language to be able to have language specific template functions. This was ... not great, as many have mentioned above.

Now we have a "soft fork" of Go's template packages where we have made some slight adjustments to the execution flow. The relevant part in this discussion could be illustrated with the 2 interfaces below:

// Preparer prepares the template before execution.
type Preparer interface {
    Prepare() (*Template, error)
}

// Executer executes a given template.
type Executer interface {
    ExecuteWithContext(ctx context.Context, p Preparer, wr io.Writer, data any) error
}

With the above, Executer holds a map of functions and the templates can be reused. The Prepare method above is just for the (little bit odd) t.escape() construct in the html package.

oliverpool commented 1 year ago

@bep would the proposed API help hugo get rid of the "soft fork" of Go's template? (or at least remove some customization)

I would welcome such a change to get rid of the Clone call for the csrf field.

wxiaoguang commented 1 year ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

Hello ~~ kindly ping .... @rsc is there any progress (weekly review)?

Update: if the name is somewhat "cumbersome", it could only keep the "ExecuteFuncMap" since "ExecuteTemplateFuncMap" is only a wrapper. "ExecuteFuncMap" is good enough and in most cases developers only need it.

bep commented 1 year ago

@bep would the proposed API help hugo get rid of the "soft fork" of Go's template?

@oliverpool It would be a step forward ... I can understand why the Go std library team would be a little worried about changing the API, but it should be easy to keep to old while getting a, in my eye, much more performant and clean design where the concerns of parsing and execution are clearly separated.

gopherbot commented 1 year ago

Change https://go.dev/cl/510738 mentions this issue: text/template, html/template: add ExecuteFuncs

rsc commented 1 year ago

I spent a while looking into optimizing Clone, and while I've convinced myself it can be done with significant changes, it seems that not making those changes is the better part of valor.

I also think there's no need for ExecuteTemplateFuncMap, since ExecuteTemplate is just a helper wrapper around Lookup+Execute and can be left alone.

Finally I think we can shorten ExecuteFuncMap to ExecuteFuncs, especially since the Template method is Funcs.

So this proposal shortens to just adding ExecuteFuncs in the two packages, which I've sent CL 510738. I've retitled it as such.

Please try out that CL and see if it does everything you want it to do. Thanks!

wxiaoguang commented 1 year ago

The only question in my mind is:

Is it necessary to call createValueFuncs in func (t *Template) execute() ?

The code is a hot path, checking the funcs or not, the result could be the same.

Suppose a developer passes "funcs" into ExecuteFuncs:

With createValueFuncs in func (t *Template) execute():
- It uses loops to check many things, which is slow
- ExecuteFuncs returns an error or panics if any func is not valid
Without createValueFuncs in func (t *Template) execute():
- Just add the funcs into the state.funcs
- ExecuteFuncs returns an error or panics if any func is not valid

Eventually there seems no different, while "without createValueFuncs" is more performant

rsc commented 1 year ago

Please comment on the CL for that implementation detail. I don't think it affects the proposal. Thanks!

rsc commented 1 year ago

@bep, does this look good to you? Can you try it in Hugo and see if it lets you remove one of the divergences?

bep commented 1 year ago

@rsc so, the current set of execution related methods duplicated in both text/template.Template and html/template.Template are:

Execute
ExecuteTemplate

With the addition of ExecuteFuncs we get

Execute
ExecuteTemplate
ExecuteFuncs

When you eventually decide you want to add context.Context to these, you get:

Execute
ExecuteTemplate
ExecuteFuncs
ExecuteWithContext
ExecuteTemplateWithContext
ExecuteFuncsWithContext

Or something.

I have read your recent blog articles about storing state in control flow etc., but reading the above, I would say that it would make perfect sense to introduce a new exported struct type that would be responsible to execute a given template. This new struct would hold the funcs (which could be initialized once at construction time) and you could get a API ala:

TemplateExec.Execute(TemplateInterface, Writer) error
TemplateExec.ExecuteWithContext(Context, TemplateInterface, Writer) error

Or something. This would also support most future requirements that may pop up in this area.

Merovius commented 1 year ago

@bep It seems questionable to me, that you assume there will be context.Context variants thus doubling the numbers of relevant methods. I see no reason why template-execution would take a context. It's not a network operation. And in the rare corner cases where someone does believe they need it, they can create closures over them and use those in a FuncMap passed to ExecFunc.

And even if we decided we need a native way to call context functions, I don't see why it couldn't take the form of a single func (Template) WithContext(context.Context) Template method - the FuncMap functions are called via reflect anyways. So, I see it as very unlikely that we'd get this doubling of methods you just assume. But then, lastly: Even if we would get it, so what? What's the harm of having those methods? And why would we need a TemplateExec type (which feels strongly like a Java-ism), instead of making it a package-scoped function?

So, no, I honestly don't see that.

bep commented 1 year ago

@Merovius My context.Context example is a relevant one (there's an existing proposal floating around) and I don't see how any of your proposed workarounds working/be any good (I could start a discussion about that, but that would derail this proposal).

The thing is, state doesn't vanish just because you add it as a function argument (e.g. ExecuteFuncs(funcs)):

Someone has to maintain that state, creating that func map is not a per request operation.
And as seen by the CL in question, doing costly conversion of (I presume) the funcs from interface{} to reflect.Value may end up killing the performance you tried to fix in the first place.

I feel like every proposal about improving something in any of these template packages end up with making the least amount of change to get something working, often ending up with yet another unforeseen data race etc.

And why would we need a TemplateExec type (which feels strongly like a Java-ism), instead of making it a package-scoped function?

Se my "the state doesn't vanish" argument above. I'm not sure how global state would somehow be better than a struct for this (again, see my comment about introducing unforeseen data races above).

Merovius commented 1 year ago

there's an existing proposal floating around

Do you mean #31107? Because that is closed, ultimately in favor of this one, AIUI.

I could start a discussion about that, but that would derail this proposal.

I agree, for what it's worth. That's why I said that we shouldn't complicate this proposal by assuming we'd also add context.Context at some point. Like, even if you think we should, taking it as a given that we will seems very questionable.

bep commented 1 year ago

I agree, for what it's worth. That's why I said that we shouldn't complicate this proposal by assuming we'd also add context.Context at some point

Sure, but we should take into account that for every similar API in Go's stdlib (that does "per request operations", e.g. http.Client) will eventually need to be expanded/adjusted to fit new requirements/fix bugs, so going in a direction where this gets harder and harder to do, is not ideal. Which I still think my contex.Context serves good as an example (also, a closed issue is not the same as "it will. never happen").

earthboundkid commented 1 year ago

Background: One of the things Hugo can do is to make an HTTP requests in a template. It wouldn’t make sense in a normal HTTP server app, but it makes sense for an application where the user controls templates but not the handlers. If a user makes a file change and then another one, the first request needs to be cancelled.

rsc commented 11 months ago

If I were going to do ExecuteContext (big if!) I would make that one new function take both the context and the funcs.

rsc commented 11 months ago

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

rsc commented 11 months ago

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

golang / go

text/template, html/template: add ExecuteFuncs #54450

Summary

Detailed rationale

Proposed API

Examples

Current Status

Why isn't `template.Clone()` `template.Funcs()` `template.Execute()` sufficient?

Proposed

Compatibility

Implementation

golang / go

text/template, html/template: add ExecuteFuncs #54450

Summary

Detailed rationale

Proposed API

Examples

Current Status

Why isn't template.Clone() template.Funcs() template.Execute() sufficient?

Proposed

Compatibility

Implementation

Why isn't `template.Clone()` `template.Funcs()` `template.Execute()` sufficient?