Fix xml parsing of open_search.xml #267
Conversation
open_search.xml file was being served using html/template package and since it doesn't know about xml contexts, it changes the first character of xml template "<" into "<". Now, we use two different packages: - html/template - for parsing html template files - text/template - for parsing text or xml template files Here it's arguable that we could use encoding/xml package to parse the xml template files in a better way. However, to keep the parsing logic consistent, we are fine with using text/template only. If, in future, we come across a use case where we want to parse and serve xml file where we need to know the xml context while parsing the template, we may think of switching to encoding/xml at the cost of inconsistency. There's no need as of now to use it, because it's a very simple xml file.
sahildua2305
force-pushed
the
fix-xml-parsing
branch
from
644689d
to
2dd7862
Compare
ui/content.go
Outdated
|
|
||
| // This is only created in prd-mode, the pre-parsed template | ||
| // Text template - currently used for /open_search.xml only | ||
| ttpl *ttemplate.Template |
There was a problem hiding this comment.
Seems like you should be able to better take advantage of the fact that html/template.Template.Execute and text/template.Template.Execute are structurally equivalent. You should be able to store both types into an interface field.
type renderer interface {
Execute(w io.Writer, data interface{}) error
}
That would at least clean up one set of switch statements for dispatching.
There was a problem hiding this comment.
Thanks @kellegous, I was thinking of using an interface in this case, but wasn't able to figure out how to use one. I'll try again and push the fixes.
ui/ui.go
Outdated
| // See - https://github.com/etsy/hound/issues/239 | ||
| if c.tplType == "xml" || c.tplType == "text" { | ||
| return c.ttpl.Execute(w, map[string]interface{}{ | ||
| "Host": r.Host, |
There was a problem hiding this comment.
Is there any reason not to pass the same data map to all templates? If not, seems like consistency would help us avoid surprises in future additions.
There was a problem hiding this comment.
No reason as such. However, you're right. Inconsistency may hurt in future additions. I'll fix that.
sahildua2305
force-pushed
the
fix-xml-parsing
branch
from
f249a78
to
bed54bc
Compare
|
@kellegous I just pushed the changes you suggested and the code is much simpler now. Thanks for suggestions! 🙂 |
Use interface renderer which acts as a field in content struct corresponding to html/template or text/template object depending on the tplType field. This should simplify the entire logic of Execute and hence eliminating the need of an extra switch statement.
sahildua2305
force-pushed
the
fix-xml-parsing
branch
from
bed54bc
to
1d878bb
Compare
ui/ui.go
Outdated
| "fmt" | ||
| "html/template" | ||
| htemplate "html/template" |
There was a problem hiding this comment.
Rename these to html_template and text_template
There was a problem hiding this comment.
Good one! Will do.
| cnt.tpl, err = template.New(cnt.template).Parse(string(a)) | ||
| if err != nil { | ||
| return nil, err | ||
| // For more context, see: https://github.com/etsy/hound/issues/239 |
There was a problem hiding this comment.
Dry this bit up into a function :)
There was a problem hiding this comment.
It's actually different for prod and dev.
There was a problem hiding this comment.
In prod, we load the contents of template from bindata, however, in dev mode, we load the contents of template directly from the file.
Change import names of template modules from htemplate and ttemplate to html_template and text_template to make it more explicit.
|
Thanks @sahildua2305! |
|
Thanks @stanistan and @kellegous :) |
This turned out to be more cumbersome than I had expected it to be. Below
is the description of what I did and the rationale behind it.
open_search.xml file was being served using html/template package and
since it doesn't know about xml contexts, it changes the first character
of xml template "<" into "<".
Now, we use two different packages:
Here it's arguable that we could use encoding/xml package to parse the
xml template files in a better way. However, to keep the parsing logic
consistent, we are fine with using text/template only. If, in future, we
come across a use case where we want to parse and serve xml file where
we need to know the xml context while parsing the template, we may think
of switching to encoding/xml at the cost of inconsistency. There's no
need as of now to use it, because it's a very simple xml file.
Fixes #239
/cc @stanistan