Issue 29327966: Issue 3084 - [cms] Show full tracebacks for exceptions passing template code (Closed)
|
Created:
Sept. 15, 2015, 5:37 p.m. by Sebastian Noack Modified:
Sept. 17, 2015, 9:30 p.m. Visibility:
Public. |
Patch Set 1 #
Total comments: 6
Patch Set 2 : Pass through filename and use execfile() when loading filters/globals #
Total comments: 11
Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Unpack converter source before calling get_html() #
MessagesTotal messages: 12
https://codereview.adblockplus.org/29327966/diff/29327967/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29327967/cms/converters.py#n... cms/converters.py:125: for i, line in enumerate(lines): In order to prevent line numbers from being off we replace lines setting parameters with "{#\n#}" rather than removing them in jinja2 templates. https://codereview.adblockplus.org/29327966/diff/29327967/cms/converters.py#n... cms/converters.py:253: self._params["include"] = name There seems to be no way to get the filename for includes currently. https://codereview.adblockplus.org/29327966/diff/29327967/cms/converters.py#n... cms/converters.py:358: source = self._params["source"] And here is where the problematic part starts. I don't really want to have that logic. https://codereview.adblockplus.org/29327966/diff/29327967/cms/converters.py#n... cms/converters.py:369: code = env.compile(source, None, self.get_template_filename()) There are two ways to make the template aware of the filename, either calling compile() directly passing the filename, as we do here. Or using a loader that is aware of the filename and using get_template(). https://codereview.adblockplus.org/29327966/diff/29327967/cms/converters.py#n... cms/converters.py:375: env.handle_exception() Note that this is necessary because as opposed to make_module() doesn't call handle_exception(). Hence the line number refers to the line in the generated Python code, not the jinja source code, without calling handle_exception(). https://codereview.adblockplus.org/29327966/diff/29327967/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29327966/diff/29327967/cms/sources.py#newc... cms/sources.py:314: return SuffixTemplateLoader(jinja2.FileSystemLoader(self._dir)) We need a loader that is aware of the filename for templates loaded via {% include %} and {% import %}. Theoretically we could also use Environment.get_template() now, but this doesn't seem to workout with the architecture of the CMS .
IMHO, whoever is calling the converter should set the pagefile parameter in addition to pagedata (similarly, templatefile and includefile). Instead of a single "key" parameter, the converter should have two parameters: datakey and filenamekey. So the callers already passing in the key parameter explicitly would now pass datakey and filenamekey. This would make sure that the file name is set by the same code that is also reading the file (meaning that it wouldn't get out of sync).
Alternatively, params["pagedata"] could be a dictionary instead of a string:
{"filename": "foo.md", "contents": "Hello, World!"}.
New patch set is up. Indeed, passing through the filename from Source.read_file() all the way up to Converter.get_html() turned out being not as complicated as I thought. I went for using a tuple. Also it turned out that I have to use execfile() when loading template filters and globals in order to get a proper traceback for exceptions raised there. The patch can be considered no longer a proof of concept, but is IMO ready to be merged, after another review round of course. I will file an issue shortly.
https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py#n... cms/converters.py:131: lines[i] = self.removed_line Why not just assign "\n" here? I don't think that the few extra newlines are a big deal, and you could drop the removed_line attribute then. https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py#n... cms/converters.py:335: return result + (None,) I'm not really happy with unrelated changes being mixed in here. And in this particular case, I really don't think that this should be a public class - it's an implementation detail of the TemplateConverter class. https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py#n... cms/converters.py:376: template = env.template_class.from_code(env, code, env.globals) From what I can tell, Environment.template_class is undocumented - and it is merely a constant anyway. So I'd suggest using jinja2.Template.from_code() directly here. https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py#newc... cms/sources.py:232: return (data, None) How about returning "%s!%s" % (self._name, filename) instead of None? This will give *some* context in production instead of none whatsoever. https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py#newc... cms/sources.py:238: return namespace I looked into this and this change appears to be unnecessary - a simple tweak to the existing import_symbol method should do: source, filename = self.read_file(filename) code = compile(source, filename, "exec") namespace = {} exec code in namespace
https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py#n... cms/converters.py:131: lines[i] = self.removed_line On 2015/09/16 17:35:52, Wladimir Palant wrote: > Why not just assign "\n" here? I don't think that the few extra newlines are a > big deal, and you could drop the removed_line attribute then. Done. https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py#n... cms/converters.py:335: return result + (None,) On 2015/09/16 17:35:53, Wladimir Palant wrote: > I'm not really happy with unrelated changes being mixed in here. And in this > particular case, I really don't think that this should be a public class - it's > an implementation detail of the TemplateConverter class. The unrelated change wasn't intentional. As you can see in the previous patch set, I removed the template loader here, and added it back again with this patch set. However, frankly, I don't like these nested classes. IMO they make the code harder to read. And quoting the Zen of Python: "flat is better than nested". So I'm inclined to leave it like that, unless you insist. But note that the AttributeParser class for example is also defined at the top-level. https://codereview.adblockplus.org/29327966/diff/29327997/cms/converters.py#n... cms/converters.py:376: template = env.template_class.from_code(env, code, env.globals) On 2015/09/16 17:35:52, Wladimir Palant wrote: > From what I can tell, Environment.template_class is undocumented - and it is > merely a constant anyway. So I'd suggest using jinja2.Template.from_code() > directly here. Done. https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py#newc... cms/sources.py:232: return (data, None) On 2015/09/16 17:35:53, Wladimir Palant wrote: > How about returning "%s!%s" % (self._name, filename) instead of None? This will > give *some* context in production instead of none whatsoever. Not sure if I like the notation with the exclamation mark, but I don't have a better idea. Done. https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py#newc... cms/sources.py:238: return namespace On 2015/09/16 17:35:53, Wladimir Palant wrote: > I looked into this and this change appears to be unnecessary - a simple tweak to > the existing import_symbol method should do: > > source, filename = self.read_file(filename) > code = compile(source, filename, "exec") > namespace = {} > exec code in namespace Nice catch. Done.
LGTM https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29327966/diff/29327997/cms/sources.py#newc... cms/sources.py:232: return (data, None) On 2015/09/16 19:04:04, Sebastian Noack wrote: > Not sure if I like the notation with the exclamation mark, but I don't have a > better idea. Done. For reference, this notation is inspired by the JAR protocol (http://docs.oracle.com/cd/E19253-01/819-0913/author/jar.html#jarprotocol).
Message was sent while issue was closed.
(Just being nosy in the hope to learn something, I realise this has already been pushed.) https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#n... cms/converters.py:284: def get_html(self, (source, filename)): Any reason we don't just have source and filename as separate parameters here? (Below too.) https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#n... cms/converters.py:373: template = jinja2.Template.from_code(env, code, env.globals) I was trying to look up jinja2.Template.from_code to get more context about what's happening here but I had no luck. Where can I find it?
Message was sent while issue was closed.
https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#n... cms/converters.py:284: def get_html(self, (source, filename)): On 2015/09/17 08:41:17, kzar wrote: > Any reason we don't just have source and filename as separate parameters here? > (Below too.) Indeed, we could do the unpacking in the calling code like |self.get_html(*self._params[self._key])|. I think this would actual be nicer. As this patch already landed, feel free to change it yourself. https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#n... cms/converters.py:373: template = jinja2.Template.from_code(env, code, env.globals) On 2015/09/17 08:41:17, kzar wrote: > I was trying to look up jinja2.Template.from_code to get more context about > what's happening here but I had no luck. Where can I find it? Indeed, it's not documented, at least not in the online docs. I wasn't ware of that. But honestly, I think it's better to rely on these one undocumented method, rather than having incomplete tracebacks. And to answer your question, you find here in the source code: https://github.com/mitsuhiko/jinja2/blob/2.8/jinja2/environment.py#L929
https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#n... cms/converters.py:284: def get_html(self, (source, filename)): On 2015/09/17 08:49:59, Sebastian Noack wrote: > On 2015/09/17 08:41:17, kzar wrote: > > Any reason we don't just have source and filename as separate parameters here? > > (Below too.) > > Indeed, we could do the unpacking in the calling code like > |self.get_html(*self._params[self._key])|. I think this would actual be nicer. > As this patch already landed, feel free to change it yourself. This bothered me. So I uploaded another patch set.
LGTM
Message was sent while issue was closed.
LGTM again https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29327966/diff/29328088/cms/converters.py#n... cms/converters.py:373: template = jinja2.Template.from_code(env, code, env.globals) On 2015/09/17 08:49:59, Sebastian Noack wrote: > Indeed, it's not documented, at least not in the online docs. It's documented under http://code.nabla.net/doc/jinja2/api/jinja2/environment/jinja2.environment.Te... but apparently that's not the official documentation. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
