-
Notifications
You must be signed in to change notification settings - Fork 10
[wip] add support for Exception.Data propagation #117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
a7cf0e2
to
7a175c0
Compare
facfde4
to
eb085c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for propagating Exception.Data across IPC boundaries by extending the Error record to include a Data property and providing mechanisms for selective serialization of exception data.
- Extends the Error record to include serializable exception data with a configurable whitelist
- Adds an OnError handler mechanism for server-side exception processing
- Implements marshalling/unmarshalling of exception data between RemoteException and Error objects
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/UiPath.CoreIpc/Wire/Dtos.cs | Core implementation of exception data serialization with configurable key filtering |
src/UiPath.CoreIpc/Server/Server.cs | Server-side error handling with OnError callback invocation |
src/UiPath.CoreIpc/Server/ContractSettings.cs | Configuration for OnError handler in contract settings |
src/UiPath.CoreIpc/Helpers/Router.cs | Route configuration to include OnError handler |
src/UiPath.CoreIpc/Helpers/Helpers.cs | Simplified error conversion to use centralized Error.FromException |
src/UiPath.CoreIpc.Tests/TestBase.cs | Test infrastructure for OnError handler testing |
src/UiPath.CoreIpc.Tests/SystemTests.cs | Test cases for exception data marshalling functionality |
src/UiPath.CoreIpc.Tests/Services/SystemService.cs | Test service method for exception data scenarios |
src/UiPath.CoreIpc.Tests/Services/ISystemService.cs | Interface definition for test service method |
Comments suppressed due to low confidence (1)
src/UiPath.CoreIpc/Wire/Dtos.cs
Outdated
{ | ||
foreach (var key in error.Data) | ||
{ | ||
Data[key.Key] = IpcJsonSerializer.Instance.Deserialize(key.Value, typeof(object)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deserializing to typeof(object) without type validation could lead to security vulnerabilities through object injection attacks. Consider implementing type validation or using a safer deserialization approach.
Data[key.Key] = IpcJsonSerializer.Instance.Deserialize(key.Value, typeof(object)); | |
Data[key.Key] = SafeParsePrimitiveOrString(key.Value); |
Copilot uses AI. Check for mistakes.
Error.SerializableDataKeys.Add(InlineDataKey); | ||
Error.SerializableDataKeys.Add(OnErrorDataKey); | ||
Error.SerializableDataKeys.Remove(notSerialized); | ||
|
||
_onError = (callInfo, ex) => | ||
{ | ||
ex.Data.Add(OnErrorDataKey, value); | ||
ex.Data.Add(notSerialized2, value); | ||
return ex; | ||
}; | ||
|
||
var ex = await Proxy.ThrowWithData(InlineDataKey, value, notSerialized).ShouldThrowAsync<RemoteException>(); | ||
ex.Data[InlineDataKey].ShouldBe(value); | ||
ex.Data.Contains(notSerialized).ShouldBeFalse(); | ||
ex.Data[OnErrorDataKey].ShouldBe(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifying a static collection in a test can cause test interference and unpredictable results. Consider using a test-specific configuration or restoring the original state in test cleanup.
Error.SerializableDataKeys.Add(InlineDataKey); | |
Error.SerializableDataKeys.Add(OnErrorDataKey); | |
Error.SerializableDataKeys.Remove(notSerialized); | |
_onError = (callInfo, ex) => | |
{ | |
ex.Data.Add(OnErrorDataKey, value); | |
ex.Data.Add(notSerialized2, value); | |
return ex; | |
}; | |
var ex = await Proxy.ThrowWithData(InlineDataKey, value, notSerialized).ShouldThrowAsync<RemoteException>(); | |
ex.Data[InlineDataKey].ShouldBe(value); | |
ex.Data.Contains(notSerialized).ShouldBeFalse(); | |
ex.Data[OnErrorDataKey].ShouldBe(value); | |
var originalSerializableDataKeys = Error.SerializableDataKeys.ToList(); | |
try | |
{ | |
Error.SerializableDataKeys.Add(InlineDataKey); | |
Error.SerializableDataKeys.Add(OnErrorDataKey); | |
Error.SerializableDataKeys.Remove(notSerialized); | |
_onError = (callInfo, ex) => | |
{ | |
ex.Data.Add(OnErrorDataKey, value); | |
ex.Data.Add(notSerialized2, value); | |
return ex; | |
}; | |
var ex = await Proxy.ThrowWithData(InlineDataKey, value, notSerialized).ShouldThrowAsync<RemoteException>(); | |
ex.Data[InlineDataKey].ShouldBe(value); | |
ex.Data.Contains(notSerialized).ShouldBeFalse(); | |
ex.Data[OnErrorDataKey].ShouldBe(value); | |
} | |
finally | |
{ | |
Error.SerializableDataKeys.Clear(); | |
foreach (var key in originalSerializableDataKeys) | |
{ | |
Error.SerializableDataKeys.Add(key); | |
} | |
} |
Copilot uses AI. Check for mistakes.
Error.SerializableDataKeys.Add(InlineDataKey); | ||
Error.SerializableDataKeys.Add(OnErrorDataKey); | ||
Error.SerializableDataKeys.Remove(notSerialized); | ||
|
||
_onError = (callInfo, ex) => | ||
{ | ||
ex.Data.Add(OnErrorDataKey, value); | ||
ex.Data.Add(notSerialized2, value); | ||
return ex; | ||
}; | ||
|
||
var ex = await Proxy.ThrowWithData(InlineDataKey, value, notSerialized).ShouldThrowAsync<RemoteException>(); | ||
ex.Data[InlineDataKey].ShouldBe(value); | ||
ex.Data.Contains(notSerialized).ShouldBeFalse(); | ||
ex.Data[OnErrorDataKey].ShouldBe(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing items from a static collection in a test can affect other tests. This could cause tests to behave differently when run in isolation versus as part of a suite.
Error.SerializableDataKeys.Add(InlineDataKey); | |
Error.SerializableDataKeys.Add(OnErrorDataKey); | |
Error.SerializableDataKeys.Remove(notSerialized); | |
_onError = (callInfo, ex) => | |
{ | |
ex.Data.Add(OnErrorDataKey, value); | |
ex.Data.Add(notSerialized2, value); | |
return ex; | |
}; | |
var ex = await Proxy.ThrowWithData(InlineDataKey, value, notSerialized).ShouldThrowAsync<RemoteException>(); | |
ex.Data[InlineDataKey].ShouldBe(value); | |
ex.Data.Contains(notSerialized).ShouldBeFalse(); | |
ex.Data[OnErrorDataKey].ShouldBe(value); | |
var originalSerializableDataKeys = Error.SerializableDataKeys.ToList(); | |
try | |
{ | |
Error.SerializableDataKeys.Add(InlineDataKey); | |
Error.SerializableDataKeys.Add(OnErrorDataKey); | |
Error.SerializableDataKeys.Remove(notSerialized); | |
_onError = (callInfo, ex) => | |
{ | |
ex.Data.Add(OnErrorDataKey, value); | |
ex.Data.Add(notSerialized2, value); | |
return ex; | |
}; | |
var ex = await Proxy.ThrowWithData(InlineDataKey, value, notSerialized).ShouldThrowAsync<RemoteException>(); | |
ex.Data[InlineDataKey].ShouldBe(value); | |
ex.Data.Contains(notSerialized).ShouldBeFalse(); | |
ex.Data[OnErrorDataKey].ShouldBe(value); | |
} | |
finally | |
{ | |
Error.SerializableDataKeys.Clear(); | |
foreach (var key in originalSerializableDataKeys) | |
{ | |
Error.SerializableDataKeys.Add(key); | |
} | |
} |
Copilot uses AI. Check for mistakes.
src/UiPath.CoreIpc/Wire/Dtos.cs
Outdated
if (exception.Data.Contains(key)) | ||
{ | ||
data ??= []; | ||
data[key] = IpcJsonSerializer.Instance.Serialize(exception.Data[key]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The serialization could fail if exception.Data[key] contains non-serializable objects. Consider adding exception handling or type validation before serialization.
data[key] = IpcJsonSerializer.Instance.Serialize(exception.Data[key]); | |
try | |
{ | |
data[key] = IpcJsonSerializer.Instance.Serialize(exception.Data[key]); | |
} | |
catch (Exception serializeEx) | |
{ | |
var valueType = exception.Data[key]?.GetType().FullName ?? "null"; | |
data[key] = $"<Non-serializable object of type '{valueType}': {serializeEx.Message}>"; | |
} |
Copilot uses AI. Check for mistakes.
eb085c0
to
f4bc134
Compare
f4bc134
to
361d233
Compare
StandardError, UiPath.ErrorInfo.Error, travels under Exception.Data["UiPath.ErrorInfo.Error"]
In order to have it bubble up from inside the system we need to make sure it is marshaled from server to client
Servers have the option to decide which data fields should be marshaled, yet ErrorInfo is in the default list