(GH-1655) Allow adapted resources to define the return kind for set and test

CHANGELOG.md

@@ -107,6 +107,22 @@ changes since the last release, see the [diff on GitHub][unreleased].
 
   </details>
 
+- Added the `return` property to the `set` and `test` operations in adapted resource manifests.
+  Resource authors can set it to `state` or `stateAndDiff` to declare what the adapter emits when
+  it invokes that operation for the adapted resource. Previously, every adapted resource inherited
+  the return kind from the manifest of its adapter. When an adapted resource manifest defines
+  `return` for an operation, DSC uses that value instead of the one in the adapter's manifest.
+  Adapted resources that don't define the property continue to inherit it from the adapter. The
+  override applies when the adapter is invoked for a single resource instance, which is the case
+  for adapters that set `inputKind` to `single`.
+
+  <details><summary>Related work items</summary>
+
+  - Issues: [#1655][#1655]
+  - PRs: [#1802][#1802]
+
+  </details>
+
 - Added what-if support to the MCP server tools. The `invoke_dsc_config` tool now accepts a
   `what_if` option for the `set` operation, and the `invoke_dsc_resource` tool accepts `what_if`
   for the `set` and `delete` operations. This mirrors the `--what-if` flag on the `dsc config set`,
@@ -2029,7 +2045,9 @@ Version `3.0.0` is the first generally available release of DSC.
 [#1557]: https://github.com/PowerShell/DSC/issues/1557
 [#1558]: https://github.com/PowerShell/DSC/issues/1558
 [#1562]: https://github.com/PowerShell/DSC/issues/1562
+[#1655]: https://github.com/PowerShell/DSC/issues/1655
 [#1729]: https://github.com/PowerShell/DSC/issues/1729
 [#1755]: https://github.com/PowerShell/DSC/issues/1755
 [#1758]: https://github.com/PowerShell/DSC/issues/1758
 [#1759]: https://github.com/PowerShell/DSC/issues/1759
+[#1802]: https://github.com/PowerShell/DSC/issues/1802

dsc/tests/discovery/dsc_adaptedResource.tests.ps1

@@ -54,4 +54,152 @@ Describe 'Tests for adapted resources' {
             }
         }
     }
+
+    Context 'Return kind for set and test' {
+        BeforeAll {
+            $desired = @{ one = 'desired' } | ConvertTo-Json -Compress
+        }
+
+        It 'Inherits the return kind of the adapter for <operation> when the adapted resource does not define it' -TestCases @(
+            @{ operation = 'test'; stateProperty = 'actualState' }
+            @{ operation = 'set'; stateProperty = 'afterState' }
+        ) {
+            param($operation, $stateProperty)
+
+            $out = dsc resource $operation -r Adapted/Three -i $desired 2>$TestDrive/error.txt | ConvertFrom-Json
+            $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Path $TestDrive/error.txt -Raw)
+            $out.$stateProperty.one | Should -BeExactly 'value3'
+        }
+
+        It 'Uses return kind state from the adapted resource manifest for test' {
+            $out = dsc -l debug resource test -r Adapted/ReturnState -i $desired 2>$TestDrive/error.txt | ConvertFrom-Json
+            $errorTxt = Get-Content -Path $TestDrive/error.txt -Raw
+            $LASTEXITCODE | Should -Be 0 -Because $errorTxt
+            $out.actualState.one | Should -BeExactly 'fromOperation'
+            $out.inDesiredState | Should -BeFalse
+            $out.differingProperties | Should -Be @('one')
+            $errorTxt | Should -BeLike "*Adapted resource 'Adapted/ReturnState' defines return kind 'state' for operation 'test'*"
+        }
+
+        It 'Uses return kind stateAndDiff from the adapted resource manifest for test' {
+            $out = dsc -l debug resource test -r Adapted/ReturnStateAndDiff -i $desired 2>$TestDrive/error.txt | ConvertFrom-Json
+            $errorTxt = Get-Content -Path $TestDrive/error.txt -Raw
+            $LASTEXITCODE | Should -Be 0 -Because $errorTxt
+            $out.actualState.one | Should -BeExactly 'fromOperation'
+            $out.inDesiredState | Should -BeFalse
+            $out.differingProperties | Should -Be @('fromAdapter')
+            $errorTxt | Should -BeLike "*Adapted resource 'Adapted/ReturnStateAndDiff' defines return kind 'stateAndDiff' for operation 'test'*"
+        }
+
+        It 'Uses return kind state from the adapted resource manifest for set' {
+            $out = dsc -l debug resource set -r Adapted/ReturnState -i $desired 2>$TestDrive/error.txt | ConvertFrom-Json
+            $errorTxt = Get-Content -Path $TestDrive/error.txt -Raw
+            $LASTEXITCODE | Should -Be 0 -Because $errorTxt
+            $out.beforeState.one | Should -BeExactly 'fromGet'
+            $out.afterState.one | Should -BeExactly 'fromOperation'
+            $out.changedProperties | Should -Be @('one')
+            $errorTxt | Should -BeLike "*Adapted resource 'Adapted/ReturnState' defines return kind 'state' for operation 'set'*"
+        }
+
+        It 'Uses return kind stateAndDiff from the adapted resource manifest for set' {
+            $out = dsc -l debug resource set -r Adapted/ReturnStateAndDiff -i $desired 2>$TestDrive/error.txt | ConvertFrom-Json
+            $errorTxt = Get-Content -Path $TestDrive/error.txt -Raw
+            $LASTEXITCODE | Should -Be 0 -Because $errorTxt
+            $out.beforeState.one | Should -BeExactly 'fromGet'
+            $out.afterState.one | Should -BeExactly 'fromOperation'
+            $out.changedProperties | Should -Be @('fromAdapter')
+            $errorTxt | Should -BeLike "*Adapted resource 'Adapted/ReturnStateAndDiff' defines return kind 'stateAndDiff' for operation 'set'*"
+        }
+
+        It 'Uses the return kind from the adapted resource manifest in a configuration' {
+            $config = @{
+                '$schema' = 'https://aka.ms/dsc/schemas/v3/bundled/config/document.json'
+                resources = @(
+                    @{
+                        name       = 'state and diff'
+                        type       = 'Adapted/ReturnStateAndDiff'
+                        properties = @{ one = 'desired' }
+                    }
+                )
+            } | ConvertTo-Json -Depth 5 -Compress
+            $out = dsc config test -i $config 2>$TestDrive/error.txt | ConvertFrom-Json
+            $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Path $TestDrive/error.txt -Raw)
+            $out.results[0].result.actualState.one | Should -BeExactly 'fromOperation'
+            $out.results[0].result.differingProperties | Should -Be @('fromAdapter')
+        }
+
+        It 'Inherits the return kind the adapter defines for <operation> when the adapted resource does not define it' -TestCases @(
+            @{ operation = 'test'; stateProperty = 'actualState'; diffProperty = 'differingProperties' }
+            @{ operation = 'set'; stateProperty = 'afterState'; diffProperty = 'changedProperties' }
+        ) {
+            param($operation, $stateProperty, $diffProperty)
+
+            $out = dsc -l debug resource $operation -r AdaptedReturn/Inherit -i $desired 2>$TestDrive/error.txt | ConvertFrom-Json
+            $errorTxt = Get-Content -Path $TestDrive/error.txt -Raw
+            $LASTEXITCODE | Should -Be 0 -Because $errorTxt
+            $out.$stateProperty.one | Should -BeExactly 'fromOperation'
+            $out.$diffProperty | Should -Be @('fromAdapter')
+            $errorTxt | Should -Not -BeLike "*Adapted resource 'AdaptedReturn/Inherit' defines return kind*"
+        }
+
+        It 'Prefers the return kind of the adapted resource over the one the adapter defines for <operation>' -TestCases @(
+            @{ operation = 'test'; stateProperty = 'actualState'; diffProperty = 'differingProperties' }
+            @{ operation = 'set'; stateProperty = 'afterState'; diffProperty = 'changedProperties' }
+        ) {
+            param($operation, $stateProperty, $diffProperty)
+
+            $out = dsc -l debug resource $operation -r AdaptedReturn/Override -i $desired 2>$TestDrive/error.txt | ConvertFrom-Json
+            $errorTxt = Get-Content -Path $TestDrive/error.txt -Raw
+            $LASTEXITCODE | Should -Be 0 -Because $errorTxt
+            $out.$stateProperty.one | Should -BeExactly 'fromOperation'
+            $out.$diffProperty | Should -Be @('one')
+            $errorTxt | Should -BeLike "*Adapted resource 'AdaptedReturn/Override' defines return kind 'state' for operation '$operation'*"
+        }
+
+        It 'Uses the return kind of the legacy whatIf method of the adapter instead of the one the adapted resource defines for set' {
+            $out = dsc -l debug resource set --what-if -r AdaptedReturn/Override -i $desired 2>$TestDrive/error.txt | ConvertFrom-Json
+            $errorTxt = Get-Content -Path $TestDrive/error.txt -Raw
+            $LASTEXITCODE | Should -Be 0 -Because $errorTxt
+            $out.afterState.one | Should -BeExactly 'fromWhatIf'
+            $out.changedProperties | Should -Be @('fromWhatIf')
+            $errorTxt | Should -Not -BeLike "*Adapted resource 'AdaptedReturn/Override' defines return kind 'state' for operation 'set'*"
+        }
+
+        It 'Rejects an adapted resource manifest with an invalid return kind for <operation>' -TestCases @(
+            @{ operation = 'set' }
+            @{ operation = 'test' }
+        ) {
+            param($operation)
+
+            $manifest = @{
+                '$schema'      = 'https://aka.ms/dsc/schemas/v3/bundled/resource/adapted/manifest.json'
+                type           = 'Adapted/InvalidReturn'
+                kind           = 'resource'
+                version        = '1.0.0'
+                capabilities   = @('get', 'set', 'test')
+                requireAdapter = 'Test/Adapter'
+                path           = 'invalidReturn.dsc.adaptedResource.json'
+                $operation     = @{ return = 'diff' }
+                schema         = @{ embedded = @{ type = 'object' } }
+            } | ConvertTo-Json -Depth 5
+            $manifestDir = Join-Path $TestDrive "invalidReturn-$operation"
+            $null = New-Item -Path $manifestDir -ItemType Directory
+            Set-Content -Path (Join-Path $manifestDir 'invalidReturn.dsc.adaptedResource.json') -Value $manifest
+
+            $oldPath = $env:DSC_RESOURCE_PATH
+            $oldRestrictedPath = $env:DSC_RESTRICTED_PATH
+            try {
+                # DSC_RESTRICTED_PATH takes precedence over DSC_RESOURCE_PATH
+                $env:DSC_RESTRICTED_PATH = $null
+                $env:DSC_RESOURCE_PATH = $manifestDir + [System.IO.Path]::PathSeparator + (Split-Path (Get-Command dsctest).Source)
+                $out = dsc resource list 'Adapted/InvalidReturn' 2>$TestDrive/error.txt
+                $out | Should -BeNullOrEmpty
+                Get-Content -Path $TestDrive/error.txt -Raw | Should -BeLike '*unknown variant*diff*'
+            }
+            finally {
+                $env:DSC_RESOURCE_PATH = $oldPath
+                $env:DSC_RESTRICTED_PATH = $oldRestrictedPath
+            }
+        }
+    }
 }

lib/dsc-lib/locales/en-us.toml

@@ -238,6 +238,7 @@ exportNotSupportedUsingGet = "Export is not supported by resource '%{resource}'
 runProcessError = "Failed to run process '%{executable}': %{error}"
 whatIfWarning = "Resource '%{resource}' uses deprecated 'whatIf' operation. See https://github.com/PowerShell/DSC/issues/1361 for migration information."
 securityContextRequired = "Operation '%{operation}' for resource '%{resource}' requires security context '%{context}'"
+adaptedReturnKind = "Adapted resource '%{resource}' defines return kind '%{kind}' for operation '%{operation}', overriding the adapter manifest"
 noAdaptedContent = "No adapted content available for resource '%{resource}'"
 invalidAdaptedContent = "Invalid adapted content for resource '%{resource}': %{error}"
 exportFilteringNotSupported = "Resource '%{resource}' does not support export filtering"

lib/dsc-lib/src/dscresources/adapted_resource_manifest.rs

@@ -2,7 +2,7 @@
 // Licensed under the MIT License.
 
 use crate::{
-    configure::config_doc::SecurityContextKind, dscresources::{dscresource::ResourceCapability, resource_manifest::{Kind, ResourceManifest}},  schemas::dsc_repo::DscRepoSchema, types::{FullyQualifiedTypeName, ResourceVersion},
+    configure::config_doc::SecurityContextKind, dscresources::{dscresource::ResourceCapability, resource_manifest::{Kind, ResourceManifest, ReturnKind}},  schemas::dsc_repo::DscRepoSchema, types::{FullyQualifiedTypeName, ResourceVersion},
 };
 use rust_i18n::t;
 use schemars::JsonSchema;
@@ -30,6 +30,9 @@ pub struct GetOperation {
 #[serde(deny_unknown_fields, rename_all = "camelCase")]
 pub struct SetOperation {
     pub require_security_context: Option<SecurityContextKind>,
+    /// The type of return value the adapter emits when it invokes the Set operation for this resource. When specified, this overrides the `return` property of the Set method in the adapter's manifest.
+    #[serde(rename = "return", skip_serializing_if = "Option::is_none")]
+    pub returns: Option<ReturnKind>,
 }
 
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
@@ -44,6 +47,9 @@ pub struct DeleteOperation {
 #[serde(deny_unknown_fields, rename_all = "camelCase")]
 pub struct TestOperation {
     pub require_security_context: Option<SecurityContextKind>,
+    /// The type of return value the adapter emits when it invokes the Test operation for this resource. When specified, this overrides the `return` property of the Test method in the adapter's manifest.
+    #[serde(rename = "return", skip_serializing_if = "Option::is_none")]
+    pub returns: Option<ReturnKind>,
 }
 
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]

lib/dsc-lib/src/dscresources/command_resource.rs

@@ -101,6 +101,7 @@ pub fn invoke_set(resource: &DscResource, desired: &str, skip_test: bool, contex
     };
     let operation_type: String;
     let mut is_synthetic_what_if = false;
+    let mut is_legacy_what_if = false;
     let command_resource = match context.target_resource.as_ref() {
         Some(target) => target,
         None => resource
@@ -124,6 +125,7 @@ pub fn invoke_set(resource: &DscResource, desired: &str, skip_test: bool, contex
                 &manifest.set
             } else if manifest.what_if.is_some() {
                 warn!("{}", t!("dscresources.commandResource.whatIfWarning", resource = &command_resource.type_name));
+                is_legacy_what_if = true;
                 &manifest.what_if
             } else {
                 is_synthetic_what_if = true;
@@ -227,10 +229,17 @@ pub fn invoke_set(resource: &DscResource, desired: &str, skip_test: bool, contex
 
     let (exit_code, stdout, stderr) = invoke_command(&set.executable, args, input_desired, resource.directory.as_deref(), env, manifest.exit_codes.as_ref())?;
 
+    // The adapted resource manifest defines `return` for the `set` method. The legacy `whatIf`
+    // method is a separate command with its own `return`, so the override doesn't apply to it.
+    let returns = if is_legacy_what_if {
+        set.returns.as_ref()
+    } else {
+        resolve_return_kind(context.target_resource.as_ref(), set.returns.as_ref(), &Operation::Set)
+    };
     let return_kind = if context.execution_type == ExecutionKind::WhatIf {
-        set.what_if_returns.as_ref().or(set.returns.as_ref())
+        set.what_if_returns.as_ref().or(returns)
     } else {
-        set.returns.as_ref()
+        returns
     };
 
     match return_kind {
@@ -348,7 +357,7 @@ pub fn invoke_test(resource: &DscResource, expected: &str, context: &Context) ->
     }
 
     let mut expected_value: Value = serde_json::from_str(expected)?;
-    match test.returns {
+    match resolve_return_kind(context.target_resource.as_ref(), test.returns.as_ref(), &Operation::Test) {
         Some(ReturnKind::State) => {
             if command_resource.kind == Kind::Resource {
                 debug!("{}", t!("dscresources.commandResource.testVerifyOutput", resource = &command_resource.type_name, executable = &test.executable));
@@ -1367,6 +1376,46 @@ pub fn log_stderr_line<'a>(process_id: &u32, trace_line: &'a str) -> &'a str
     ""
 }
 
+/// Resolve the kind of output DSC expects from the command for a `set` or `test` operation.
+///
+/// An adapted resource manifest can define `return` for its `set` and `test` operations. When
+/// it does, that value takes precedence over the `return` defined for the same method in the
+/// manifest of the adapter that invokes the command. Otherwise the adapted resource inherits
+/// the value from the adapter's manifest.
+///
+/// The override describes the output of the adapter's `set` and `test` methods. Don't call this
+/// function for the legacy `whatIf` method, which defines its own `return`.
+///
+/// # Arguments
+///
+/// * `target_resource` - The adapted resource the command is invoked for, if any.
+/// * `manifest_return` - The return kind defined for the method in the manifest of the resource
+///   or adapter that owns the command.
+/// * `operation` - The operation being invoked. Only [`Operation::Set`] and [`Operation::Test`]
+///   can be overridden by an adapted resource manifest.
+///
+/// # Returns
+///
+/// The return kind from the adapted resource manifest when it defines one for the operation,
+/// otherwise `manifest_return`.
+fn resolve_return_kind<'a>(target_resource: Option<&'a DscResource>, manifest_return: Option<&'a ReturnKind>, operation: &Operation) -> Option<&'a ReturnKind> {
+    let adapted_return = target_resource
+        .and_then(|resource| resource.adapted_manifest.as_ref())
+        .and_then(|adapted_manifest| match operation {
+            Operation::Set => adapted_manifest.set.as_ref().and_then(|set| set.returns.as_ref()),
+            Operation::Test => adapted_manifest.test.as_ref().and_then(|test| test.returns.as_ref()),
+            _ => None,
+        });
+    if let (Some(resource), Some(return_kind)) = (target_resource, adapted_return) {
+        let kind = match return_kind {
+            ReturnKind::State => "state",
+            ReturnKind::StateAndDiff => "stateAndDiff",
+        };
+        debug!("{}", t!("dscresources.commandResource.adaptedReturnKind", resource = &resource.type_name, operation = operation, kind = kind));
+    }
+    adapted_return.or(manifest_return)
+}
+
 pub(crate)fn validate_security_context(target_resource: Option<&DscResource>, required_security_context: &Option<SecurityContextKind>, resource_type: &str, operation: &Operation) -> Result<(), DscError> {
     if let Some(resource) = target_resource && let Some(adapted_manifest) = &resource.adapted_manifest {
         let require_security_context = match operation {