fix: flushFilePath() with fix for Windows #4362

jeffb-sfdc · 2022-08-11T16:03:02Z

What does this PR do?

There's a bug in VS Code, which Microsoft refuses to fix (see microsoft/vscode#152993). After a file has been renamed, if only the casing has changes (eg no characters added or removed) VS Code passes the stale path to commands. This PR adds flushFilePath() and flushFilePaths() to fix this issue.

What issues does this PR fix or reference?

##3575, @W-10273785@

Functionality Before

Rename an Apex class's file name (only changing the case of the filename), and rename it's metadata file
Open the file and change the name of the class to match
With the file open, right-click in the editor and select "SFDX : Deploy This Source to Org"
This results in an error in the output: "File name mismatch with class name"

Functionality After

1-3: the same
4. This deployment succeeds

jeffb-sfdc · 2022-08-11T16:05:52Z

This PR was originally #4240, but needed to be backed out due to an issue running on Windows. I looked into the issue, and it was b/c under Windows, fs.realpathSync.native() returns a string with the first character (the drive letter) capitalized, and this causes code to fail further downstream.

jeffb-sfdc · 2022-08-11T16:10:43Z

packages/salesforcedx-utils-vscode/test/unit/helpers/utils.test.ts

-
-    const exampleJsonString = JSON.stringify({ name: 'exampleName', error: 'exampleError' });
+    const exampleJsonString = JSON.stringify({
+      name: 'exampleName',


Note: there's a lot of format changes here in this PR. This was not my doing. When I save files, they are automatically being re-formatted. I'm not sure if there was a recent configuration change, but it seems like we have a lot of formatting changes in PRs lately.

I've seen this also. Checked in w Gordon about it yesterday. We're going to create a ticket to look into why this is happening.

jeffb-sfdc · 2022-08-11T16:11:06Z

packages/salesforcedx-vscode-apex/src/commands/forceLaunchApexReplayDebuggerWithCurrentFile.ts

-      fsPath: sourceUri.fsPath
-    }
-  );
+  await vscode.commands.executeCommand('sfdx.launch.replay.debugger.logfile', {


more auto-formatting changes...

jeffb-sfdc · 2022-08-11T16:11:26Z

packages/salesforcedx-vscode-apex/src/commands/forceLaunchApexReplayDebuggerWithCurrentFile.ts

-import {
-  forceAnonApexDebug
-} from './forceAnonApexExecute';
+import { testOutlineProvider } from '../views/testOutlineProvider';


more auto-formatting changes...

klewis-sfdc · 2022-08-11T16:44:11Z

packages/salesforcedx-utils-vscode/src/helpers/utils.ts

+    // causes problems further down stream.  To fix this, we'll use the path
+    // returned from fs.realpathSync.native() and then capitalize the first
+    // character.
+    nativePath = nativePath.charAt(0).toLowerCase() + nativePath.slice(1);


stupid questions b/c windows. The default is C:\ but can a user have a drive named a lower case letter? So would d:\ be valid vs D:? There may be a corner case here were a drive is actually a lower case letter (a quick google told me this does happen in some instances, like when using a usb drive). We probably need a follow on ticket here to address the corner case and having the ability to ask the filesystem the valid path.

From what I can tell, what's displayed is always uppercase (D:\) but what the APIs/SDK uses is always lowercase (d:\). I tested this on a VM running Windows, but it's VERY slow, so I'd like a more thorough test on a real Windows machine b/f we merge.

(also, if I remember correctly, Windows is case-insensitive, so d:\ is the same as D:\. When one performs a FileExists("c:\\foo\\bar.txt"), it's the same as FileExists("C:\\FOO\\BAR.TXT")

I'm full of dumb questions. With the case insensitivity of windows why are we required to make the above change? It's been too long I don't remember what the 'down stream' changes are that result in the issue.

That's a really good question, and I wondered about that as well, but unfortunately, I don't know why the casing is failing. When I was debugging this, I noticed that the path was "c:\bla\bla\bla.cls" but fs.realpathSync.native() was returning "C:\bla\bla\bla.cls", and when I changed the case for the first char (the drive letter), our actions (deploy, delete, etc...) worked again.

klewis-sfdc · 2022-08-11T16:45:23Z

packages/salesforcedx-utils-vscode/test/unit/helpers/utils.test.ts

+
+    const result = flushFilePath(filePath);
+
+    expect(realpathSyncNativeStub.called).to.equal(true);


Since calledOnce is used below, is the expect for "called" needed here?

I wrote this a while ago... I think at the time my thought was, _"test & verify that it was called... ...AND test & verify that it was only called once"..., but I think you are right, in that it might be a bit redundant. I'll go & remove it.

+1 nice catch

klewis-sfdc · 2022-08-11T16:47:10Z

packages/salesforcedx-utils-vscode/test/unit/helpers/utils.test.ts

+
+    expect(result).to.equal(filePaths);
+
+    realpathSyncNativeStub.restore();


Would this restore() be better placed in an afterEach() block? Or do you prefer to keep it here inside each test case block?

For the moment, there's only one test, so I think keeping it central to the test is better. If there were more tests, and that stub was used in every test (and the stubbed value never changed), then placing it in beforeEach would be a better place, but for here & now, probably best to keep it where it is.

+1
Seems you could move the stub into a beforeEach and the restore into an afterEach FTW

hum... I can't tell from the formatting but the flushFilePath and flushFilePaths describe blocks both appear to have tests that create and restore the following stub

.stub(fs.realpathSync, 'native') .returns(realpathSyncFilePath); ... realpathSyncNativeStub.restore();

should those not be consolidated? I typically lean towards following patterns in tests to make them easier to grok for both new and existing developers so I'd prefer we put the stub/restore in before/after each blocks. This could be at the top level describe if that makes sense or just in a describe wrapper for all flushFilePath* describe blocks.

oh..above I mean I couldn't tell if both describe blocks were wrapped in a single top level describe.

@gbockus-sf maybe we should take this off line & discuss f2f. realpathSyncNativeStub can't be hoisted or shared, b/c the mocked value that's returned changes from test to test.

@jeffb-sfdc Sure I'll ping ya in slack

I wrapped everything in a describe(), added a beforeEach() and afterEach(), and moved realpathSyncNativeStub

klewis-sfdc · 2022-08-11T16:51:48Z

packages/salesforcedx-vscode-apex/src/commands/forceLaunchApexReplayDebuggerWithCurrentFile.ts

+  // but it actually it returns string | undefined.  Well, regardless, since flushFilePath() takes
+  // a string (and guards against empty strings) using the Non-null assertion operator
+  // (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html#non-null-assertion-operator)
+  // fixes the issue.


klewis-sfdc · 2022-08-11T16:53:29Z

packages/salesforcedx-vscode-core/test/vscode-integration/commands/forceSourceDelete.test.ts

  });
 });

 describe('ConfirmationAndSourcePathGatherer', () => {
  const examplePath = path.join('example', 'path');
-  const explorerPath = { fsPath: examplePath } as vscode.Uri;
+  const explorerPathUri = { fsPath: examplePath } as vscode.Uri;


klewis-sfdc

Looks good to me @jeffb-sfdc ! Left one question about maybe using afterEach in the tests, up to you for consideration.

gbockus-sf

One concerning point of discussion around windows machines that have a lower case letter drive.

klewis-sfdc · 2022-08-11T20:28:40Z

I ran the same tests on M1 Mac as on the last PR, all checks out:

✅ SFDX: Deploy Source to Org works after renaming locally by only making changes to the casing in the file+class name
✅ SFDX: Deploy Source to Org works when deploying multiple files after renaming one locally by only making changes to the casing in the file+class name
✅ SFDX: Retrieve Source from Org works as before
* Note: If you rename locally, you still need to deploy the class first so that it is renamed on the server before executing a “Retrieve” operation
✅ SFDX: Delete from Project and Org works as before
* Note: Similarly, if you rename the component/class locally, you need to deploy it so that the name is aligned on the server before running a delete operation

gbockus-sf

changes look good 👍
✅ verified on windows

Also attempted to create a volume to check the lower case volume letter and found it wasn't possible using the out of the box disk manager for windows so I think we're good to go 👍

fix: flushFilePath() with fix for Windows

08ed3c5

jeffb-sfdc requested a review from klewis-sfdc August 11, 2022 16:03

jeffb-sfdc requested a review from a team as a code owner August 11, 2022 16:03

jeffb-sfdc commented Aug 11, 2022

View reviewed changes

jeffb-sfdc added 2 commits August 11, 2022 09:19

fix: flushFilePath() with fix for Windows

a4427fc

fix: flushFilePath() with fix for Windows

da8080c

klewis-sfdc reviewed Aug 11, 2022

View reviewed changes

klewis-sfdc approved these changes Aug 11, 2022

View reviewed changes

fix: flushFilePath() with fix for Windows

84a48d7

gbockus-sf reviewed Aug 11, 2022

View reviewed changes

fix: flushFilePath() with fix for Windows

f632691

jeffb-sfdc and others added 3 commits August 11, 2022 16:03

fix: flushFilePath() with fix for Windows

02458aa

Merge branch 'develop' into jb/flushFilePath-with-fix-for-windows

bf78cf3

Merge branch 'develop' into jb/flushFilePath-with-fix-for-windows

b172579

gbockus-sf approved these changes Aug 16, 2022

View reviewed changes

jeffb-sfdc merged commit 1c52c54 into develop Aug 16, 2022

jeffb-sfdc deleted the jb/flushFilePath-with-fix-for-windows branch August 16, 2022 15:36

mingxuanzhangsfdx mentioned this pull request Mar 15, 2024

fix: support folder with symbol link #5507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: flushFilePath() with fix for Windows #4362

fix: flushFilePath() with fix for Windows #4362

jeffb-sfdc commented Aug 11, 2022

jeffb-sfdc commented Aug 11, 2022 •

edited

Loading

jeffb-sfdc Aug 11, 2022 •

edited

Loading

klewis-sfdc Aug 11, 2022

jeffb-sfdc Aug 11, 2022

jeffb-sfdc Aug 11, 2022

klewis-sfdc Aug 11, 2022

gbockus-sf Aug 11, 2022

jeffb-sfdc Aug 11, 2022

jeffb-sfdc Aug 11, 2022

gbockus-sf Aug 11, 2022

jeffb-sfdc Aug 11, 2022

klewis-sfdc Aug 11, 2022

jeffb-sfdc Aug 11, 2022

gbockus-sf Aug 11, 2022

klewis-sfdc Aug 11, 2022

jeffb-sfdc Aug 11, 2022 •

edited

Loading

gbockus-sf Aug 11, 2022

gbockus-sf Aug 11, 2022

gbockus-sf Aug 11, 2022

jeffb-sfdc Aug 11, 2022 •

edited

Loading

gbockus-sf Aug 11, 2022

jeffb-sfdc Aug 11, 2022

klewis-sfdc Aug 11, 2022

klewis-sfdc Aug 11, 2022

klewis-sfdc left a comment

gbockus-sf left a comment

klewis-sfdc commented Aug 11, 2022

gbockus-sf left a comment


		const result = flushFilePath(filePath);

		expect(realpathSyncNativeStub.called).to.equal(true);


		expect(result).to.equal(filePaths);

		realpathSyncNativeStub.restore();

fix: flushFilePath() with fix for Windows #4362

fix: flushFilePath() with fix for Windows #4362

Conversation

jeffb-sfdc commented Aug 11, 2022

What does this PR do?

What issues does this PR fix or reference?

Functionality Before

Functionality After

jeffb-sfdc commented Aug 11, 2022 • edited Loading

jeffb-sfdc Aug 11, 2022 • 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

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

jeffb-sfdc Aug 11, 2022 • 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

jeffb-sfdc Aug 11, 2022 • 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

Choose a reason for hiding this comment

klewis-sfdc left a comment

Choose a reason for hiding this comment

gbockus-sf left a comment

Choose a reason for hiding this comment

klewis-sfdc commented Aug 11, 2022

gbockus-sf left a comment

Choose a reason for hiding this comment

jeffb-sfdc commented Aug 11, 2022 •

edited

Loading

jeffb-sfdc Aug 11, 2022 •

edited

Loading

jeffb-sfdc Aug 11, 2022 •

edited

Loading

jeffb-sfdc Aug 11, 2022 •

edited

Loading