feat: implement recursive placeholder resolution in Default parameters #1013
Conversation
AshokThangavel
requested review from
isc-dchui,
isc-egabhart,
isc-eneil,
isc-jili,
isc-jlechtne,
isc-kiyer,
isc-pbarton and
isc-tleavitt
as code owners
isc-dchui
left a comment
isc-dchui
left a comment
There was a problem hiding this comment.
A few comments and questions
| continue:data'["${" | ||
|
|
||
| set varExpr = "${" _ $piece($piece(data, "${", 2), "}") _ "}" | ||
| set resolved = ##class(%IPM.Utils.Module).%EvaluateSystemExpression(varExpr) |
There was a problem hiding this comment.
Does this not work if you pass the whole string into %EvaluateSystemExpression and let it resolve all the expressions at once instead of just extracting the first one?
There was a problem hiding this comment.
I've modified the logic to ensure the system variables and custom variables.
| <Default Name="ipmtest" Value="TESTING MY STRING"/> | ||
| <Default Name="ipmdir" Value="/usr/irissys/mgr/user/mts"/> | ||
| <Default Name="datapath" Value="${libdir}data/" /> | ||
| <Default Name="datapath1" Value="${ipmdir}data/" /> |
There was a problem hiding this comment.
I'm a bit curious about how this will handle the rather pathological case of using defaults to create a new system expression from parts. So something like this:
<Default Name="start" Value="${" />
<Default Name="middle" Value="namespace" />
<Default Name="end" Value="}" />
<Default Name="frankenstein" Value="${start}${middle}${end}"/>
There was a problem hiding this comment.
Thank you for pointing out that excellent scenario. You are absolutely right—this is a 'pathological' but critical edge case for configuration flexibility. I have updated the implementation.
-Support nested ${var} and {$var} delimiters and handled Frankenstein-case
isc-dchui
left a comment
isc-dchui
left a comment
There was a problem hiding this comment.
Thanks for the updates! Here's another pass of comments/questions
| set found = 0 | ||
| set maxLevels = maxLevels - 1 | ||
| //Decrement levels and check for circular references | ||
| if '$increment(maxLevels, -1) { |
There was a problem hiding this comment.
I don't think $increment will thrown an error if maxLevels <= 0. Only if it is (way) too large, so this error condition needs a bit of adjustment
There was a problem hiding this comment.
The ' negation catches when $increment returns 0, which happens exactly after 20 passes. However, I'll update it to an explicit if (maxLevels <= 0) check to ensure it's more robust and readable. Thank you!
There was a problem hiding this comment.
Ah you're so right, my bad for misreading that.
|
|
||
| set varExpr = "${" _ $piece($piece(data, "${", 2), "}") _ "}" | ||
| set resolved = ##class(%IPM.Utils.Module).%EvaluateSystemExpression(varExpr) | ||
| set initialData = data |
There was a problem hiding this comment.
Hm, let me reword what I meant. Instead of all this manual parsing, can we not do something like this?
set param = $order(customParams(param), 1, data)
quit:param=""
//Skip if no placeholders remain
continue:data'["{"
set resolved=##class(%IPM.Utils.Module).%EvaluateSystemExpression(param)
// and then for (key, val) in customParams, set resolved = %RegExReplace(resolved, key, val)
...
Ideally we do not duplicate the SystemExpressions into another function.
There was a problem hiding this comment.
I agree. I initially considered using %EvaluateSystemExpression but opted for a local array approach to optimize the resolution process. Using a pre-fetched systemParams array avoids redundant scans and regex evaluations inside the nested loop, allowing for more efficient lookups. However, I have updated the code to invoke %EvaluateSystemExpression directly to maintain consistency with the existing API.
Thank you!
| } | ||
|
|
||
| /// Create class definition at runtime to capute the resovled reference value through <invoke> in manifest | ||
| ClassMethod CreateClassdef() As %Status |
There was a problem hiding this comment.
I'm not sure I quite understand. Could you please explain why we need a new dynamically created class to capture frankenstein when the other defaults don't?
There was a problem hiding this comment.
The reason I created this class was to verify that placeholders are correctly translated and passed to the method. Additionally I captured other variables in args... and temple global verify those variables. However, I explicitly check frankenstein for an additional layer of confirmation to ensure everything works as expected during testing. Thank you!
…xpression - Adopted %EvaluateSystemExpression for system variable resolution to align with existing APIs. - Retained multi-pass logic to handle nested and "Frankenstein" placeholders. - Implemented a priority gate: customParams take precedence over system defaults. - Hardened maxLevels condition using an explicit guard clause for better robustness.
2 participants
Support Recursive Placeholder Resolution in Module Parameter
Description
This PR introduces deeply nested variables or chained configuration settings where one
${variable}depends on another.with a safety handle complex dependencies and prevent infinite loops in the case of circular references.
fixes: #1009
Changes
ResolvePlaceholdersto use awhileloop that continues as long as substitutions are being made.Testing Performed
${A}->${B}->${C}->FinalValue.Test.PM.Integration.Modulethat:module.xmlfrom XData.loadcommand to verify the full lifecycle of the variable resolution.Test Execution Results:
Test.PM.Integration.Module