Get-Content throws improved error when targeting a container #7797

kvprasoon · 2018-09-16T19:34:46Z

PR Summary

Fix #7770
Fix #2686

Enhancing the error message as per #7770 and #2686

PR Checklist

Fix for Issue #7770

CodeFactor fix

iSazonov

I think the fix should be in GetContentReaders() or even in ResolvePaths().

iSazonov · 2018-09-17T06:16:51Z

test/powershell/Modules/Microsoft.PowerShell.Management/Get-Content.Tests.ps1

+
+    It "Should throw error on a directory with specific message" {
+        { Get-Content . -ErrorAction Stop } |
+            Should -Throw "Unable to access '.' contents as it is a container. Please use 'Get-ChildItem .' instead."


We never test a message because of localization. Please remove.

Sure, will remove the tests, and it was not put in a new BeginProcessing() or GetContentReadres() as we already have other check for verifying Tail and Header coexistence, So I thought to put it before GetContentReadres().

iSazonov · 2018-09-17T07:33:36Z

Please look Set-Content. We have access denied there for a container too. I believe we should consider the fix in ResolvePaths().

Enhanced error message for Get-Content Set-Content Clear-Content Add-Content

CodeFactor fix

iSazonov · 2018-09-18T04:24:15Z

@kvprasoon it seems more complicated than it seemed at first. The *-Content cmdlets do path expansion and we should test directory existence after the wilcard expansion. Complexity is that these cmdlets use different paths for path expansion. So I suggest exclude Clear-Content and fix it in new PR.

Reverting Clear-content changes as this will be done in a seperate PR.

kvprasoon · 2018-09-19T04:37:59Z

@iSazonov I think CI failures are not related to current code changes.

iSazonov

Please resolve a merge conflict.

iSazonov · 2018-09-19T08:05:09Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ContentCommandBase.cs

+                    string errMsg = StringUtil.Format(SessionStateStrings.GetContainerContentException, path);
+                    ErrorRecord error = new ErrorRecord(new InvalidOperationException(errMsg), "GetContainerContentException", ErrorCategory.InvalidOperation, null);
+                    WriteError(error);
+                    return results;


Below we call SessionState.Path.GetResolvedPSPathFromPSPath() which can return array of paths including containers.
I'd consider line 614 to the fix.

The issue is provider specific (basically any provider that implements IContentCmdletProvider). Looking in our code base, it seems the only public provider that's affected is the FileSystemProvider. The fix should be in in this method.

Then GetContentWriter should be fixed too.

@SteveL-MSFT fix in that method will address only for Get-Content cmdlet. How about Set/Add/Clear-Content cmdlets ? Do we need to make PRs for each of those cmdelts ?

@kvprasoon yes, we should fix those at the same time if possible as I imagine the change to be very similar. I haven't looked at the code closely to see if there's a single point to make the change, but I suspect probably not.

Single method what is identified is ResolvePaths() for *-Content cmdlets except for Clear-Content

ResolvePaths() in ContentCommandBase.cs would not be the right place as we'd specialize that generic code path that calls into providers to a specific provider.

iSazonov · 2018-09-19T12:16:39Z

src/Microsoft.PowerShell.Commands.Management/commands/management/GetContentCommand.cs

Please fix the line.

iSazonov · 2018-09-19T12:18:39Z

test/powershell/Modules/Microsoft.PowerShell.Management/Add-Content.Tests.ps1

    }

+    It "should throw 'GetContainerContentException' when -Path is a container" {
+      { Add-Content -Path . -Value "GetContainerContentException" -ErrorAction Stop } | Should -Throw -ErrorId "GetContainerContentException,Microsoft.PowerShell.Commands.AddContentCommand"


For "Add" in cmdlet name the "GetContainerContentException" does not look very good.

mklement0 · 2018-09-19T13:59:52Z

src/System.Management.Automation/resources/SessionStateStrings.resx

    <value>The dynamic parameters for the GetContentWriter operation cannot be retrieved for the '{0}' provider for path '{1}'. {2}</value>
  </data>
+  <data name="GetContainerContentException" xml:space="preserve">
+    <value>Unable to access '{0}' contents as it is a container. Please use 'Get-ChildItem {0}' instead.</value>


Thanks for taking this on. Given that this message is specific to the filesystem provider, I suggest using directory instead of container for clarity. Perhaps something like the following?

'{0}' is a directory, not a file. To list the items contained in a directory, use Get-ChildItem.

@mklement0 sounds good, will change it the container part. I believe giving Get-ChildItem <the input path> in error message will be more helpful than just giving the cmdlet name to use.

your thoughts ?

Thanks.

I personally think it's cleaner (less noisy) without the path, and is generally better to be concise in error message - without leaving out vital information, obviously, which is always a balancing act.

In this particular case, if the user managed to call Get-Content someDir and ran into the error, I think they 'll be able to infer from mentioning only the name of the cmdlet how to apply it analogously.

I don't feel strongly about this, however.

@SteveL-MSFT, what do you think?

This is provider specific issue and it appears only the FileSystemProvider supports IContentCmdletProvider. I think the error string should be in FileSystem Provider resx and I'm fine with @mklement0's suggested error message.

SteveL-MSFT · 2018-09-19T16:36:50Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ContentCommandBase.cs

+                    string errMsg = StringUtil.Format(SessionStateStrings.GetContainerContentException, path);
+                    ErrorRecord error = new ErrorRecord(new InvalidOperationException(errMsg), "GetContainerContentException", ErrorCategory.InvalidOperation, null);
+                    WriteError(error);
+                    return results;


The issue is provider specific (basically any provider that implements IContentCmdletProvider). Looking in our code base, it seems the only public provider that's affected is the FileSystemProvider. The fix should be in in this method.

SteveL-MSFT · 2018-09-19T16:38:56Z

src/System.Management.Automation/resources/SessionStateStrings.resx

    <value>The dynamic parameters for the GetContentWriter operation cannot be retrieved for the '{0}' provider for path '{1}'. {2}</value>
  </data>
+  <data name="GetContainerContentException" xml:space="preserve">
+    <value>Unable to access '{0}' contents as it is a container. Please use 'Get-ChildItem {0}' instead.</value>


This is provider specific issue and it appears only the FileSystemProvider supports IContentCmdletProvider. I think the error string should be in FileSystem Provider resx and I'm fine with @mklement0's suggested error message.

kvprasoon · 2018-09-19T20:54:44Z

Closing this PR, need to rebase my fork and will do the changes in different PRs.
Thanks everyone for the review comments.

Fix for Issue #7770

b1b0f7d

Fix for Issue #7770

kvprasoon requested review from adityapatwardhan and daxian-dbw as code owners September 16, 2018 19:34

CodeFactor fix

95641aa

CodeFactor fix

iSazonov reviewed Sep 17, 2018

View reviewed changes

kvprasoon added 2 commits September 17, 2018 23:28

Enhancing error message for *-Contnet cmdlets.

24a706f

Enhanced error message for Get-Content Set-Content Clear-Content Add-Content

CodeFactor fix

4d2f454

CodeFactor fix

kvprasoon added 2 commits September 18, 2018 23:28

Merge remote-tracking branch 'upstream/master'

8cd7f7f

Reverting Clear-content changes as per the comments

f800ca4

Reverting Clear-content changes as this will be done in a seperate PR.

iSazonov reviewed Sep 19, 2018

View reviewed changes

kvprasoon mentioned this pull request Sep 19, 2018

Add support for 'cd +' #7206

Merged

11 tasks

iSazonov reviewed Sep 19, 2018

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/GetContentCommand.cs

return;

}

Copy link

Collaborator

iSazonov Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the line.

iSazonov reviewed Sep 19, 2018

View reviewed changes

mklement0 reviewed Sep 19, 2018

View reviewed changes

SteveL-MSFT changed the title ~~Fix for Issue #7770~~ Get-Content throws improved error when targeting a container Sep 19, 2018

SteveL-MSFT requested changes Sep 19, 2018

View reviewed changes

kvprasoon closed this Sep 19, 2018

Get-Content throws improved error when targeting a container #7797

Get-Content throws improved error when targeting a container #7797

Uh oh!

Conversation

kvprasoon commented Sep 16, 2018 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

iSazonov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvprasoon Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 17, 2018

Uh oh!

iSazonov commented Sep 18, 2018

Uh oh!

kvprasoon commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvprasoon Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklement0 Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvprasoon commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kvprasoon commented Sep 16, 2018 •

edited by iSazonov

Loading

iSazonov left a comment •

edited

Loading

kvprasoon Sep 17, 2018 •

edited

Loading

kvprasoon commented Sep 19, 2018 •

edited

Loading

iSazonov Sep 19, 2018 •

edited

Loading

kvprasoon Sep 19, 2018 •

edited

Loading

mklement0 Sep 19, 2018 •

edited

Loading

kvprasoon commented Sep 19, 2018 •

edited

Loading