Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaces iterative character removal in ActSetVariableValue.ClearSpecialChar with a single Regex.Replace call, adding a System.Text.RegularExpressions using and operating on a local VariableString reference. No public API or control flow changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Refactor regex to remove non-alphanumeric characters from variable value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Ginger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.cs`:
- Around line 215-217: The comment and implementation disagree: the
Regex.Replace call (the assignment to variable.Value using Regex.Replace) claims
to exclude Unicode letters/digits but uses ASCII-only character classes
[A-Za-z0-9]; either replace the pattern with a Unicode-aware pattern using \p{L}
and \p{N} (i.e., use [^\p{L}\p{N}\s]) to truly allow all Unicode letters/digits,
or if ASCII-only behavior is intended, update the comment to state "ASCII
letters and digits only" so the comment matches the Regex.Replace usage.
- Line 211: The code directly casts Var to VariableString with
(VariableString)Var which can throw if Var isn't that type; update the usage in
ActSetVariableValue (and the line where variable = (VariableString)Var is set)
to perform a type guard (e.g., if (Var is VariableString variable) { ... } or
var variable = Var as VariableString; if (variable == null) { /* handle: skip
ClearSpecialChar, log warning or throw a descriptive exception */ }) and only
call VariableString-specific members like ClearSpecialChar when the guard
succeeds to match the existing pattern used elsewhere in this class.
- Around line 209-219: The ClearSpecialChar branch uses an unsafe cast
(VariableString)Var and unconditionally strips all non-alphanumeric chars;
change it to mirror the earlier pattern by first checking if (Var.GetType() ==
typeof(VariableString)) before casting to VariableString, and then restore the
original selective-removal behavior by building the Regex using the
user-specified characters from variable.Value (escape them for regex) rather
than using @"[^A-Za-z0-9\s]"; ensure you only remove the characters listed in
variable.Value when SetVariableValueOption ==
VariableBase.eSetValueOptions.ClearSpecialChar and leave other characters
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 011c88ba-aabf-445d-89bf-e20c10d69a3c
📒 Files selected for processing (1)
Ginger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.cs
| else if (SetVariableValueOption == VariableBase.eSetValueOptions.ClearSpecialChar) | ||
| { | ||
| string specChar = ValueExpression.Calculate(this.Value); | ||
| if (string.IsNullOrEmpty(specChar)) | ||
| { | ||
| specChar = @"{}(),\"""; | ||
| } | ||
| if (!string.IsNullOrEmpty(((VariableString)Var).Value)) | ||
| var variable = (VariableString)Var; | ||
|
|
||
| if (!string.IsNullOrEmpty(variable.Value)) | ||
| { | ||
| foreach (char c in specChar) | ||
| { | ||
| ((VariableString)Var).Value = ((VariableString)Var).Value.Replace(c.ToString(), ""); | ||
| } | ||
| // Remove any character that is NOT a Unicode letter, NOT a Unicode digit, and not whitespace. | ||
| // Example: "Wlcome@#\"$%Amdocs123" -> "WlcomeAmdocs123" | ||
| variable.Value = Regex.Replace(variable.Value, @"[^A-Za-z0-9\s]", string.Empty); | ||
|
|
||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find and display the test that exercises ClearSpecialChar to verify behavioral expectations
# Search for the test method
rg -n -A 30 'TestVariable_StringClearSpecialChar' --type csRepository: Ginger-Automation/Ginger
Length of output: 3596
🏁 Script executed:
#!/bin/bash
# Check for type casting patterns in ActSetVariableValue.cs to see if casts are typically guarded
rg -n '\(VariableString\)Var|\(VariableString\)|\bVar\s+as\s+VariableString' Ginger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.cs -B 2 -A 0Repository: Ginger-Automation/Ginger
Length of output: 505
Fix unsafe cast and behavioral inconsistency with ClearSpecialChar option.
Two issues in lines 209-219:
-
Unsafe type cast (line 211): The direct cast
(VariableString)Varbypasses type validation. Just 85 lines earlier, line 124 demonstrates the pattern used in this same file: guard withif (Var.GetType() == typeof(VariableString))before casting. -
Breaking behavioral change: The new regex
[^A-Za-z0-9\s]removes all non-alphanumeric characters, ignoring theValueproperty entirely. The testTestVariable_StringClearSpecialChar()passes only by coincidence—the test input "S$tr!ing@123#" contains exactly the special chars "$@#!" specified for removal, so both approaches yield "String123". The underlying feature contract has changed: selective removal of user-specified characters → unconditional removal of all special characters. If this is intentional, the test expectations and documentation need updating. If selective removal should be preserved, the regex must incorporate the user-specified characters fromValue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.cs` around lines
209 - 219, The ClearSpecialChar branch uses an unsafe cast (VariableString)Var
and unconditionally strips all non-alphanumeric chars; change it to mirror the
earlier pattern by first checking if (Var.GetType() == typeof(VariableString))
before casting to VariableString, and then restore the original
selective-removal behavior by building the Regex using the user-specified
characters from variable.Value (escape them for regex) rather than using
@"[^A-Za-z0-9\s]"; ensure you only remove the characters listed in
variable.Value when SetVariableValueOption ==
VariableBase.eSetValueOptions.ClearSpecialChar and leave other characters
intact.
| specChar = @"{}(),\"""; | ||
| } | ||
| if (!string.IsNullOrEmpty(((VariableString)Var).Value)) | ||
| var variable = (VariableString)Var; |
There was a problem hiding this comment.
Add type check before casting to VariableString.
The direct cast (VariableString)Var will throw InvalidCastException if Var is not a VariableString. While VariableString is the only type that declares ClearSpecialChar as supported, this file consistently uses explicit type checks before casting (see lines 124, 128, 138, 151, 155, 177, 232, 245, 258, 270, 287).
🛡️ Proposed fix to add type guard
else if (SetVariableValueOption == VariableBase.eSetValueOptions.ClearSpecialChar)
{
- var variable = (VariableString)Var;
-
- if (!string.IsNullOrEmpty(variable.Value))
+ if (Var.GetType() == typeof(VariableString))
{
- // Remove any character that is NOT a Unicode letter, NOT a Unicode digit, and not whitespace.
- // Example: "Wlcome@#\"$%Amdocs123" -> "WlcomeAmdocs123"
- variable.Value = Regex.Replace(variable.Value, @"[^A-Za-z0-9\s]", string.Empty);
-
+ var variable = (VariableString)Var;
+ if (!string.IsNullOrEmpty(variable.Value))
+ {
+ // Remove characters based on user specification or all special chars
+ variable.Value = Regex.Replace(variable.Value, @"[^A-Za-z0-9\s]", string.Empty);
+ }
+ }
+ else
+ {
+ Error = "Operation type " + SetVariableValueOption + " is not supported for " + GingerDicser.GetTermResValue(eTermResKey.Variable) + " of type " + Var.GetType();
+ return;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.cs` at line 211,
The code directly casts Var to VariableString with (VariableString)Var which can
throw if Var isn't that type; update the usage in ActSetVariableValue (and the
line where variable = (VariableString)Var is set) to perform a type guard (e.g.,
if (Var is VariableString variable) { ... } or var variable = Var as
VariableString; if (variable == null) { /* handle: skip ClearSpecialChar, log
warning or throw a descriptive exception */ }) and only call
VariableString-specific members like ClearSpecialChar when the guard succeeds to
match the existing pattern used elsewhere in this class.
| // Remove any character that is NOT a Unicode letter, NOT a Unicode digit, and not whitespace. | ||
| // Example: "Wlcome@#\"$%Amdocs123" -> "WlcomeAmdocs123" | ||
| variable.Value = Regex.Replace(variable.Value, @"[^A-Za-z0-9\s]", string.Empty); |
There was a problem hiding this comment.
Comment is inaccurate: regex matches ASCII only, not Unicode.
The comment states "NOT a Unicode letter, NOT a Unicode digit" but the regex [A-Za-z0-9] only matches ASCII letters and digits. For true Unicode support, use \p{L} (Unicode letters) and \p{N} (Unicode digits):
[^\p{L}\p{N}\s]If ASCII-only is intentional, update the comment to reflect that.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.cs` around lines
215 - 217, The comment and implementation disagree: the Regex.Replace call (the
assignment to variable.Value using Regex.Replace) claims to exclude Unicode
letters/digits but uses ASCII-only character classes [A-Za-z0-9]; either replace
the pattern with a Unicode-aware pattern using \p{L} and \p{N} (i.e., use
[^\p{L}\p{N}\s]) to truly allow all Unicode letters/digits, or if ASCII-only
behavior is intended, update the comment to state "ASCII letters and digits
only" so the comment matches the Regex.Replace usage.
1 participant
Description
Type of Change
Checklist
[IsSerializedForLocalRepository]where neededReporter.ToLog()patternSummary by CodeRabbit