Allow accessing struct fields using lower case names

[zfLQ2qx2]

@annismckenzie Curious your thoughts about this - internally my structs have a lot of string, int, *bool, etc. where the values can be nil if not set explicitly. In Jet if a structure has a field and its value is nil then it appears to be the same as it not existing -- https://github.com/CloudyKit/jet/blob/master/eval.go#L1111

From a templating perspective I'd think they would be treated as safe values such as "", 0, false, etc. if it exists unless I'm explicitly comparing them with nil with isset. If I made a change to differentiate between a field that doesn't exist and a field that does exist but it set to nil, is that a change you would be interested in?

I was also thinking that using field tags to have a preferred name for struct fields (like encoding/json does) so that its not so obvious that Golang is involved in rendering the template or to obfuscate where the value might be coming from. It looks like there is just couple places in eval.go where a change would be needed to implement that. Would you be interested in that change if I got it working?

[sauerbraten]

If I made a change to differentiate between a field that doesn't exist and a field that does exist but it set to nil, is that a change you would be interested in?

In Go, struct fields can't "not exist". All fields in your struct exist and contain their type's zero value, but since you declared them to be pointer types, their zero value is nil until you explicitly set them to a different value: https://golang.org/ref/spec#Pointer_types

Do you mean differentiating between an *int pointing to nil and an *int pointing to a 0? Since both are valid values for an *int, the code you linked to will treat them the same, yes. You don't have to use isset() though, if you don't want: you can just compare them to nil with x == nil.

[zfLQ2qx2]

@sauerbraten Yes, but it appears as if Jet explicitly gives an "does not exist in this scope" if the value is nil. If its not doing that then let me know and maybe there is a mistake on my part I'm overlooking.

If that is the behavior then it looks like https://github.com/CloudyKit/jet/blob/11c8098840bc265e9f1d6f1844bb674086d4e242/eval.go#L270 is actually where this is happening because this is the only reference to reflect.FieldByName I see. So question is if we really want nil fields to not be in the scope (in which case I'll do IsSet()s or if we want to tweak things a little so that nil is possible.

[zfLQ2qx2]

So my test case is:

vars.Set("x_tag", nil)

And then in my template I do:

{{if x_tag}}

Then I get the error:

main.go:262: Jet Runtime Error ("x.jet":19): identifier "x_tag" is not available in the current scope map

It looks like it should be accepted here: https://github.com/CloudyKit/jet/blob/master/template.go#L367

eval.go line 1102 is definitely where the error message is being emitted.

It looks like x_tag is in the scope, but it has an invalid value which is why it can't be found.

It is definitely becoming invalid in template.go line 367. So question is how do I correctly detect and add a nil value.

I'm not very experienced with the reflect package but this works, so the issue seems to be a lack of type: vars.Set("x_tag", (*string)(nil))

Doing some Googling it looks like this a common issue when you start to mess with reflect, but I don't see a really good way to resolve it beyond what I did above.

[tooolbox]

internally my structs have a lot of string, int, *bool, etc. where the values can be nil if not set explicitly. In Jet if a structure has a field and its value is nil then it appears to be the same as it not existing

This is different from the example that you provide, isn't it? You're setting an untyped nil directly on the varmap. Should be able to do data.field == nil for struct pointer fields.

My opinion based on data to hand is that if you want to set untyped nils on the varmap and then check them, this is an odd use case, and you should just do vars.Set("x_tag", (*string)(nil)) or check isset().

I was also thinking that using field tags to have a preferred name for struct fields (like encoding/json does) so that its not so obvious that Golang is involved in rendering the template or to obfuscate where the value might be coming from.

Can you give a concrete example of why this is valuable?

[zfLQ2qx2]

@tooolbox There are two places where I think that being able to set the field names would be useful 1) Method and variables in title/pascal case might tip off an attacker that golang is the underlying system, using all lower case or camel case would be more consistent with javascript styles 2) the consistent use of lowercase is known to give a slight bump in compressibility because so much of html/javascript/css tends to also be written in lower case.

[sauerbraten]

1. I think you have a different problem when someone can read your template source files... They are rendered on the server so if the attacker can read them, that means he already broke into your server. When he sees *.jet files, he'll know it uses Jet, a Go templating engine, anyway.

2. Your template variable names should never occur next to your final (pre-compression) HTML/JS/CSS. That's the point of templating: they are gone after Jet executed the template.

[zfLQ2qx2]

@sauerbraten I'm thinking more in terms of giving third parties the ability to modify the templates - but without necessarily being aware of the underlying implementation. Right now this looks like a subset of handlebars so nodejs would probably be the assumption.

[tooolbox]

@sauerbraten took the words out of my mouth.

I'm thinking more in terms of giving third parties the ability to modify the templates - but without necessarily being aware of the underlying implementation. Right now this looks like a subset of handlebars so nodejs would probably be the assumption.

Sounds pretty wild.

Thoughts:

1. If you're giving a third party enough access to modify Jet templates, they can probably find out it's Jet even if your variable names aren't title case.
2. Knowing a system is run by Go is not as much of a liability as, say, knowing a system is run by PHP. The attack surface is just much smaller IMO.
3. If you're truly worried about 3rd party template authors using struct field names as-is, suggested workaround is to throw all the data into a map with whatever names you want.

[sauerbraten]

I'm inclined to say protecting against the attack vectors you describe (where an attacker has write access to your templates) is out of scope for Jet itself and you would have to address them in a layer you build around it, i.e. custom template validation. You could enforce only lowercase field names in incoming templates there and rewrite them to Title case that Jet & Go require.

I renamed the issue to be about the lower case field access and will close it. We can discuss the "not in scope" vs. "set but nil" handling of nil pointer fields in a new issue if you want.

[zfLQ2qx2]

@sauerbraten Turns out to be a fairly simple change, this will use the "json" name over the go field name if specified. Could just as easily be tagged "jet" or something custom.

diff --git a/eval.go b/eval.go
index 0845580..7894244 100644
--- a/eval.go
+++ b/eval.go
@@ -20,6 +20,7 @@ import (
        "reflect"
        "runtime"
        "strconv"
+       "strings"
        "sync"

        "github.com/CloudyKit/fastprinter"
@@ -1549,7 +1550,16 @@ func buildCache(typ reflect.Type, cache map[string][]int, parent []int) {
                                buildCache(typ, cache, index)
                        }
                }
-               cache[field.Name] = index
+               if jsonTag := field.Tag.Get("json"); jsonTag != "" && jsonTag != "-" {
+                       // check for possible comma as in "...,omitempty"
+                       var commaIdx int
+                       if commaIdx = strings.Index(jsonTag, ","); commaIdx < 0 {
+                               commaIdx = len(jsonTag)
+                       }
+                       cache[jsonTag[:commaIdx]] = index
+               } else {
+                       cache[field.Name] = index
+               }
        }
 }

[sauerbraten]

That's not why I and @tooolbox are against this "feature". It's about keeping things simple.

The feature you suggest could in fact be confusing to users. For example:

type User struct {
    Name string `json:-`
    APIName string `json:"Name"`
}

Imagine I have the above struct as part of my data model, and want to output lists of users on a website and via a JSON API. I use the JSON tag to make sure the JSON response gets the APIName as the user's name, and I use {{ .Name }} in the Jet template to render the user's full name when rendering an HTML response. With your change, the user has no way to get the actual Name field from his User struct in his Jet templates.

The behavior you suggest would have to be documented and would still be something users have to watch out for while giving them very little benefit (in most cases, none).

We should be aiming to make Jet's behavior more predictable, unsurprising and intuitive. Sorry, but this change would do the opposite.