allow disabling sketchy string coercion in config

[vkarpov15]

Re: strongloop/loopback#1217, strongloop/loopback#1221, #299, etc. Ran into this bug yet again today. This coercion of string params is a major antipattern if you're using mongodb and there really needs to be a way to opt out of it.

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[emgeee]

+1

[diegonetto]

+1

[alexhidalgo]

+1

[richardpringle]

ok to test

[richardpringle]

@vkarpov15, you need to sign the CLA for this to land!

[vkarpov15]

@richardpringle where's the CLA?

[richardpringle]

@superkhau, do you remember if the tests were passing CI on this PR merge: strongloop/loopback-connector-remote@8343a64 ?

Same test keeps failing on difference builds.

(edit) Same test keeps failing on different instances of the same dependent build.

Also, are there other tests that aren't running due to the failed dependent? I'm having the same issue with a PR on datasource-juggler.

[superkhau]

Yeah, it was green when I merged it. However, when I check now, its partially red. I put in a fix here: strongloop/loopback-connector-remote#47

[stevegaossou]

+1

[vkarpov15]

Anything else I can do to help get this merged? I'd really rather not have to write a separate access hook for every single model to work around this bug.

[superkhau]

@richardpringle I believe remoting is all green again, just gotta wait for the LGTM from @bajtos at strongloop/loopback-connector-remote/pull/47

[richardpringle]

@slnode test please

[bajtos]

Hey @vkarpov15, thank you for the pull request, this is a neat solution. Just to let you know, our intention is to rework the coercion significantly to fix (hopefully all of) know issues. However, I think your solution is a great short-term improvement!

In order to get it landed, I need you to add a unit-test demonstrating correctness of your implementation and preventing regressions in the future.

[vkarpov15]

@bajtos there's your test, this feature now has 100% coverage.

[bajtos]

Please add an empty line before the test to serve as a visual delimiter.

[bajtos]

Ah, I might have misunderstood the intention of this change originally, I though the option would be applied on per-argument basis.

If I understand it correctly, the new option is set globally. LoopBack users have two options how to enable the new feature:

1. In server/config.json, set:

{
  "remoting": {
    "rest": {
      "coerce": false
    }
  }
}

2. In server/middleware.json:

{
  "routes": {
     "loopback#rest": {
       "params": {
         "coerce": false
        }
     }
  }
}

@vkarpov15 is my understanding correct?

Considering that this new flag is global and controls only coercion of "any" types, I would like to use a different - more descriptive - option name, e.g. coerceAnyType or similar.

[vkarpov15]

Yep your understanding is correct. I want a way to opt out of coercion of URL params at a global level. Feel free to make the changes you suggested on your end or close this PR - I've already put in way more time into this trivial one-liner than it's worth.

[vkarpov15]

I'm gonna close this since I'm getting the feeling that this PR is not welcome. I'm just gonna hack around this for now with:

const HttpContext = require('strong-remoting/lib/http-context')

const _httpContextBuildArgs = HttpContext.prototype.buildArgs
HttpContext.prototype.buildArgs = function (method) {
  const accepts = method.accepts
  accepts.forEach(param => {
    if (param && param.arg === 'id') {
      param.type = 'string'
    }
  })
  return _httpContextBuildArgs.call(this, method)
}

as a temporary fix while we work on the long-term solution of getting off of loopback.