Skip to content

Fix retrieving dynamic parameters from provider even if globbed path returns no results #15525

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 14, 2021
Merged

Fix retrieving dynamic parameters from provider even if globbed path returns no results #15525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c1ebf5
Fix retrieving dynamic parameters from provider even if globbed path …
SteveL-MSFT Jun 4, 2021
a6a7620
Add comment to test to clarify what is being tested
SteveL-MSFT Jun 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/System.Management.Automation/engine/SessionStateContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4681,7 +4681,7 @@ internal object CopyItemDynamicParameters(
}
}

if (providerPath != null)
if (providerInstance != null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was very wonder to read comments in the method above.
Is the method used for both FromSession and ToSession?
If so I wonder again because obviously the method is only for FromSession scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is independent from ToSession and FromSession, it applies to all dynamic parameters

Copy link
Collaborator

Choose a reason for hiding this comment

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

I say that original design was broken - it makes no sense to check the path existence if it is on remote.
Next question is does it make sense to check the existence for local path? If it even makes sense this is the wrong place for it - all we need is the providerInstance as you said.

Do we really need globbing? I suggest to make minor breaking change and use GetUnresolvedProviderPathFromPSPath() or GetProviderPath().

Then if providerInstance is null then pathNotFoundOnClient is always true and we always effectively fall back to FileSystemProvider:

        internal object CopyItemDynamicParameters(
            string path,
            string destination,
            bool recurse,
            CmdletProviderContext context)
        {
            if (path is null)
            {
                return null;
            }

            ProviderInfo provider = null;
            string providerPath = null;
            try
            {
                providerPath =
                    context.ExecutionContext.SessionState.Path.GetUnresolvedProviderPathFromPSPath(
                        path,
                        context,
                        out provider,
                        out _);
            }
            catch (DriveNotFoundException)
            {
                // This exception is expected for remote sessions where drives exist in a remote session but not
                // on the client.
            }
            catch (ItemNotFoundException)
            {
                // This exception is expected for remote sessions where item exist in a remote session but not
                // on the client.
            }

            if (provider is null)
            {
                // At this point, we don't know if this is a remote use of copy-item because the FromSession
                // and ToSession dynamic parameters have not been retrieved yet.
                // Ignore these exceptions and use the FileSystem provider as default.  If this is a real drive
                // issue, or if the item does not exist, it will be caught later when the drive or item path is used.
                var fileSystemProviders = Providers["FileSystem"];
                if (fileSystemProviders.Count > 0)
                {
                    providerPath = path;
                    provider = fileSystemProviders[0];
                }
            }

            if (provider is null)
            {
                return null;
            }

            CmdletProvider providerInstance = ExecutionContext.EngineSessionState.GetProviderInstance(provider);
            return providerInstance is null ? null : CopyItemDynamicParameters(providerInstance, providerPath, destination, recurse, context);
        }

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Jun 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure what the original design intent was, but the path(s) after globbing are passed down to the provider for dynamic parameters (so null is now possible where it wasn't before, but it's an assert so could have happened in retail builds anyways), so presumably a provider could use that information and we should probably not make that breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the path(s) after globbing are passed down to the provider for dynamic parameters

so presumably a provider could use that information

This just makes me think that it's much more useful to pass the original path than after globbing, otherwise in CopyItemDynamicParameters there's no way to know that globbing was used. I believe it is more right design and the breaking change is under bracket 3 (unlikely breaking change).

Although if you find it totally unacceptable, it's worth cleaning up the code anyway, like in my sample.

so null is now possible where it wasn't before

providerPath argument is never null in CopyItemDynamicParameters().

Copy link
Member Author

Choose a reason for hiding this comment

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

Not providerPath, but just path. I think if provider authors would find value in the original search path, we would have heard about it. I don't think it's worth making this change.

Copy link
Collaborator

@iSazonov iSazonov Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Not providerPath, but just path.

I don't understand. The method starts with:

            if (path == null)
            {
                return null;
            }

I think if provider authors would find value in the original search path, we would have heard about it. I don't think it's worth making this change.

I guess nobody uses this :-) I mainly think that the binder is already too slow and we could make it faster.
Moreover, we have already made a breaking change by passing a non-existent file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this is code seems wrong and that parameter binding is not the right place to do path validation. My guess is this code was here when copy-item was local only, and then adjusted when copy-item was extended to support remote copy.

I feel it is worth considering a breaking change to re-review this and possibly fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer to take a small change now to improve the user experience (they would get an error from the provider instead of the parameter binder not helping them understand why it didn't work) and defer any potential breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is reasonable. Can you create an issue to track it?

{
// Get the dynamic parameters for the first resolved path
return CopyItemDynamicParameters(providerInstance, providerPath, destination, recurse, newContext);
Expand Down Expand Up @@ -4733,10 +4733,6 @@ private object CopyItemDynamicParameters(
providerInstance != null,
"Caller should validate providerInstance before calling this method");

Dbg.Diagnostics.Assert(
path != null,
"Caller should validate path before calling this method");

Dbg.Diagnostics.Assert(
context != null,
"Caller should validate context before calling this method");
Expand Down
8 changes: 8 additions & 0 deletions test/powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,14 @@ Describe "Parameter Binding Tests" -Tags "CI" {
DynamicParamTest -PipelineVariable bar | ForEach-Object { $bar } | Should -Be "hi"
}

It 'Dynamic parameter is found even if globbed path does not exist' {
$guid = New-Guid

# This test verifies that the ErrorRecord is coming from parameter validation on a dynamic parameter
# instead of an error indicating that the dynamic parameter is not found
{ Copy-Item "~\$guid*" -Destination ~ -ToSession $null } | Should -Throw -ErrorId 'ParameterArgumentValidationError'
}

Context "PipelineVariable Behaviour" {

BeforeAll {
Expand Down