fix: use safe-decode-uri-component #450

ramybenaroya · 2018-04-23T11:19:42Z

This takes care of file names with '%' character. File names like
these can be automatically created by IDEs for backup.
Also, added a test for adding file with '%' character to InMemoryFileSystem.

Fixes #422

This takes care of file names with `'%'` character. File names like these can be automatically created by IDEs for backup. Also, added a test for adding file with `'%'` character to `InMemoryFileSystem`. Fixes sourcegraph#422

ramybenaroya · 2018-04-23T11:27:54Z

tsconfig.json

@@ -9,6 +9,7 @@
    "sourceMap": true,
    "declaration": true,
    "strictFunctionTypes": false,
+    "esModuleInterop": true,


Added this because safe-decode-uri-component is written in CommonJS. With this flag safe-decode-uri-component can be imported by:

import safeDecodeURIComponent from 'safe-decode-uri-component'

rather than

import * as safeDecodeURIComponent from 'safe-decode-uri-component'

ramybenaroya · 2018-04-23T11:31:36Z

src/test/memfs.test.ts

@@ -18,6 +18,14 @@ describe('memfs.ts', () => {
                sinon.assert.calledOnce(listener)
                sinon.assert.calledWithExactly(listener, 'file:///foo/bar.txt', undefined)
            })
+            it('should add a URI with `%` character and emit an event', () => {


This test would have been failed before this PR changes.
This is exactly the case #422 referres to.

Since esModuleInterop set to true, `rimraf` can be imported like a regular module

ramybenaroya · 2018-04-23T11:42:59Z

src/test/fs.test.ts

@@ -2,7 +2,7 @@ import * as chai from 'chai'
 import chaiAsPromised = require('chai-as-promised')
 import * as fs from 'mz/fs'
 import * as path from 'path'
-import * as rimraf from 'rimraf'
+import rimraf from 'rimraf'


Fix build failure
https://travis-ci.org/sourcegraph/javascript-typescript-langserver/jobs/370047243#L1806

codecov · 2018-04-23T11:46:57Z

Codecov Report

Merging #450 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #450      +/-   ##
==========================================
+ Coverage   83.18%   83.19%   +<.01%     
==========================================
  Files          15       15              
  Lines        2040     2041       +1     
  Branches      415      415              
==========================================
+ Hits         1697     1698       +1     
  Misses        341      341              
  Partials        2        2

Impacted Files	Coverage Δ
src/util.ts	`89.88% <100%> (+0.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 312e432...bca9756. Read the comment docs.

felixfbecker · 2018-04-30T19:23:07Z

Reading the code, it appears to me this only fixes the issue for file names with invalid escapes. If you modify your test to use a valid escape in the file name, would it still pass?

ramybenaroya · 2018-04-30T19:29:05Z

Do you mean 'file:///foo/a%20bar.txt'?
If so, yes it would pass.

felixfbecker · 2018-04-30T20:25:13Z

I meant a file that is literally called my%20file.txt

ramybenaroya · 2018-04-30T23:29:21Z

oh, I see what you mean.
You are right, my PR's purpose is only for fixing invalid escaped.
Generally, on runtime, at this line, segment argument will already be my%2520file.txt since InMemoryFileSystem.add's input is a URI, meaning a valid escaped URL.

I made this PR to fix #422 (where for some reason paths are not escaped properly) in order to prevent a fatal exception.

felixfbecker · 2018-05-01T00:27:24Z

Okay, in that case I believe the issue should be fixed properly.

ramybenaroya · 2018-05-01T11:04:07Z

Do you want to close the PR?
I am ready to help with a proper fix if you can guide me a bit since this is my first contribution in this project.
What do you have in mind for a proper fix?

autozimu · 2018-05-01T17:36:39Z

Sorry for bluntly chime in.

#422 is not caused by invalid encoding.

The file name is literally %test.js, which is used by vim as backup file by default (vaguely remember emacs does the same thing).

As I also commented under #422, language client didn't send this path. It is picked up by language server when trying to build index.

With that said, this should resolve #422, though it only fixes the consequeue but not the cause.

I'd love to see #422 got resolved, but a concern from this project's perspective is that how much compatibility this new safe-decode-uri-component can provide as a replacement of decodeURIComponent , when it comes to decoding valid uris.

ramybenaroya · 2018-05-01T18:01:36Z

@autozimu Thanks for the explanation.
I am closing this for the following reasons:
1.safe-decode-uri-componentis probably not a good candidate for replacing native decodeURIComponent since it is not widely used and doesn't meet this project's standards.
2. A better solution will be to fix the index building process in order that %test.js will be encoded to %25test.js.
3. Fixing the consequence is never good enough.

@felixfbecker @autozimu Thanks for your input and attention.

felixfbecker · 2018-05-01T18:09:13Z

For context, the reason why the code calls encodeURIComponent(decodeURIComponent(segment)) is so that files names have a canonical name and can be used as map keys. Otherwise you can end up with the client sending URIs for certain files with a different URI than what the language server came up with and you get duplicate entries in Maps. For example, a client might encode a space as +, but the language server used %20. Or the language server chose to escape some special character that the client didn't.

Now the bug here seems to be indeed that the URI for %test.js at that point in the code is not properly encoded to %25test.js. I would set a conditional breakpoint inside the function (or an if with console.trace()) and see where the value comes from.

autozimu · 2018-05-01T18:49:29Z

The call comes from

javascript-typescript-langserver/src/fs.ts

Line 81 in 312e432

subscriber.next(normalizeUri(base + file))

The result come from glob should already be valid paths.

For the concern that the path might have other aliases other than its canonical form. I believe it can be done by querying from filesystem, https://github.com/autozimu/LanguageClient-neovim/blob/84445be4a3bf7f67547abad205c6c46eb289844d/src/languageclient.rs#L1619 It won't cover all cases, but should be enough for general cases.

felixfbecker · 2018-05-01T19:34:30Z

Oh, seems like all it needs it to encode file in that line! When constructing the URI from file paths, it needs to be encoded.

For the concern that the path might have other aliases other than its canonical form. I believe it can be done by querying from filesystem

That won't work because not all files the language server works with are available on the file system.

fix: use safe-decode-uri-component

7fb570e

This takes care of file names with `'%'` character. File names like these can be automatically created by IDEs for backup. Also, added a test for adding file with `'%'` character to `InMemoryFileSystem`. Fixes sourcegraph#422

ramybenaroya commented Apr 23, 2018

View reviewed changes

fix: dont import rimraf with property notation

9ed359e

Since esModuleInterop set to true, `rimraf` can be imported like a regular module

ramybenaroya commented Apr 23, 2018

View reviewed changes

Merge branch 'master' into safer-decode-uri-component

bca9756

ramybenaroya closed this May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: use safe-decode-uri-component #450

fix: use safe-decode-uri-component #450

Uh oh!

ramybenaroya commented Apr 23, 2018

Uh oh!

ramybenaroya Apr 23, 2018

Uh oh!

ramybenaroya Apr 23, 2018

Uh oh!

ramybenaroya Apr 23, 2018

Uh oh!

codecov bot commented Apr 23, 2018 •

edited

Loading

Uh oh!

felixfbecker commented Apr 30, 2018

Uh oh!

ramybenaroya commented Apr 30, 2018

Uh oh!

felixfbecker commented Apr 30, 2018

Uh oh!

ramybenaroya commented Apr 30, 2018

Uh oh!

felixfbecker commented May 1, 2018 •

edited

Loading

Uh oh!

ramybenaroya commented May 1, 2018

Uh oh!

autozimu commented May 1, 2018

Uh oh!

ramybenaroya commented May 1, 2018

Uh oh!

felixfbecker commented May 1, 2018

Uh oh!

autozimu commented May 1, 2018

Uh oh!

felixfbecker commented May 1, 2018

Uh oh!

Uh oh!

fix: use safe-decode-uri-component #450

fix: use safe-decode-uri-component #450

Uh oh!

Conversation

ramybenaroya commented Apr 23, 2018

Uh oh!

ramybenaroya Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

ramybenaroya Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

ramybenaroya Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

felixfbecker commented Apr 30, 2018

Uh oh!

ramybenaroya commented Apr 30, 2018

Uh oh!

felixfbecker commented Apr 30, 2018

Uh oh!

ramybenaroya commented Apr 30, 2018

Uh oh!

felixfbecker commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramybenaroya commented May 1, 2018

Uh oh!

autozimu commented May 1, 2018

Uh oh!

ramybenaroya commented May 1, 2018

Uh oh!

felixfbecker commented May 1, 2018

Uh oh!

autozimu commented May 1, 2018

Uh oh!

felixfbecker commented May 1, 2018

Uh oh!

Uh oh!

codecov bot commented Apr 23, 2018 •

edited

Loading

felixfbecker commented May 1, 2018 •

edited

Loading