Throw error on entering non-existing type in Get-FormatData #7434

faraazahmad · 2018-08-02T06:06:31Z

Fix #4235

PR Summary

Write non-terminating error when Get-FormatData doesn't found a type definition for the provided type,

PR Checklist

msftclas · 2018-08-02T06:06:42Z

All CLA requirements met.

iSazonov · 2018-08-02T07:12:20Z

....PowerShell.Commands.Utility/commands/utility/FormatAndOutput/common/GetFormatDataCommand.cs

@@ -169,27 +171,41 @@ protected override void ProcessRecord()
                viewList.Add(formatdef);
            }// foreach(ViewDefinition...

-            // write out all the available type definitions
-            foreach (var pair in typedefs)
+            // If no Type Definitions are found for _typename throw a terminating error


Please remove obvious comment.

iSazonov · 2018-08-02T07:13:08Z

....PowerShell.Commands.Utility/commands/utility/FormatAndOutput/common/GetFormatDataCommand.cs

+            }
+            else 
+            {
+                // write out all the available type definitions


Please remove obvious comment.

iSazonov · 2018-08-02T07:14:53Z

....PowerShell.Commands.Utility/commands/utility/FormatAndOutput/common/GetFormatDataCommand.cs

-                var typeNames = pair.Key;
-
-                if (writeOldWay)
+                // All files must have the same extension otherwise throw.


Here the comment is unneeded. I believe we should remove it. (Or move to the method description).

iSazonov · 2018-08-02T07:16:18Z

....PowerShell.Commands.Utility/commands/utility/FormatAndOutput/common/GetFormatDataCommand.cs

@@ -4,6 +4,8 @@
 using System;
 using System.Linq;
 using System.Management.Automation;
+using System.Management.Automation.Internal;
+


Please alphabetically resort all using-s.

Isn't this already sorted?

We have other using-s below.

It is minor comment.

iSazonov · 2018-08-02T07:28:42Z

Seems we could use TypeLoadException

faraazahmad · 2018-08-02T08:12:54Z

Some of those obvious comments were already there, so I didn't mess with them.

iSazonov

@faraazahmad Thanks for you contribution!

iSazonov · 2018-08-02T10:49:49Z

@mklement0 Is the fix good for you?

SteveL-MSFT · 2018-08-02T16:00:29Z

....PowerShell.Commands.Utility/commands/utility/FormatAndOutput/common/GetFormatDataCommand.cs

-                if (writeOldWay)
+                ErrorRecord errorRecord = new ErrorRecord(
+                    new TypeLoadException("No such type could be found"),
+                    "SPECIFIED_TYPE_NOT_FOUND",


For the FullQualifiedErrorId, the convention is to use Pascal casing: SpecifiedTypeNotFound.

Good point, @SteveL-MSFT.

Additionally, it should be a non-terminating error, given that the cmdlet accepts multiple inputs and potentially produces multiple outputs.

@faraazahmad: I know I suggested ResourceUnavailable initially, but on further reflection it probably isn't the right category.

We can use Remove-TypeData as a model, which already reports an error (also non-terminating), and uses category InvalidOperation:

PS> Remove-TypeData NoSuchType Remove-TypeData : Error in TypeData "NoSuchType": The type "NoSuchType" was not found. The type name value must be the full name of the type. Verify the type name and run the command again. At line:1 char:1 + Remove-TypeData NoSuchType + ~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : InvalidOperation: (:) [Remove-TypeData], RuntimeException + FullyQualifiedErrorId : TypesDynamicRemoveException,Microsoft.PowerShell.Commands.RemoveTypeDataCommand

As an aside: I'm not sure TypesDynamicRemoveException was a great choice of error ID.

@SteveL-MSFT, @iSazonov, what do you think?

Yes, this should be a non-terminating error if multiple are accepted so no need to stop the entire operation. Agree that TypesDynamicRemoveException is a bad error id, but it's a breaking change to change it now as the intent of the FQEI is to make it internet searchable. For symmetry, it makes sense to use InvalidOperation and probably makes more sense for an end user whereas TypeLoadException is more dev-centric.

iSazonov · 2018-08-06T05:44:51Z

@SteveL-MSFT I added new commit to address your comment, please update your review.

dantraMSFT · 2018-08-06T20:48:41Z

....PowerShell.Commands.Utility/commands/utility/FormatAndOutput/common/GetFormatDataCommand.cs

-
-                if (writeOldWay)
+                ErrorRecord errorRecord = new ErrorRecord(
+                    new TypeLoadException("No such type could be found"),


The exception message string should come from the resource file; UtilityCommonStrings.resx is probably the best location.

iSazonov · 2018-08-08T05:24:50Z

@SteveL-MSFT @dantraMSFT Please update your review.

SteveL-MSFT · 2018-08-08T16:30:44Z

src/Microsoft.PowerShell.Commands.Utility/resources/GetFormatDataStrings.resx

+    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
+  </resheader>
+  <data name="SpecifiedTypeNotFound" xml:space="preserve">
+    <value>Cannot find the type.</value>


Should probably be something like:

The type name '{0}' could not be found.

dantraMSFT

Other than Steve's recommendation about including the type name in the error message string, LGTM

Jaykul · 2018-08-09T16:20:08Z

HANG ON ... This is a breaking change to the function!!!

Does -ErrorAction SilentlyContinue work, or are you now forcing everyone to wrap this in a try/catch block?

SteveL-MSFT · 2018-08-09T16:29:23Z

@Jaykul this is a non-terminating error

Jaykul · 2018-08-10T21:52:58Z

@SteveL-MSFT it still breaks code like this, right?

if(!(Get-FormatData $Type)) {
   Update-FormatData -TypeName $Type ...
}

I mean, in the sense that we now get an error where there was none, even though the user had deliberately handled the situation correctly the way the cmdlet worked before.

Are we only doing this because we think nobody uses it, or would you accept a PR for Get-Module too?

This command follows the exact same semantics as Get-Module and may return multiple entries even without a wildcard. That's why it returns silently when it finds nothing.

mklement0 · 2018-08-11T02:55:29Z

@Jaykul:

Yes, for someone who relied on the previous de facto behavior, this would be a breaking change.

That in itself may be reason enough to not make the change - I have no sense of and have made no attempt to assess the real-world impact. What do you think the real-world impact is?

On the other end of the spectrum: any bug fix is a breaking change: what used to break / work nonsensically before now no longer does.

But let us be clear re:

deliberately handled the situation correctly

Correctly in this situation means:

inferred from the de facto behavior
with no supporting evidence from the documentation
despite the behavior being inconsistent with that of other cmdlets.

And, indeed, the same applies to the current behavior of Get-Module: with a non-wildcard argument it should not default to quietly outputting "nothing" ([System.Management.Automation.Internal.AutomationNull]::Value)

or would you accept a PR for Get-Module too?

In that vein: either yes, or document the surprising existing behavior - see #7498

As an aside:

may return multiple entries even without a wildcard.

Unless you're referring to Get-Module's -ListAvailable switch - an explicit opt-in to multiple matches - neither Get-Module nor Get-FormatData return multiple matches with non-wildcard arguments.

Jaykul · 2018-08-12T04:37:11Z

It's ironic that I'm the one arguing this -- I argued rather passionately against the no-output, no-error design pattern years ago in a discussion on the PowerShell MVP mailing list! Although at the time, my argument was specifically against commands that behave differently when you have a simple name, versus a wildcard (thus allowing the possibility of multiple items).

Ultimately I was rather deluged with examples from the broader ecosystem (Exchange, SCSM, etc.) showing examples of commands quietly returning nothing when they could have returned many things. My point is that this is how PowerShell commands behave, in general:

If the command can return MULTIPLE items, then when there is nothing to return, they just return nothing. That's a deliberate design choice.

And yes, you're wrong @mklement0, when you say that those commands don't return multiple items with simple parameters. Here are some concrete examples.

# There are about *eight* default rules that affect ScriptInfo, for example:
Get-FormatData System.Management.Automation.ScriptInfo

# Users can have multiple versions of the same module imported because of dependencies.
# We can import multiple versions of Pester as an example...
# (most of us have 3.4.0 and a newer one, right?):
Get-Module Pester -ListAvailable | Import-Module
Get-Module Pester

But it's not just those, most of the default Get commands do this if they could return multiple items:

During the first command of your session, there's no history (or, do `Clear-History; Get-History`)

Get-History

When a folder is empty

mkdir emptyfolder
Get-ChildItem .\emptyfolder

When there's no input:

Get-Unique
# Or even ...
Get-ChildItem .\EmptyFolder\ | Get-Unique
# Or even ...
Get-ChildItem .\EmptyFolder\ | Get-Random

When there are no sessions

Get-PSSession

When there are no commands

Get-Command -Verb Build

When there's no TypeData:

Get-TypeData System.Boolean

When there's no Jobs:

Get-Job

When there's no scheduled jobs

Get-ScheduledJob

When the clipboard is empty, as after KeePass

Set-Clipboard $null 
Get-Clipboard

I trust I don't need to go through and test every Get command to make the point?

mklement0 · 2018-08-12T05:09:38Z

@Jaykul:

It's ironic that I'm the one arguing this

I hear you, but let me ask you this before I address your examples:

Despite your earlier misgivings, do all the behaviors you list now make sense to you?
Do you believe that all of them apply directly to the case at hand?

Jaykul · 2018-08-13T15:04:13Z

They make sense:

They are returning what's there.
They are consistent to the principle that if the command can return multiple items, in any particular case where there is nothing to return, they return that -- without error.

Obviously in a lot of these cases, it's very clear that there's not an error. That is (if you understood the situation), you didn't actually expect there to be any child items, history, jobs, content, sessions, etc...

I think all of them are examples of the rule, and serve to show that this cmdlet was correct without the PR.

For what it's worth:

If we were willing to break the backward compatibility of PowerShell, then I would be happy to argue again this design pattern and try to convince the world that all commands should have the same output behavior regardless of how many items they might output:

Users (and developers) can't always tell when this will be the case.
Although it's a "common" design pattern, it's clearly not well-known or documented.

But in reality, changing this behavior across all the built-in commands would break too much, and there's no way to actually "fix" it because too many commands are outside the control of this team. We should therefore not alter the expectations of design, and thus, should not change commands.

faraazahmad · 2018-08-14T13:41:49Z

I have to agree with @Jaykul , this makes sense. For example, if I ls an empty directory, the expected behaviour would be to not print anything, and not throw an error.

mklement0 · 2018-08-14T15:22:29Z

The issue is not about output behavior, so what I should've said earlier is: unless you explicitly opt in with switches such as -ListAvailable, specifying a literal name targets a specific, single entity (and how many objects are output as a result is incidental).

The issue is that if you're asking for a specific entity by its literal name (as opposed to a wildcard pattern), not finding that entity should be an error.

(By contrast, a wildcard pattern quietly producing no output if no matches are found makes sense (Get-FormatData *NoSuch*), because instead of saying "give me entity x" you're saying "give me whatever entities happen to match this pattern, if any".)

This is how it currently works in most cmdlets, Get-Module and Get-TypeData being the only other exceptions that I'm aware of - and they should be fixed as well, or - if that is considered too breaking a change - their aberrant behavior should be documented; I've created an issue for Get-TypeData as well: #7521

To quote @BrucePay from the Get-Module issue that I've opened:

As a matter of convention, we usually generate an error for a non-wildcard query e.g. get-item nosuchitem returns a error, but get-module nosuchmodule quietly returns nothing.

None of @Jaykul's other examples therefore apply:

Targeting no specific entity (e.g., Get-History) is a different use case: you're asking for existing entities to be enumerated.
As for Get-ChildItem .\emptyfolder: the named specific entity is found, and it just so happens that the information about that entity results in no output, but, as stated, that is incidental. What matters is that if the entity is not found, an error is reported: Get-ChildItem NoSuchDir

mklement0 · 2018-08-14T16:37:47Z

@SteveL-MSFT:

If this fix is ultimately kept, I suggest amending the error message in 2 ways:

Make it clear that what couldn't be found wasn't a type per se, but that type's formatting data loaded into the current session.
Given the need to to always specify the full type name (that is, you cannot omit the System. component and you cannot use type accelerators), the error message should provide a hint to that effect.

Therefore, instead of The type name '{0}' could not be found., perhaps we could say:

No formatting data for type name '{0}' has been loaded into the current session.
Note that type names must be specified exactly as defined and are typically full .NET type names.

SteveL-MSFT · 2018-08-16T18:15:10Z

@PowerShell/powershell-committee discussed this and we also discussed it in this morning's Community Call. We agreed that the intent is that if a literal is passed and is not found, it should return a non-terminating error. Cmdlets that don't have this behavior have bugs. In cases where it's explicit or implicit filtering, it should return nothing. We also agreed that we should revert this change and revisit making cmdlets consistent in 6.2. Finally, the real issue is that we are missing Test-* cmdlets so users had to rely on current behavior with Get-* cmdlets to see if something exists and we should add those cmdlets in the future.

Jaykul · 2018-08-16T18:52:57Z

I think you're going to break so very many things -- that 6.2 needs to be 7.0 to be semantic

essentialexch · 2018-08-16T21:41:46Z

Lord have mercy... once you do this, WPS and CPS are completely different languages that just happen to be related. Forget about moving scripts between the two.

joeyaiello · 2018-08-16T21:51:31Z

I heard today in the @PowerShell/powershell-committee Community Call that we're going to revert this and work towards consistency and Test-* cmdlets across the board.

@SteveL-MSFT if we agree that's the case, will the revert make RC? GA?

SteveL-MSFT · 2018-08-16T21:53:51Z

The @PowerShell/powershell-maintainers will be reverting this commit, but that won't happen for RC where we are already in process of release but will make it for GA. @iSazonov if you are available, can you revert this as the other maintainers are working on the release?

…owerShell#7434)" This reverts commit b754cd8.

…7434)" (#7546) Revert "Throw error on entering non-existing type in Get-FormatData (#7434) This reverts commit b754cd8. Per this comment #7434 (comment)

iSazonov · 2018-08-17T03:44:05Z

@SteveL-MSFT The commit already reverted by @TravisEz13

Have we a tracking issue to address the PowerShell Committee conclusion about Get-* and Test-* cmdlets.

SteveL-MSFT · 2018-08-17T16:16:36Z

@iSazonov The discussion is continuing in #7498. I've created #7562 for new Test- cmdlets

faraazahmad · 2018-08-17T17:17:27Z

Didn't know my first contribution to Powershell could stir up so back and forth 😅. Is it safe to delete the branch?

SteveL-MSFT · 2018-08-17T17:29:01Z

@faraazahmad I think it's a healthy discussion. Yes, you can delete the branch.

faraazahmad added 3 commits August 1, 2018 01:23

Get-FormatData throws no such type error

9b3b652

throw error on condition

0928b54

add test for showing error on Get-FormatData

14300ba

faraazahmad requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners August 2, 2018 06:06

faraazahmad added 2 commits August 2, 2018 11:41

fix codefactor ussies

c050a04

fix codefactor issues PowerShell#2

8f93544

iSazonov reviewed Aug 2, 2018

View reviewed changes

remove unnecessary comments

dfebcd7

iSazonov approved these changes Aug 2, 2018

View reviewed changes

iSazonov requested a review from SteveL-MSFT August 2, 2018 10:50

iSazonov self-assigned this Aug 2, 2018

faraazahmad added 2 commits August 2, 2018 20:17

sort using-s alphabetically

26e6c84

put system using-s first

5beb054

SteveL-MSFT requested changes Aug 2, 2018

View reviewed changes

Address SteveL-MSFT's comment

2a2a708

dantraMSFT suggested changes Aug 6, 2018

View reviewed changes

Move message to resx

928ea10

SteveL-MSFT requested changes Aug 8, 2018

View reviewed changes

dantraMSFT approved these changes Aug 8, 2018

View reviewed changes

Use format string

f530406

iSazonov merged commit b754cd8 into PowerShell:master Aug 10, 2018

mklement0 mentioned this pull request Aug 11, 2018

Get-Module with a non-wildcard module name that doesn't match a loaded / available module quietly returns nothing #7498

Closed

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 11, 2018

faraazahmad deleted the getformat-error branch August 11, 2018 06:19

faraazahmad restored the getformat-error branch August 11, 2018 06:20

mklement0 mentioned this pull request Aug 14, 2018

Get-TypeData with a non-wildcard type name that matches no loaded type data quietly returns nothing #7521

Closed

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Aug 16, 2018

TravisEz13 added a commit to TravisEz13/PowerShell that referenced this pull request Aug 16, 2018

Revert "Throw error on entering non-existing type in Get-FormatData (P…

075bf72

…owerShell#7434)" This reverts commit b754cd8.

TravisEz13 mentioned this pull request Aug 16, 2018

Revert "Throw error on entering non-existing type in Get-FormatData (#7434) #7546

Merged

11 tasks

faraazahmad deleted the getformat-error branch August 18, 2018 06:38

Jaykul mentioned this pull request Aug 21, 2018

When should you write an error if you have no output? PoshCode/PowerShellPracticeAndStyle#113

Closed

Throw error on entering non-existing type in Get-FormatData #7434

Throw error on entering non-existing type in Get-FormatData #7434

Conversation

faraazahmad commented Aug 2, 2018 • edited by iSazonov Loading

PR Summary

PR Checklist

msftclas commented Aug 2, 2018 • 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

iSazonov commented Aug 2, 2018 • edited Loading

faraazahmad commented Aug 2, 2018 • edited Loading

iSazonov left a comment

Choose a reason for hiding this comment

iSazonov commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantraMSFT left a comment

Choose a reason for hiding this comment

Jaykul commented Aug 9, 2018

SteveL-MSFT commented Aug 9, 2018

Jaykul commented Aug 10, 2018 • edited Loading

mklement0 commented Aug 11, 2018

Jaykul commented Aug 12, 2018 • edited Loading

If the command can return MULTIPLE items, then when there is nothing to return, they just return nothing. That's a deliberate design choice.

During the first command of your session, there's no history (or, do Clear-History; Get-History)

When a folder is empty

When there's no input:

When there are no sessions

When there are no commands

When there's no TypeData:

When there's no Jobs:

When there's no scheduled jobs

When the clipboard is empty, as after KeePass

mklement0 commented Aug 12, 2018

Jaykul commented Aug 13, 2018 • edited Loading

faraazahmad commented Aug 14, 2018

mklement0 commented Aug 14, 2018 • edited Loading

mklement0 commented Aug 14, 2018

SteveL-MSFT commented Aug 16, 2018

Jaykul commented Aug 16, 2018

essentialexch commented Aug 16, 2018

joeyaiello commented Aug 16, 2018

SteveL-MSFT commented Aug 16, 2018

iSazonov commented Aug 17, 2018 • edited Loading

SteveL-MSFT commented Aug 17, 2018

faraazahmad commented Aug 17, 2018 • edited Loading

SteveL-MSFT commented Aug 17, 2018

faraazahmad commented Aug 2, 2018 •

edited by iSazonov

Loading

msftclas commented Aug 2, 2018 •

edited

Loading

iSazonov commented Aug 2, 2018 •

edited

Loading

faraazahmad commented Aug 2, 2018 •

edited

Loading

Jaykul commented Aug 10, 2018 •

edited

Loading

Jaykul commented Aug 12, 2018 •

edited

Loading

During the first command of your session, there's no history (or, do `Clear-History; Get-History`)

Jaykul commented Aug 13, 2018 •

edited

Loading

mklement0 commented Aug 14, 2018 •

edited

Loading

iSazonov commented Aug 17, 2018 •

edited

Loading

faraazahmad commented Aug 17, 2018 •

edited

Loading