COUCH2PG_USERS_META_DOC_LIMIT Safety Check

[nomulex]

We need to put a numerical safety check on this variable to guard against wrong entries.

[mrsarm]

@nomulex could you specify what would be the acceptable range?

Moreover, I noticed that we didn't document the variable in the table here, a good definition with recommendations about what value to set is here, so just a note to add that same description in the same PR in the README file.

[garethbowen]

I think the main thing is to make sure it's numerical using the safeNum function just like the other environment variables. We could also check that it's greater than 0, but I think that's overkill.

[nomulex]

+1 @garethbowen. Just making sure it's numerical.

[mrsarm]

This PR #100 is addressing this ticket, that kind of duplicate #97.

[mrsarm]

We forgot to triage and assign a project to this ticket before fixing it. I just assigned it to one board and moved it to ready for test because it was fixed.

Because it is a doc fix and then only add a safety check, we should only check whether setting the variable or not the startup is affected, how it behave if a wrong value is assigned, and whether at least the sync continue to happens after a successful startup.

The branch is 97-add-documentation-on-COUCH2PG_USERS_META_DOC_LIMIT, (#100).

[meghna-khemka]

Tested above issue and issue seems to be working fine

Scenario 1:
The launching the app without passing the variable.

Scenario 2:Pass with a valid numeric number

Scenario 3:Pass an invalid number.

[mrsarm]

Actually I don't think that the last test behaves as we expected , the process should fail if an invalid number is passed, otherwise it gives the impression the value passed was "accepted" and used by medic-couch2pg, but in reality it parsed as a NaN value, and not sure what that means when the process tries to use the value there to limit the number of docs per request.

@njogz I know we use the same trick to parse other values (the use of safeNum), but maybe we have to change it to prevent future errors, or I'm missing something here...

[njogz]

@mrsarm it should work with a default value if the environment variable passed is invalid which is what this code is supposed to do. Do you mind trying to replicate what @meghna-khemka got? I see the default value when I pass an invalid number.

[mrsarm]

@njogz OK maybe it's out of scope, the behavior of safeNum is wrong, but previous to your work.

I have created a separated ticket to fix the issue later, and with low priority: #107

So for me the PR is OK, and because was approved in AT, going to merge.

[meghna-khemka]

@mrsarm : I think I had some Git rebase issues which lead me to believe that it showing up NaN whereas incase I pass a arbritary string in this variable.Upon testing again I can confirm that if I pass a string it is picking default value of 50 instead of NaN .I believe this is not expected? Can we get this fixed...

I think change required would be
couchdbUsersMetaDocLimit: safeNum(process.env.COUCH2PG_USERS_META_DOC_LIMIT || 50)

[mrsarm]

@meghna-khemka thanks for the new report. Anyway, this is not new, because the safeNum function is used in most of the numeric env variables. I have created another issue to keep track of the malfunctioning of the function: #107. Could you add your findings there please?

Anyway, returning the default value instead of a NaN makes the error less severe, because it's hard to know how a NaN may be interpreted by the different parts of the app, while default values we know are safe, but still this is a bug.