fix

[GiuseppeCM]

@boutell this will fix definitely the issue

[GiuseppeCM]

this issue https://github.com/apostrophecms/apostrophe-elasticsearch/pull/8 @boutell

[boutell]

A string will never be == null. Therefore the filter is never set. I don't think this fix makes sense.

Are you aware that I already merged and published the previous PR? Is that not working for you as written?

[GiuseppeCM]

@boutell A string will never be null but it could be undefined. In the previous PR, the condition !s check if the string does not exist, so !s is true if s == undefined, and in the next condition, !s.length return a typeError. In this PR, the condition is s == null, undefined == null is true, so with the first condition, is ok. For my opinion in this PR, I was wrong anyway, because the function must enter in if, only in two cases, if the string is undefined or the string is empty, could this condition be right? if(s == undefined || s == '') sorry for the previous PR, but this, for my opinion is right

[boutell]

Oh man, this is getting silly (:

OK, I wrote a new check that is very unambiguous, and I wrote a simple test program to verify that only things with a nonempty string representation can get past:

const values = [ null, undefined, false, true, 0, '0', 1, '1', '' ];

for (const s of values) {
  if ((s === null) || (s === undefined) || (s === '')) {
    console.log('Do NOT set for: ' + require('util').inspect(s));
  } else {
    console.log('DO SET for: ' + require('util').inspect(s) + ' which
stringifies to ' + require('util').inspect(s.toString()));
  }
}

I will ship this check.

On Mon, Mar 18, 2019 at 9:56 AM GiuseppeCM notifications@github.com wrote:

@boutell https://github.com/boutell A string will never be null but it could be undefined. In the previous PR, the condition !s check if the string does not exist, so !s is true if s == undefined, and in the next condition, !s.length return a typeError. In this PR, the condition is s == null, undefined == null is true, so with the first condition, is ok. For my opinion in this PR, I was wrong anyway, because the function must enter in if, only in two cases, if the string is undefined or the string is empty, could this condition be right? if(s == undefined || s == '') sorry for the previous PR, but this, for my opinion is right

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-elasticsearch/pull/9#issuecomment-473919748, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB9fVZhIzC0LsvBg53dsR8tVnXHd5CRks5vX5rzgaJpZM4b2vWB .

--

Thomas Boutell, Chief Software Architect P'unk Avenue | (215) 755-1330 | punkave.com

[GiuseppeCM]

@boutell This is a nice check :), but the condition s == null is useless, because this filter is called only if the .search() is added to the cursor, and if a variable is not set, in the search function have undefined :D So the right control is if ((s === undefined) || (s === ''))

[boutell]

It can't hurt to be explicit about null too in case the rules change anywhere else, and it's not hurting the test results, so I'm going to keep that.

I just npm published a version with extensive unit test coverage of this to be quite sure, check it out.

[GiuseppeCM]

@boutell I have seen 👍