nsqd: fix mpub binary param parsing

[andyxning]

According to the docs about mput.

topic - the topic to publish to
binary - bool ('true' or 'false') to enable binary mode

It indicates that:

• if binary param is not specified, text mode is used.
• if binary param is specified
  • "true", use binary mode
  • "false", use text mode

However, according to the implementation, if we specified binary=false in the mput url, it will be processed as binary mode which is wrong as we currently only check the existence of binary url param. :)

/cc @mreiferson @ploxiln @jehiah

[ploxiln]

It was probably intended that you could specify "binary=true" or "binary=1" or "binary=True" etc.
We may want to keep compatibility with some of these other ways that people may have been using (accidentally, but it worked).

[andyxning]

@ploxiln Yes, it is difficult for us to correct the non normalized uses now. I still suggest we correct and normalize the correct usage for v1.0, as the final release for 1.0 has not been released yet. :) We still has a time window to merge this. And, also we should record this backward uncompatibility clearly for end users.

[mreiferson]

I agree with @ploxiln, I think the simplest improvement would be to explicitly check for 0 and false (lowercase) and skip binary mode if so. All other values should be considered true and thus effectively backwards compatible.

[jehiah]

We could treat values outside of the 0,1, true, false set as "true" but log them as deprecated so we have the option to tighten things up in the future.

[andyxning]

@jehiah Making 0 and false as true can not make a positive progress in correcting this error usage.

@mreiferson

explicitly check for 0 and false (lowercase) and skip binary mode if so. All other values should be considered true and thus effectively backwards compatible.

This seems good. However, we can not enumerate or simply assume that end users will only use 0 and "false" as the indicator to use text multiput mode. We can not make any assumption about this.

As we are approaching to release v1.0, backward compatibility is an best effort instead of a guarantee. Please correct me if i am wrong. :)

[jehiah]

@andyxning sorry my comment might not have been clear. I'm saying that we can treat documented/expected values of (0, 1, true, false) different from all other non-empty unknown values (Which we could treat as true AND log as deprecated). That gives backwards compatibility, but by logging unknown values as deprecated, it gives forwards compatibility to being more strict in our parsing.

[andyxning]

@jehiah If i understand you correctly,

• if binary param is not specified, text mode is used.
• if binary param is specified
  • "true" or "1", use binary mode
  • "false" or "0", use text mode
  • All other empty or non-empty values will be logged as deprecated and will be parsed as true, i.e., binary mode

Is that correct?

/cc @mreiferson @ploxiln

[ploxiln]

Yes that's what we have in mind.

[andyxning]

@mreiferson @jehiah

If both of you agree with the binary param parse schema, i will change code later today. :)

[mreiferson]

👍🏻

[andyxning]

@mreiferson @jehiah @ploxiln

Done. PTAL.

[andyxning]

Btw, can we make a appoint about at which version we will enforce the parse schema and forbid all deprecated binary param values?

I think this may be needed to be placed in nsq.io about the the mput api.

[ploxiln]

I think the functionality is correct. But the code (and the log message) is too verbose. Also, we need not accommodate anything but lowercase. So this can be shorter and simpler.

One better option would be to use a single map like so:

var boolParams = map[string]bool{
    "true":  true,
    "false": false,
    "1":     true,
    "0":     false,
}

...

binary := false
param, ok := reqParams["binary"]
if ok {
    binary, ok = boolParams[binaryParam]
    if !ok {
        binary = true
        s.ctx.nsqd.logf(LOG_WARN, "deprecated value '%s' used for /mpub binary param")
    }
}

if binary {
    ....

This even simpler version would also be fine in this situation, in my humble opinion:

binary := false
param, ok := reqParams["binary"]

if param == "true" || param == "1" {
    binary = true
} else if param == "false" || param == "0" {
    binary = false
} else if ok {
    binary = true
    s.ctx.nsqd.logf(LOG_WARN, "deprecated value '%s' used for /mpub binary param")
}

if binary {
    ....

(I didn't use tabs just because I can't type them manually in this comment box.)

I don't think it's worth testing that the log message is emitted. But it is a good idea to test that the binary param is interpreted the way we want (by having binary or text mode mpub requests accepted or rejected).

[andyxning]

@ploxiln Thanks for your advice. The code i wrote first was really complicated than yours. 👍

I have read your version and adjust may initial implementation. PTAL.

Btw, i have added the binary param parse schema in the comment.

[ploxiln]

I think this can just be if !binaryMode {

[andyxning]

Will do.

[ploxiln]

I called this "boolParams" because it could be used for other boolean params besides "binary" (we don't really have any other simple boolean params right now though).

If it was really only for "binary", we wouldn't put it in the file/package scope, we'd just put it in the only function that would use it.

[andyxning]

Sounds reasonable. Will do.

[ploxiln]

Looks OK to me.

[andyxning]

@ploxiln Comments addressed. PTAL.

[ploxiln]

Thanks for the updates. Still looks OK to me :)

I have the ability to merge this, since just a few weeks ago, but I'd prefer to give more time for a thumbs-up from one of the other maintainers.

[jehiah]

Thanks @andyxning for the fix, and @ploxiln for leading review on this!

[andyxning]

Thanks for your review @ploxiln @jehiah @mreiferson