refactor: explicit switch for CIM method names in Invoke-ComputerRegistry (STRUCT-5)#12
refactor: explicit switch for CIM method names in Invoke-ComputerRegistry (STRUCT-5)#12dutch2005 wants to merge 1 commit into
Conversation
…stry (STRUCT-5)
The Set branch composed the StdRegProv method name at runtime from the
ValueType parameter: `$methodName = "Set$($ValueType)Value"`. That made
the actual method target depend on a runtime string, meaning:
* A reviewer had to mentally concatenate three strings to know which
method is being invoked.
* PSScriptAnalyzer could not flag a drift between the ValidateSet
attribute and the actual set of methods StdRegProv exposes.
* Adding a new ValueType required updating three places (ValidateSet,
the if/elseif chain for $cimArgs, and mentally verifying the method
name composition still produced a real method).
Fix: replace the `if/elseif/elseif/else` with a single `switch` keyed
on $ValueType, where each arm sets both $methodName and $cimArgs
explicitly. Adding a new type is now a single contiguous edit; the
method target is a literal string that grep and static analyzers can
follow.
No behavioral change — the three supported types still map to the
same StdRegProv methods with the same argument shapes.
Refs: code-review-2026-04-24.md STRUCT-5
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Reviewed by nemotron-3-super-120b-a12b-20230311:free · 132,093 tokens |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the registry value setting logic in Invoke-ComputerRegistry.ps1 by replacing dynamic method name composition with an explicit switch statement, improving code auditability and maintainability. I have reviewed the changes and suggest implementing support for the 'ExpandString' registry type within the new switch block, as it is currently permitted by the parameter validation but not yet handled.
| switch ($ValueType) { | ||
| "String" { | ||
| $methodName = "SetStringValue" | ||
| $cimArgs = @{ hDefKey = $hDef; sSubKeyName = $KeyPath; sValueName = $ValueName; sValue = [string]$Value } | ||
| } | ||
| "DWord" { | ||
| $methodName = "SetDWORDValue" | ||
| $cimArgs = @{ hDefKey = $hDef; sSubKeyName = $KeyPath; sValueName = $ValueName; uValue = [uint32]$Value } | ||
| } | ||
| "QWord" { | ||
| $methodName = "SetQWORDValue" | ||
| $cimArgs = @{ hDefKey = $hDef; sSubKeyName = $KeyPath; sValueName = $ValueName; uValue = [uint64]$Value } | ||
| } | ||
| default { | ||
| Write-Warning "ValueType '$ValueType' is not yet fully implemented." | ||
| return $false | ||
| } | ||
| } |
There was a problem hiding this comment.
The switch statement is a significant improvement for auditability. However, the ValidateSet for the $ValueType parameter (line 29) includes ExpandString, Binary, and MultiString. These types currently fall into the default block, which returns $false. While this is functionally equivalent to the previous implementation (which also didn't handle them in the if chain), it would be beneficial to implement ExpandString using the SetExpandedStringValue method, as it is a common registry type and already allowed by the parameter validation.
1 participant
Summary
Setbranch ofInvoke-ComputerRegistrycomposed the StdRegProv method name at runtime:\$methodName = \"Set\$(\$ValueType)Value\"if/elseif/elseif/elsechain with a single explicitswitch— each arm sets\$methodNameas a literal and assigns the correctly-named\$cimArgsWhy
SetStringValue,SetDWORDValue,SetQWORDValueand see every call site. With runtime composition, the stringSetDWORDValueliterally does not appear in the sourceValidateSetattribute ever gains a new value ("MultiString", "Binary") without a matching method arm, the new code path now falls into thedefaultbranch (explicit[!] not yet implementedreturn) instead of attempting a CIM call to a method that may or may not exist by that composed nameChanges
LazyWinAdminModule/Private/Invoke-ComputerRegistry.ps1— replaced the Set-branch composition + if-chain with an explicit switch. No change to Get / New / Remove branches, no parameter change, no return-value change.Behavioral equivalence
Set\+String+Value=SetStringValueSetStringValueSet\+DWord+Value=SetDWordValueSetDWORDValueSet\+QWord+Value=SetQWordValueSetQWORDValueSetDWORDValue/SetQWORDValue(all-caps DWORD/QWORD), but the ValueType ValidateSet uses PowerShell-idiomaticDWord/QWord. The previous composition producedSetDWordValue/SetQWordValue, relying on CIM's method-resolution being case-insensitive. The explicit switch now spells the method names the way StdRegProv documents them. Please confirm no regression on any caller that was depending on the case-insensitive match surviving.Test plan
Set-ItemviaInvoke-ComputerRegistry -Action Set -ValueType DWordstill writes the value on a local CIM sessionSet-ComputerRDP(which touchesfDenyTSConnectionsvia DWORD set) still toggles RDP correctly\$falsewith the existing warning text (no change)Risks / rollback
SetDWordValue→SetDWORDValue) should be a no-op since CIM method resolution is case-insensitive, but any caller running under a CIM provider that enforces exact-case method names would newly succeed / newly fail based on the switch. Please verify in an integration run.git revert— the entire change is contained in one function, one branch of its switch.Refs:
code-review-2026-04-24.mdSTRUCT-5@gemini-code-assist please review
@coderabbitai review
@Kilo please review