column property from error messages is non-enumerable in 4.0.6 with noEscape: true (breaking regression)

[nathanboktae]

We had a test that tried to compile this simple template and verify the error result:

SELECT sum(gold_balance) FROM {{#if}}_p_user__gold_balance{{/foo}}

{
  "column": 33,
  "lineNumber": 1,
  "message": "if doesn't match foo - 1:33",
  "name": "Error"
}

but it started failing on 4.0.6. Rolling back to 4.0.5 solves it.

~/work/api-server 23bc2a8|merge-dashboard-queries  ✹
 9:12AM 1 ᐅ npm install
[email protected] /Users/nblack/work/api-server
└─┬ [email protected]
  └── [email protected]

~/work/api-server 23bc2a8|merge-dashboard-queries  ✹
 9:13AM ᐅ TZ=Etc/UTC mocha -g "templated query fails"

  Queries
    Dashboard Queries
      1) should return 400 if the templated query fails to compile


  0 passing (254ms)
  1 failing

  1) Queries Dashboard Queries should return 400 if the templated query fails to compile:

      Error: expected { errors:
   { query:
      { column: 33,
        lineNumber: 1,
        message: 'if doesn\'t match foo - 1:33',
        name: 'Error' } } } response body, got { errors:
   { query:
      { lineNumber: 1,
        message: 'if doesn\'t match foo - 1:33',
        name: 'Error' } } }
      + expected - actual

       {
         "errors": {
           "query": {
      +      "column": 33
             "lineNumber": 1
             "message": "if doesn't match foo - 1:33"
             "name": "Error"
           }

      at endReadableNT (_stream_readable.js:926:12)
      at _combinedTickCallback (internal/process/next_tick.js:74:11)
      at process._tickDomainCallback (internal/process/next_tick.js:122:9)




~/work/api-server 23bc2a8|merge-dashboard-queries  ✹
 9:13AM 1 ᐅ npm install
[email protected] /Users/nblack/work/api-server
└── [email protected]

~/work/api-server 23bc2a8|merge-dashboard-queries  ✹
 9:14AM ᐅ TZ=Etc/UTC mocha -g "templated query fails"

  Queries
    Dashboard Queries
      ✓ should return 400 if the templated query fails to compile


  1 passing (212ms)

[nathanboktae]

I know that... Read the subject like one more time. I'm talking about error message shape. The column of the error is missing.

[nknapp]

The part I don't fully understand right now is the following. I have done tests with 4.0.5 and 4.0.6 and the following program:

try {
    require('handlebars').compile('abc{{#abd}}aasds{{/ab}}')({abc: 'test'});
} catch (e) {
    console.log('1) ', JSON.parse(JSON.stringify(e)))
    console.log('2) ', {lineNumber: e.lineNumber, column: e.column});
}

The column-properties is missing in the stringified output of the exception in 4.0.6, but it is there when accessed directly

Output with [email protected]

1)  { lineNumber: 1,  message: 'abd doesn\'t match ab - 1:6',  name: 'Error',  column: 6 }
2)  { lineNumber: 1, column: 6 }

Output with [email protected]

1)  { lineNumber: 1,message: 'abd doesn\'t match ab - 1:6',   name: 'Error' }
2)  { lineNumber: 1, column: 6 }

[nathanboktae]

Ah yes, I tried it and see your results @nknapp, but I am using the noEscape option and it repros with that:

try {
    require('handlebars').compile('abc{{#abd}}aasds{{/ab}}', { noEscape: true })({abc: 'test'});
} catch (e) {
    console.log('1) ', JSON.parse(JSON.stringify(e)))
    console.log('2) ', {lineNumber: e.lineNumber, column: e.column});
}

Output with [email protected]

1)  { lineNumber: 1,
  message: 'abd doesn\'t match ab - 1:6',
  name: 'Error',
  column: 6 }
2)  { lineNumber: 1, column: 6 }

Output with [email protected]

1)  { lineNumber: 1,
  message: 'abd doesn\'t match ab - 1:6',
  name: 'Error' }
2)  { lineNumber: 1, column: 6 }

Somehow that property is not enumerable:

4.0.5

{ value: 6, writable: true, enumerable: true, configurable: true }

4.0.6

{ value: 6, writable: false, enumerable: false, configurable: false }

[nknapp]

I believe this is due to 20c965c. I have updated #1285 to make the column-property enumerable. It would be interesting to know, if this really solves your problem.

[nathanboktae]

if this really solves your problem

Oh yes it does. Object.defineProperty defaults enumerable to false, so it doesn't get JSON serialized out. (I tested it as well).

Thanks for the timely response!

[nknapp]

@kpdecker Can we somehow ensure that #1285 does not reintroduce the bug you fixed with 20c965c? If this is safe, I would like to see #1285 being merged (squashed?). It should then probably go into the master branch as well. Cherry-pick or merge? Am 22. Dezember 2016 00:09:23 MEZ, schrieb Nathan Black <[email protected]>:

…

> if this really solves your problem Oh yes it does. `Object.defineProperty` defaults enumerable to false, so it doesn't get JSON serialized out. (I tested it as well). Thanks for the timely response! -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1284 (comment)

-- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

[kpdecker]

@nknapp odds are I never reporduced that myself, just applied the bug fix referenced in the commit message and hoped that reduced the occurrences. Bugs like that, all you can really do is release it and hope it doesn't fuck up. (You'll find that with Handlebars, someone will be pissed for some reason for every change, so embrace it)

[nathanboktae]

Bugs like that, all you can really do is release it and hope it doesn't fuck up. (You'll find that with Handlebars, someone will be pissed for some reason for every change, so embrace it)

Uh the big stems from not knowing how Object.definePropery and ES5 properties work. Basic understanding of JavaScript IMO. Not pointing fingers, I understand the hard work it takes to maintain open source JavaScript as I do myself, but that isn't the right attitude to take here. Anyways thank you looking g forward to the fix.

[kpdecker]

@nathanboktae the concern was that the change would cause a browser crasher, which any amount of knowledge of javascript isn't going to help as they don't really spec out "the browser will crash at this point"

It's comments like these highlight why I'm no longer maintaining this project. After years of dealing with open source users at this large of a scale, for free mind you, I no longer am inclined to do the dirty work. If you don't like my attitude at this point, I suggest you invest your time in another project or spend timing maintaining the issues here.

[nknapp]

The fix is merged into the 4.x-branch and should be published with 4.0.7.
@wycats What is the preferred approach for committing those changes to the master branch? Cherry-pick? Or should I merge 4.x back into master?

[nathanboktae]

closing since this is merged in. Thanks!