Fixed crash on invalid ip and or port change #233
Conversation
…verted to default on invalid change.
| { | ||
| var hostEntry = Dns.GetHostEntry(address); | ||
| return true; | ||
| } | ||
| catch | ||
| { | ||
| // DNS resolution failed | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Could this be simplified as
return Uri.CheckHostName(address) != UriHostNameType.UnknownThat way you could avoid the exception handling, as well as resolve for hostnames and IP addresses all in a single line.
| { | ||
| if (string.IsNullOrWhiteSpace(address)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Check if it's a valid IP address | ||
| if (IPAddress.TryParse(address, out _)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Check if it's a resolvable hostname | ||
| try | ||
| { | ||
| var hostEntry = Dns.GetHostEntry(address); | ||
| return true; | ||
| } | ||
| catch | ||
| { | ||
| // DNS resolution failed | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
If you do elect to use Uri.CheckHostName, this whole function could probably be simplified to
private static bool IsValidAddress(string address) =>
!string.IsNullOrWhiteSpace(address) &&
(IPAddress.TryParse(address, out _) ||
Uri.CheckHostName(address) != UriHostNameType.Unknown);| { | ||
| if (newValue < 1) | ||
| { | ||
| InPort = DefaultInPort; | ||
| } | ||
| } | ||
|
|
||
| /// Called after the OutPort property has changed. | ||
| /// It checks if the OutPort is valid and reverts to the default if it's invalid (less than 1). | ||
| partial void OnOutPortChanged(int oldValue, int newValue) | ||
| { | ||
| if (newValue < 1) | ||
| { | ||
| OutPort = DefaultOutPort; | ||
| } | ||
| } |
There was a problem hiding this comment.
I worry this will cause an infinite loop. Also, such functionality could be achieved by editing values in the ui xaml here
and here
There was a problem hiding this comment.
Also, not sure you want to be using these operators. They both make it so that the port can only be set if the port is less than 1..
There was a problem hiding this comment.
yeah i will use that instead than the port check does not have to exist,
As for the IP verification i might change the way i approach that since host names are disallowed as it will throw an exception in
It might also be smart to do this in
VRCFaceTracking/VRCFaceTracking.Core/Services/OscRecvService.cs
Lines 30 to 51 in 25f7e95
As there is already some handling in there but it seems to actually not handle empty strings but does handle NULL ref
There was a problem hiding this comment.
Although this would still cause the application to crash on restart since it doesn't reset the value.
…ettingsPage.xaml, updated the IsValidAddress to be more comppressed and removed the hostname check since its not used.
|
Changed the files as requested. I ended up not using host name since its not supported as listed in the comment above, |
| /// Called after the DestinationAddress property has changed. | ||
| /// It validates the new address and reverts it to the default if invalid. | ||
| partial void OnDestinationAddressChanged(string oldValue, string newValue) |
There was a problem hiding this comment.
Might be an idea to use NotifyDataErrorInfo for this instead of just reverting the val
https://learn.microsoft.com/en-us/dotnet/communitytoolkit/mvvm/generators/observableproperty#requesting-property-validation
There was a problem hiding this comment.
Might even be able to create a custom validationattribute to validate it's a valid IP and be rid of that whole method
There was a problem hiding this comment.
Will look into it. Seems promising, would also like to not have an validating function in the class if its not necessary.
There was a problem hiding this comment.
Might be an idea to use
NotifyDataErrorInfofor this instead of just reverting the val https://learn.microsoft.com/en-us/dotnet/communitytoolkit/mvvm/generators/observableproperty#requesting-property-validation
this is not directly possible since NotifyDataErrorInfo is an inheritance from ObservableProperty while the OSCTarget class is an child of ObservableObject which does not support it.
There was a problem hiding this comment.
Wat? The parent of OscTarget has no relevance. You should just be able to append the attribute below the existing ObservableProperty attribute of DestinationAddress
There was a problem hiding this comment.
Idk man it didn't allow me to do it this way, i just ended up doing your second suggestion which is a more elegant solution anyway, in my opinion.
…ogging when given IP is invalid and revert back to default.
|
|
…nd OscSendService, updated ValidIpAddressAttribute to handel IsNullOrWhiteSpace correctly
| if (!Validator.TryValidateObject(oscTarget, context, validationResults, true)) | ||
| { | ||
| var errorMessages = string.Join(Environment.NewLine, validationResults.Select(vr => vr.ErrorMessage)); | ||
| _logger.LogWarning($"{errorMessages} Reverting to default."); | ||
| oscTarget.DestinationAddress = "127.0.0.1"; | ||
| } |
There was a problem hiding this comment.
On startup this gets run before the DestinationAddress is loaded from the SavedSetting which cause the warning to be printed in the Output, but this has no effect on the final DestinationAddress that gets loaded afterwards.
When changing the ip address or port number to an invalid value on the next restart the app would crash do to the loaded invalid value'(s),
This pull requests checks if: the ip address is not blank or white space, the ip is an valid ip address, it is a resolvable hostname.
It also checks if the ports are not smaller than 1.
Let me know if the code needs any modifications.