Set-SecretInfo needs different error if the exception came from the called script

[JustinGrote]

Currently if an unhandled terminating exception occurs in Set-SecretInfo, it emits the following message: Cannot set secret metadata PesterSecret2. Vault PESTER-MultipleEntries does not support secret metadata.

It should only emit this message if the module does not implement the function in its exported functions. If it is implemented, this should instead say: There was an unexpected error when setting secret metadata PesterSecret2 in Vault PESTER-MultipleEntries. The details can be found in the InnerException

[PaulHigin]

Set-SecretInfo is a special case, because it is a command that is optionally supported by an extension vault. A terminating error usually means that the extension vault does not support meta data. I don't want to include an obscure error message that might confuse the user.

So in this case I feel the extension vault Set-SecretInfo command should avoid throwing terminating errors and instead just use Write-Error to communicate errors to the user. In fact, I feel all extension vault commands should refrain from throwing terminating errors (exceptions) and just use Write-Error, even though I do surface a terminating error in required extension vault command cases.

This should be made more clear in the documentation, and I'll change this to a document bug.

[JustinGrote]

@paulhigin As you mentioned terminating errors are for truly exceptional errors, I was thinking things like .NET method exceptions and whatnot that are bugs. If this happened to a user they would get the "not supported" message and assume it was intentional and stop using the module rather than filing an issue.

As a workaround I suppose you can trap everything to a non-terminating error, I'm doing that right now anyways.

[PaulHigin]

Yes, I feel that extension vault command implementations should never throw. That may mean using try/catch or a trap, but these commands should only ever report errors and never throw. SecretManagement would only catch and report the errors anyway, but the extension vault implementations have better context and should do the reporting instead.

[JustinGrote]

Shouldn't the code express that then? If a terminating exception is sent from a extension vault have it be all stop and say "a module threw a terminating exception which it should never do" rather than " this function is not supported"?

Again, just concerned about the user experience.

[PaulHigin]

Again, SecretManagement does this (captures and reports exceptions) for most of the extension vault commands, but Set-SecretInfo metadata is special since it is an optional command and not required. Most vaults won't support it, so the assumption is that the command is not supported when an exception is thrown.

But in general, the extension vault command implementations should never throw and instead should catch and report any unexpected errors as you show above.

[JustinGrote]

@PaulHigin I see your logic, I disagree that just because it's optional to implement doesn't mean it shouldn't report to the user in a similar way so the user doesn't just silently drop using it when their use case probably could be fixed with an issue (this is assuming the vault extension DOES implement the command), but it's not a major enough deal to go back and forth any further on it.

Thanks for your input and your time responding, I appreciate it!

[PaulHigin]

Thanks Justin. I would like to keep this open because I feel it is an important extension vault implementation detail that should be documented (doc bug). I am basically pushing back and saying extension vault commands should report and not throw. This is consistent with the current design that gives extension vaults much more autonomy interacting with users.

[JustinGrote]

@PaulHigin I agree that's what they should do, but there are circumstances where that may not happen, and will lead to confusion given the current implementation

Here's an example:

In my vault lets say I call .NET method getSuperSecret(), and it throws a method exception that for whatever reason I didn't handle (or lets say I did try to handle it with Trap() but it has a special error message that breaks my trap, as a bad example). This exception only shows up on a particular user's computer and so my tests didn't catch it, but when it happens on the user computer, they get a message saying "SecretInfo not supported", thus they never report it to me as an issue and never use my feature and I have no idea it is a problem unless I run telemetry on my module.

Short of fixing it in code, I think providing documentation on at least adding a basic trap to all your module commands would be a good start.

trap {
  Write-Error $PSItem
  return $false
}

[PaulHigin]

I definitely see your point. But yes, I am saying the extension vault needs to handle and report the exception instead of SecretManagement.