path: fix posix.relative returns different results #13738

DuanPengfei · 2017-06-17T10:09:44Z

Throw an error ERR_UNSUPPORTED_PLATFOMR when direct use
path.posix.resolve on Windows or direct use path.win32.resolve
on *nix.

Update docs, list win32 functions do not support direct use on
*nix and posix functions do not support direct use on Windows.

Update tests, only run current platform type test.

Fixes: #13683
Refs: #13714

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, path

addaleax · 2017-06-17T10:11:55Z

I’m sorry, but imho this is not worth breaking people’s code.

XadillaX · 2017-06-17T10:14:23Z

@addaleax we've discussed in #13683 with @refack.

Anyway, did you prefer process.emitWarning?

addaleax · 2017-06-17T10:17:43Z

@XadillaX I’ve replied in the issue thread, but this just seems like it’s breaking code without a good reason, switching to a warning doesn’t change that.

mscdex · 2017-06-17T14:28:11Z

-1 I don't think we should be artificially limiting these functions.

refack · 2017-06-17T22:36:03Z

I’m sorry, but imho this is not worth breaking people’s code.

-1 I don't think we should be artificially limiting these functions.

@addaleax @mscdex do you think limiting the assertion to only to when process.cwd() is used, will be acceptable?
IMHO those cases are bugs:

> path.posix.resolve('./gaga/')
'C:\\bin\\dev\\node/gaga'  // bug
> path.posix.resolve('/gaga/')
'/gaga'   // Ok

mscdex · 2017-06-17T22:45:08Z

I really don't think they need changing. IMHO when you explicitly access the path functions for a different platform, you should already be aware of what you're getting into...

refack · 2017-06-17T23:17:58Z

I really don't think they need changing. IMHO when you explicitly access the path functions for a different platform, you should already be aware of what you're getting into...

🤔 it's a thinker indeed...
I just have a gut feeling.. If we know we are going to return a nonsense answer we should throw or warn. I'm not sure that devs really know what they are getting into, they assume it'll just work, and work right.

DuanPengfei · 2017-06-18T00:48:30Z

It would be better to only throw error when rely on cwd. If choose this solution, we also need to chang the path.posix.relative. We found path.posix.resolve return mistake result in the process of resolving this issue 13683. So I think need to do something to make path.posix.relative return the corrent result in most cases.

addaleax · 2017-06-18T09:52:48Z

If we know we are going to return a nonsense answer we should throw or warn.

I might agree if the answer was actually nonsense … but it isn’t? The result may not directly usable for file system operations, but that doesn’t make it less consistent or meaningless.

It would be better to only throw error when rely on cwd.

I agree with @mscdex, we should not artificially limit these methods.

DuanPengfei · 2017-06-18T10:16:06Z

I will continue to fix it.

DuanPengfei · 2017-06-18T10:20:20Z

@addaleax If do not limit the method user will get incorrect result, it is also unreasonable.

refack · 2017-06-18T23:04:41Z

IMHO this is nonsensical:

> path.posix.resolve('./gaga/')
'C:\\bin\\dev\\node/gaga'  // bug

> path.win32.resolve('.\\gaga\\')
'\\mnt\\d\\code\\node\\gaga'

Consistent, yes, meaningful, I don't think so.
We've seen several times devs who are surprised it doesn't just work...

P.S. Just a point of thought, the use of process.cwd() is borderline actual FS access, and not pure string manipulation anymore.
Just as an example for a platform independent path manipulator URL requires a absolute base argument when first argument is relative.

P.P.S. Let see what @DuanPengfei comes up with 👍

DuanPengfei · 2017-06-19T06:04:02Z

My idea is make the result correct when do't rely on cwd. If rely on cwd will throw an error Error: ERR_UNSUPPORTED_PLATFORM. Because of relative and _makeLong rely on resolve function, also need to modify relative and _makeLong, make their result as correct as possible in most case.

DuanPengfei · 2017-06-20T08:39:48Z

Implemented some feature in my local code.

Idea

resolve: It will throw error when rely on cwd.

relative:

If from and to are both relative path, will generate fake path then call resolve to produce the absolute path, then continue run the next code.

win32 fake path: C:\Users\node\${from/to}, posix fake path: /Users/node/${from/to}

If one of from and to is relative path, will use another absolute path as the cwd to generate fake path then call resolve to produce the absolute path, then continue run the next code.

win32: if from is absolute, to = resolve(from + '\' + to) else from = resolve(to + '\' + from)
posix: if from is absolute, to = resolve(from + '/' + to) else from = resolve(to + '/' + from)

Examples

If ../../x's upper dirctory to long, e.g ../../../../../../x, the resolve function only can resolve to the root directory /x

on Windows

// both relative
path.posix.relative('a/b/c', '../../x');  // => '../../../../../x'
path.posix.relative('../../x, 'a/b/c'');  // => '../home/node/a/b/c'

/**
 * `from`'s upper path is too long beyond the root path
 */
path.posix.relative('../../../../../../../x, 'a/b/c'');  // => '../home/node/a/b/c'

// one relative
path.posix.relative('/a/b/c', 'd/e/f');  // => 'd/e/f'
path.posix.relative('a/b/c', '/d/e/f');  // => '../../..'

// both absolute
path.posix.relative('/a/b/c', '/d/e/f');  // => '../../../d/e/f'

on POSIX

// both relative
path.win32.relative('a\b\c', '..\..\x');  // => '..\..\..\..\..\x'
path.win32.relative('..\..\..\..\..\x', 'a\b\c');  // => '.\Users\nodea\b\c'

// one relative
path.win32.relative('C:\a\b\c', '..\..\..\x');  // => '..\..\..\x'
path.win32.relative('C:\..\..\..\x', 'a\b\c');  // => 'a\b\c'

// both absolute
path.win32.relative('C:\a\b\c', 'C:\d\e\f');  // => '..\..\..\d\e\f'

@refack @addaleax @mscdex What do you think of how this is done?

refack · 2017-06-20T13:10:00Z

@refack @addaleax @mscdex What do you think of how this is done?

IMHO it sounds like a good solution. If you replace the word "fake" with "surrogate" it'll sound even more sophisticated.

DuanPengfei · 2017-06-21T05:36:47Z

😁 My ability to speak in English is weak. I will improve the code, document and test, then push the code.

By the way, for the surrogate path, my colleague suggested me use C:\fakepath as the Chrome did when upload file. I have no preference for using C:\fakepath or C:\Users\node, do you have any preference?

refack · 2017-06-21T21:15:27Z

C:\fakepath or C:\Users\node

If chrome is using c:\fakepath that SGTM (but for some reason I'd prefer a lowercase c 🤷‍♂️ )

Throw an error `ERR_UNSUPPORTED_PLATFOMR` when direct use `path.posix.resolve` on Windows or direct use `path.win32.resolve` on *nix. Update docs, list win32 functions do not support direct use on *nix and posix functions do not support direct use on Windows. Update tests, only run current platform type test. Fixes: nodejs#13683 Refs: nodejs#13714

DuanPengfei · 2017-06-22T11:53:14Z

According to the idea, implement the code.

refack

Look good, just a few nits.

refack · 2017-06-22T12:01:14Z

lib/path.js

+
+    if (process.platform !== 'win32') {
+      if (!isFromAbsolute && !isToAbsolute) {
+        from = 'c:\\fakepath\\' + from;


store the surrogate in a const (or even a function if it works)

function absoluteWithSurrogate(p) { return win32.resolve(win32.join(`c:\\fakepath`, p)); }

refack · 2017-06-22T12:03:28Z

lib/path.js

+    }
+
+    if (process.platform !== 'win32') {
+      if (!isFromAbsolute && !isToAbsolute) {


looks like you can do this separately for to and from

if (isFromAbsolute) ... else ... if (isToAbsolute) ... else ...

Or maybe I'm wrong?

Because need to know both type of from and to, so I think I can't do this separately.

refack · 2017-06-22T12:05:35Z

lib/path.js

+
+    if (process.platform === 'win32') {
+      if (!isFromAbsolute && !isToAbsolute) {
+        from = '/fakepath/' + from;


wrap (same as above)

refack · 2017-06-22T12:07:37Z

test/parallel/test-path.js

+};
+
+{
+  let tests;


const tests = common.isWindows ? resolveTest.win32 : resolveTest.posix; const throwing = common.isWindows ? path.posix.resolve : path.posix.resolve;

refack · 2017-06-22T12:09:22Z

test/parallel/test-path.js

-      failures.push(`\n${message}`);
+};
+{
+  let tests;


refack · 2017-06-22T12:13:45Z

test/parallel/test-path.js

-     ['\\\\foo\\baz', '\\\\foo\\baz-quux', '..\\baz-quux'],
-     ['C:\\baz', '\\\\foo\\bar\\baz', '\\\\foo\\bar\\baz'],
-     ['\\\\foo\\bar\\baz', 'C:\\baz', 'C:\\baz']
+const relativeTest = {


I think it would be best to split out only the tests that would throw.
The other cases need to be tested on all platforms.

mscdex

Making my -1 explicit. I don't think we should be changing the behavior of these functions.

refack · 2017-06-22T13:11:25Z

@addaleax @mscdex since this is a good change, if we split the throwing logic to a separate PR, and just keep the fix with the surrogate, will you approve?

mscdex · 2017-06-22T13:19:08Z

@refack As I said, I don't think we should be changing the behavior of these functions. That includes both the throwing and the injecting of arbitrary path components into path function results.

addaleax · 2017-06-22T13:42:34Z

I’m sorry, but I agree with @mscdex; we really should not be doing this kind of change.

IIUC, the original bug report had one clear problem it pointed to: path.posix.relative('a/b/c', '../../x'); on Windows changed over time and produced a result that included .... as a component. We won’t be doing our users any favours if we replace one invalid part of the result with another invalid part.

refack · 2017-06-22T14:58:32Z

I’m sorry, but I agree with @mscdex; we really should not be doing this kind of change.

IIUC, the original bug report had one clear problem it pointed to: path.posix.relative('a/b/c', '../../x'); on Windows changed over time and produced a result that included .... as a component. We won’t be doing our users any favours if we replace one invalid part of the result with another invalid part.

Trying to figure out a path forward (sorry about the pun). Should we keep the path.posix.relative/path.win32.relative fixes here, and move the cwd assertion to a different PR, and discuss merits there?
@DuanPengfei tl;dr the tests should not change, just add more cases that will work correctly after the fix.

addaleax · 2017-06-22T15:03:05Z

Should we keep the path.posix.relative/path.win32.relative fixes here

I wouldn’t think of adding fakepath as a fix.

and move the cwd assertion to a different PR, and discuss merits there?

You can do that but I think both @mscdex and I have made it pretty clear that we don’t think those changes are a good idea at all.

refack · 2017-06-22T15:09:33Z

Should we keep the path.posix.relative/path.win32.relative fixes here

I wouldn’t think of adding fakepath as a fix.

So the main goal is to add more working cases to the tests (no changes). Specifically path.posix.relative('a/b/c', '../../x') and path.win32.relative('a\b\c', '..\..\x');

DuanPengfei · 2017-06-23T02:36:15Z

😵 Change so much code is because in the modification of this problem I found more bottom bug. Inject fakepath to path is compromise way. So now I think I should fix the issue 13683 here (only make the test case path.posix.relative('a/b/c', '../../x') return the expected result). Then I will open a new issue about path.posix.resolve, path.win32.resolve, path.posix.relative and path.win32.relative return wrong path. We can continue discuss how to resolve the new problem there.

DuanPengfei · 2017-06-23T09:41:50Z

#13887

refack · 2017-06-23T10:44:21Z

😵 Change so much code is because in the modification of this problem I found more bottom bug. Inject fakepath to path is compromise way. So now I think I should fix the issue 13683 here (only make the test case path.posix.relative('a/b/c', '../../x') return the expected result). Then I will open a new issue about path.posix.resolve, path.win32.resolve, path.posix.relative and path.win32.relative return wrong path. We can continue discuss how to resolve the new problem there.

👍 I think that will be good.

jasnell · 2017-06-26T16:24:04Z

doc/api/path.md

@@ -431,6 +431,39 @@ added: v0.11.15
 The `path.posix` property provides access to POSIX specific implementations
 of the `path` methods.

+*Note*: Be careful when using the following functions directly on Windows.


This note should be moved into the bodies of each of the API sections. Sitting above it, it is far more likely that the Note will become disconnected...

jasnell · 2017-06-26T16:25:13Z

doc/api/path.md

+### path.posix.resolve([...path])
+If after processing all given `path` segments an absolute path has not yet
+been generated, `path.posix.resolve` will throw [`UNSUPPORTED_PLATFORM`] error.
+


Perhaps:

*Note*: Care should be taken when using `path.posix.resolve()` on Windows due to ...

Then include a short snippet about why...

Same with the path.posix.relative() below.

suggestion:

*Note*: Care should be taken when using `path.posix.resolve()` on Windows due to the fact that resolve will use `process.cwd()` verbatim, and so the output will be a concatenation of Windows and POSIX style paths.

jasnell · 2017-06-26T16:27:17Z

doc/api/path.md

+been generated, `path.posix.resolve` will throw [`UNSUPPORTED_PLATFORM`] error.
+
+### path.posix.relative(from, to)
+If `from` and `to` are both relative path, `path.posix.relative` will use the


Please include the () when referencing functions... e.g. path.posix.relative()

jasnell · 2017-06-26T16:28:04Z

lib/path.js

@@ -187,6 +199,10 @@ const win32 = {
        path = arguments[i];
      } else if (!resolvedDevice) {
        path = process.cwd();
+
+        // If you use the current working directory,


// If the current working directory is used ...

jasnell · 2017-06-26T16:28:18Z

lib/path.js

@@ -201,6 +217,10 @@ const win32 = {
            path.slice(0, 3).toLowerCase() !==
              resolvedDevice.toLowerCase() + '\\') {
          path = resolvedDevice + '\\';
+        } else {
+          // If you use the current working directory,


// The the current working directory is used...

DuanPengfei · 2017-06-27T01:07:11Z

👌 Then I will modify the code only resolve the problem path.posix.relative return different results in this PR. I will create a new PR and modify the code according to your advice to resolve the problem #13887.

refack · 2017-07-24T11:52:40Z

@DuanPengfei anything I can do you assist in this?

DuanPengfei · 2017-07-24T12:00:21Z

@refack 😂 Sorry for a long time no response. Some time ago sick leave, this weekend will continue to repair the bug. If there is a need, I will ask you for help.

DuanPengfei · 2017-07-31T01:34:30Z

Accept the recommendations of @timothy Gu, decide to refer to the implementation of the Python or any other language. Do you think this is a good way?

TimothyGu · 2017-07-31T01:57:51Z

FWIW Python 3's relative() and resolve() equivalents do the same things as Node.js right now. Note: Python's relpath's operands are reversed: (to, from) rather than (from, to).

>>> ntpath.realpath(r'.\gaga')
'\\home\\timothy-gu\\dev\\node\\node\\gaga'
>>> ntpath.relpath(r'a\b\c', r'..\..\x')
'..\\node\\node\\a\\b\\c'

(I'm on Linux.)

I'm reluctant to change Node.js' current behavior.

DuanPengfei · 2017-07-31T02:18:25Z

We will not change the current behavior. And in this PR I will only make the return of path.relative('a/b/c', '../../x') be ../../../../../x as the Python do. Then think of what to do in #13887. To be honest, I do't think it's a bug. It's normal to return the unexpected result when use path.posix.relative on Windows. Python or any other language also have this problem. 😂

BridgeAR · 2017-09-19T22:50:24Z

I am closing this due to the long inactivity.

@DuanPengfei please feel free to leave a comment if you would like to follow up on this by fixing the issue itself. Your work is much appreciated nevertheless.

Using path.posix.relative is not an option because it returns the wrong path. See: * nodejs/node#13887 * nodejs/node#13738

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. path Issues and PRs related to the path subsystem. labels Jun 17, 2017

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 17, 2017

DuanPengfei mentioned this pull request Jun 17, 2017

path.posix.relative returns different results for *nix and Windows versions of node #13683

Closed

mscdex removed the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jun 17, 2017

DuanPengfei added 4 commits June 22, 2017 17:03

fix: typo

0864e8e

fix: rm unnecessary test

acfdb47

feat: resolve only throw error when rely on cwd

8d604e1

DuanPengfei force-pushed the fix-path_posix_resolve branch from 6d17e03 to 8d604e1 Compare June 22, 2017 11:07

doc: update path.posix and path.win32 doc

c8bbf22

refack reviewed Jun 22, 2017

View reviewed changes

mscdex suggested changes Jun 22, 2017

View reviewed changes

DuanPengfei closed this Jun 23, 2017

DuanPengfei reopened this Jun 23, 2017

jasnell requested changes Jun 26, 2017

View reviewed changes

DuanPengfei mentioned this pull request Jun 27, 2017

path.posix.resolve, path.posix.relative return wrong path on Windows #13887

Closed

refack mentioned this pull request Jul 6, 2017

path: fix normalize on directories with two dots #14107

Closed

3 tasks

tniessen mentioned this pull request Jul 22, 2017

path.join('C:') = 'C:.' only in windows node LTS version #14405

Closed

BridgeAR closed this Sep 19, 2017

AngusMorton added a commit to AngusMorton/run that referenced this pull request May 2, 2023

Normalize the path to the routes.d.ts file

785fd15

Using path.posix.relative is not an option because it returns the wrong path. See: * nodejs/node#13887 * nodejs/node#13738

AngusMorton mentioned this pull request May 2, 2023

Normalize the routes path marko-js/run#23

Merged

rturnq pushed a commit to marko-js/run that referenced this pull request May 3, 2023

fix: Normalize the path to the routes.d.ts file (#23)

82ed1a1

Using path.posix.relative is not an option because it returns the wrong path. See: * nodejs/node#13887 * nodejs/node#13738

path: fix posix.relative returns different results #13738

path: fix posix.relative returns different results #13738

Conversation

DuanPengfei commented Jun 17, 2017

Checklist

Affected core subsystem(s)

addaleax commented Jun 17, 2017

XadillaX commented Jun 17, 2017 • edited Loading

addaleax commented Jun 17, 2017

mscdex commented Jun 17, 2017 • edited Loading

refack commented Jun 17, 2017

mscdex commented Jun 17, 2017

refack commented Jun 17, 2017

DuanPengfei commented Jun 18, 2017

addaleax commented Jun 18, 2017

DuanPengfei commented Jun 18, 2017

DuanPengfei commented Jun 18, 2017

refack commented Jun 18, 2017 • edited Loading

DuanPengfei commented Jun 19, 2017

DuanPengfei commented Jun 20, 2017 • edited by gibfahn Loading

Idea

Examples

refack commented Jun 20, 2017

DuanPengfei commented Jun 21, 2017

refack commented Jun 21, 2017 • edited Loading

DuanPengfei commented Jun 22, 2017

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex left a comment

Choose a reason for hiding this comment

refack commented Jun 22, 2017

mscdex commented Jun 22, 2017 • edited Loading

addaleax commented Jun 22, 2017

refack commented Jun 22, 2017 • edited Loading

addaleax commented Jun 22, 2017

refack commented Jun 22, 2017

DuanPengfei commented Jun 23, 2017

DuanPengfei commented Jun 23, 2017 • edited Loading

refack commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DuanPengfei commented Jun 27, 2017 • edited Loading

refack commented Jul 24, 2017

DuanPengfei commented Jul 24, 2017

DuanPengfei commented Jul 31, 2017

TimothyGu commented Jul 31, 2017

DuanPengfei commented Jul 31, 2017

BridgeAR commented Sep 19, 2017

XadillaX commented Jun 17, 2017 •

edited

Loading

mscdex commented Jun 17, 2017 •

edited

Loading

refack commented Jun 18, 2017 •

edited

Loading

DuanPengfei commented Jun 20, 2017 •

edited by gibfahn

Loading

refack commented Jun 21, 2017 •

edited

Loading

mscdex commented Jun 22, 2017 •

edited

Loading

refack commented Jun 22, 2017 •

edited

Loading

DuanPengfei commented Jun 23, 2017 •

edited

Loading

refack Jun 26, 2017 •

edited

Loading

DuanPengfei commented Jun 27, 2017 •

edited

Loading