Change in error raising behaviour

[denny]

Hi,

I'm seeing a change in error-raising behaviour between 4.4.0 and 4.4.1, and looking at the code I think it may be a bug in the Parser.run method - specifically where it decides whether to raise an exception for an invalid MJML input file:
063152c#diff-5c149cf160539fd426925ca3b65fc5dafb2a1c9d5ae56df3212543e112d242fdR40

In every case that I've tested (three or four, not loads), status.success? is returning true when the MJML template is invalid, where stderr contains content and stderr.eof? returned false with the previous code. This means that instead of a ParseError being raised, an empty string is always rendered and returned - regardless of the setting of Mjml.raise_render_exception.

Previously I was catching the ParseError in a validator in my Rails app when people attached an MJML template to a database record - so with 4.4.1, my validator passes everything and my CMS allows items to be saved with invalid syntax in their related MJML files.

I'll post some more info below, let me know if you need something specific that I haven't included. Also if there's a better way I should be checking for valid MJML syntax in an ActiveRecord validator :)

Cheers,
Denny

denny@rocinante:~/ShinyCMS$ rspec ./plugins/ShinyNewsletters/spec/models/shiny_newsletters/template_spec.rb:46
ShinyNewsletters::Template
  validations
    mjml_syntax

From: /home/denny/.rvm/gems/ruby-2.7.2/gems/mjml-rails-4.4.0/lib/mjml/parser.rb:40 Mjml::Parser#run:
    38:     command = "#{mjml_bin} -r #{in_tmp_file} -o #{out_tmp_file.path} --config.beautify #{beautify} --config.minify #{minify} --config.validationLevel #{validation_level}"
    39:     _, _, stderr, _ = Open3.popen3(command)
 => 40:     binding.pry
    41:     raise ParseError.new(stderr.read.chomp) unless stderr.eof?

[1] pry(#<Mjml::Parser>)> stderr.eof?
false
[2] pry(#<Mjml::Parser>)> stderr.read(130)
"Line 4 of /tmp/in20201126-2467728-h1e7c3.mjml (mj-title) \xE2\x80\x94 mj-title cannot be used inside mj-column, only inside: mj-attributes,"
[3] pry(#<Mjml::Parser>)> 
      fails to create a new Template if the template file is not valid MJML

Finished in 17.64 seconds (files took 3.1 seconds to load)
1 example, 0 failures

denny@rocinante:~/ShinyCMS$ rspec ./plugins/ShinyNewsletters/spec/models/shiny_newsletters/template_spec.rb:46
ShinyNewsletters::Template
  validations
    mjml_syntax

From: /home/denny/.rvm/gems/ruby-2.7.2/gems/mjml-rails-4.4.1/lib/mjml/parser.rb:40 Mjml::Parser#run:
    38:     command = "-r #{in_tmp_file} -o #{out_tmp_file.path} --config.beautify #{beautify} --config.minify #{minify} --config.validationLevel #{validation_level}"
    39:     _, stderr, status = Mjml.run_mjml(command)
 => 40:     binding.pry
    41:     raise ParseError.new(stderr.chomp) unless status.success?

[1] pry(#<Mjml::Parser>)> stderr
"Line 4 of /tmp/in20201126-2468496-1yrhn3n.mjml (mj-title) — mj-title cannot be used inside mj-column, only inside: mj-attributes, mj-head\n"
[2] pry(#<Mjml::Parser>)> status.success?
true
[3] pry(#<Mjml::Parser>)> 
      fails to create a new Template if the template file is not valid MJML (FAILED - 1)

Failures:
  1) ShinyNewsletters::Template validations mjml_syntax fails to create a new Template if the template file is not valid MJML
     Failure/Error: expect( update_successful ).to be false
            expected false
            got true
     # ./plugins/ShinyNewsletters/spec/models/shiny_newsletters/template_spec.rb:51:in `block (4 levels) in <module:ShinyNewsletters>'

Finished in 13.08 seconds (files took 3.1 seconds to load)
1 example, 1 failure

(I've edited the above runs for length, mostly by removing the first pass each takes through the MJML parser, which I assume is the layout.)

[denny]

Here's a minimal change (just the condition on line 40) that makes my test pass... but it doesn't seem likely to be the best solution:

denny@rocinante:~/ShinyCMS$ rspec ./plugins/ShinyNewsletters/spec/models/shiny_newsletters/template_spec.rb:46
ShinyNewsletters::Template
  validations
    mjml_syntax

From: /home/denny/.rvm/gems/ruby-2.7.2/gems/mjml-rails-4.4.1/lib/mjml/parser.rb:40 Mjml::Parser#run:
    38:     command = "-r #{in_tmp_file} -o #{out_tmp_file.path} --config.beautify #{beautify} --config.minify #{minify} --config.validationLevel #{validation_level}"
    39:     _, stderr, status = Mjml.run_mjml(command)
 => 40:     binding.pry
    41:     raise ParseError.new(stderr.chomp) unless stderr.blank? # status.success?

[1] pry(#<Mjml::Parser>)> status.success?
true
[2] pry(#<Mjml::Parser>)> stderr.blank?
false
[3] pry(#<Mjml::Parser>)> stderr
"Line 4 of /tmp/in20201126-2469595-nhp775.mjml (mj-title) — mj-title cannot be used inside mj-column, only inside: mj-attributes, mj-head\n"
[4] pry(#<Mjml::Parser>)> 
      fails to create a new Template if the template file is not valid MJML

Finished in 32.81 seconds (files took 3.1 seconds to load)
1 example, 0 failures

[sighmon]

@doits status.success? seems to be returning true for MJML template syntax errors.

I'll push the fix @denny mentions checking for stderr.blank? (thanks for the thorough bug report).

@doits Was there a reason you chose Open3.capture3 over Open3.popen3 and the change in error checking to status.success??

[doits]

Was there a reason you chose Open3.capture3 over Open3.popen3 and the change in error checking to status.success??

Actually I was under the impression that mjml will return a correct error code when compilation fails, but I didn't check it – sorry for breaking it! I created #71 with a test for it so it won't break next time.

[doits]

I took a dive again and made another PR #72 with some more explanation.

[sighmon]

@denny How does version 4.5.0 look to you? Does that work for your workflow?

[chasegiunta]

@sighmon @doits @denny So, the change between 4.4.1 & 4.4.2 (and subsequent 4.5.0 & 4.6.0 releases) has caused templates with mjml errors to not render at all and fail silently if:

Mjml.setup do |config|
   config.raise_render_exception = false
end

Previously, in 4.4.1, you could still render a template with a validation error, and suppress the exception(s) with the config above. With 4.4.2, while errors are apparent if config.raise_render_exception = true, absolutely nothing will render if that setting is set to false, and no errors will be output.

Been driving me mad for the past day trying to figure this out, so let me know if there's anything I can do to help fix this. Otherwise, the config.raise_render_exception = false is not applicable currently.

[doits]

Yes, to solve your issue only set config.validation_level = "soft" if you want to render templates with errors. This tells MJML-Rails to not raise an exception if only the template is invalid.

Normally you do not want to render malformed templates, that is why it raises an exception by default. You shouldn't change config.raise_render_exception for your use case. With validation level soft exceptions are only raised if something really bad happens, not if the template is malformed, and therefore you shouldn't ignore exceptions if you depend on MJML – because config.raise_render_exception = false is exactly the setting to tell MJML-Rails to fail silently.

I'm not sure though that it was possible to render invalid templates in version 4.4.0 at all, because MJML-Rails checked standard error output to decide whether to raise an exception or not. So if there was any error in the template it should have raised an exception there and ignoring it always returned an empty string from my understanding.

Maybe you did update from a version before 4.4.0 (4.4.0 added validation level) or update MJML at the same time?

[doits]

Actually I think you are right – only in version 4.4.1 it did render invalid templates, which was a bug though (and it had nothing to do with raise_render_exception = false). 4.4.1 was released only for a short time (I think a day?) and I'm sorry if you hit this and based your assumptions/config on this.

[chasegiunta]

@doits That makes sense. Thanks for the thorough explanation 👍