Bug fixes and code refactoring to get it ready for durable function support #72
Conversation
|
@tylerl0706 I need to work on top of this refactoring for cleaning up the durable function implementation. |
|
|
||
| } | ||
| else | ||
| if (parameterMetadata.ContainsKey(binding.Name)) |
There was a problem hiding this comment.
This is fine for now... but we should do the validation to make sure that the param block has all the input bindings.
tracked in #15
| } | ||
|
|
||
| PSObject returnObject = null; | ||
| using (ExecutionTimer.Start(_logger, "Execution of the user's function completed.")) | ||
| var result = _pwsh.AddCommand("Microsoft.Azure.Functions.PowerShellWorker\\Get-OutputBinding") |
There was a problem hiding this comment.
nit: change the var to the actual returned type for clarity
| /// <summary> | ||
| /// Create an object of 'StreamingMessage' as a template, for further update. | ||
| /// </summary> | ||
| private StreamingMessage NewStreamingMessageTemplate(string requestId, StreamingMessage.ContentOneofCase msgType, out StatusResult status) |
There was a problem hiding this comment.
NewStreamingMessageTemplate should probably be renamed to CreateStreamingMessageTemplate or something else...
| if(string.Equals(outBindingName, AzFunctionInfo.DollarReturn, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| response.ReturnValue = results[outBindingName].ToTypedData(_powerShellManager); | ||
| continue; |
There was a problem hiding this comment.
for $return are we only suppose to set it to the ReturnValue? Or are we also suppose to set the parameter binding for $return?
cc/ @pragnagopa
| @@ -92,7 +93,23 @@ public static object ToObject (this TypedData data) | |||
| } | |||
| } | |||
|
|
|||
| public static RpcException ToRpcException (this Exception exception) | |||
| private static JsonSerializerSettings setting = | |||
There was a problem hiding this comment.
I see why you put this here, but I think it should either be in the function as a const, or at the top of the file since it's a constant.
| case JObject dict: | ||
| return new Hashtable(dict.ToObject<Hashtable>(), StringComparer.OrdinalIgnoreCase); | ||
| case JArray list: | ||
| return list.ToObject<object[]>(); |
There was a problem hiding this comment.
good catch. I forgot about arrays.
| break; | ||
| case IDictionary hashtable: |
There was a problem hiding this comment.
how come you got rid of hashtable?
| return false; | ||
| } | ||
|
|
||
| if ((str[0] == '{' && str[len - 1] == '}') || |
There was a problem hiding this comment.
is all of this stuff before the JToken.Parse needed? I would think that JToken.Parse could handle all this.
| @@ -26,7 +26,14 @@ public class PowerShellManagerTests | |||
| } | |||
| } | |||
| }; | |||
| public readonly RpcFunctionMetadata rpcFunctionMetadata = new RpcFunctionMetadata(); | |||
There was a problem hiding this comment.
The function that this is used in gets called multiple times and in that function, you're changing the properties of this object. I think you should just make a new RpcFunctionMetadata inside the function for each invocation instead of mutating the existing one.
| @@ -129,8 +146,13 @@ static RpcHttp ToRpcHttp (this HttpResponseContext httpResponseContext) | |||
| return rpcHttp; | |||
| } | |||
|
|
|||
| public static TypedData ToTypedData(this object value) | |||
| internal static TypedData ToTypedData(this object value, PowerShellManager psHelper) | |||
There was a problem hiding this comment.
I'm not sure how I feel about having to pass in the psHelper all the time just so that we can reuse the powershell session to run ConvertTo-Json...
Can we just create a constant PowerShell instance in this file that's only used to call ConvertTo-Json?
Fix #71
ConvertTo-Jsonfor converting the results returned from the function to JSON. So objects that are wrapped inPSObjectwon't be a problem anymore.The implementation of invoking an orchestration function is made a placeholder in this PR, where it throws
NotImplementedExceptionwhen an orchestration function is triggered.See
RequestProcessor.InvokeOrchestrationFunction.