Preserve trailing slashes in RestClient.getUrl()

[stephenmichaelf]

Thanks for contributing @andrew8er ! Could you add unit tests for the change? Including ones that would fail in the current code. Thanks :)

[andrew8er]

I added a test that demonstrates the bug.

Something else I noticed: Util.getUrl() uses Node's path.resolve(). Under certain conditions, this function tries to resolve file system paths from the current working directory. I wonder if this could lead to an unintended information disclosure. Maybe it would be better to use a combination of join() and normalize()?

[damccorm]

This looks good to me, if you move the tests to test/units/resttests.ts we should be able to merge (we changed the testing structure). Concerning resolve vs join/normalize, I believe that resolve only uses the current working directory if no non-empty paths are specified, which doesn't seem to be a risk here.

[andrew8er]

I rebased the branch, but I get some errors while compiling the tests:

> tsc -p ./test/units
test/units/resttests.ts(77,112): error TS2345: Argument of type '{ deserializeDates: boolean; }' is not assignable to parameter of type 'IRequestOptions'.
  Object literal may only specify known properties, and 'deserializeDates' does not exist in type 'IRequestOptions'.
test/units/resttests.ts(99,112): error TS2345: Argument of type '{ deserializeDates: boolean; }' is not assignable to parameter of type 'IRequestOptions'.
  Object literal may only specify known properties, and 'deserializeDates' does not exist in type 'IRequestOptions'.

[damccorm]

I just did a clean install and didn't have any issues. Did you pull the most recent version? It sounds like you don't have the changes from here and/or here

[andrew8er]

I rebased my PR atop master, so yes, I have both commits.

Additionally, while trying to install dependencies with npm 5.6.0 i get:

npm WARN ajv-keywords@3.2.0 requires a peer of ajv@^6.0.0 but none is installed. You must install peer dependencies yourself.

It works with yarn 1.7.0 though.

[damccorm]

Did you make sure to rebuild before you tested? Maybe try just a clean clone/install/build/test? I can't reproduce on my end, even though I'm using npm 5.6.0 as well.

[andrew8er]

Ok, it was not clear from the Readme that I needed to run build before test. Tests pass.

[damccorm]

Ok great! And it probably should just build automatically, I'll update so that it does.