Check bounds for switched on values and roslyn cleanup #3264
Conversation
…xpected enum values Co-authored-by: marcschier <[email protected]>
Co-authored-by: marcschier <[email protected]>
…entType values Co-authored-by: marcschier <[email protected]>
Co-authored-by: marcschier <[email protected]>
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3264 +/- ##
==========================================
- Coverage 57.81% 57.51% -0.30%
==========================================
Files 365 365
Lines 79423 79838 +415
Branches 13870 13874 +4
==========================================
+ Hits 45920 45922 +2
- Misses 29338 29704 +366
- Partials 4165 4212 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on checking bounds for values used in switch statements and includes various Roslyn analyzer cleanup improvements across the codebase.
Key changes include:
- Enhanced switch statement handling with proper bounds checking and default cases
- Replacement of manual exception construction with ServiceResultException.Unexpected() utility method
- Addition of missing default cases in switch statements to handle unexpected enum values
- Code cleanup and modernization following Roslyn analyzer suggestions
Reviewed Changes
Copilot reviewed 123 out of 123 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/Opc.Ua.Server.Tests/ServerFixtureUtils.cs | Replaced manual ServiceResultException construction with ServiceResultException.Unexpected() |
| Tests/Opc.Ua.Core.Tests/Types/Encoders/JsonValidationData.cs | Converted switch statement to switch expression with bounds checking |
| Tests/Opc.Ua.Core.Tests/Types/Encoders/EncoderCommon.cs | Added default case with bounds checking in decode method |
| Tests/Opc.Ua.Core.Tests/Types/Encoders/EncodeableTests.cs | Added missing default cases and exception documentation |
| Tests/Opc.Ua.Core.Tests/Types/BuiltIn/BuiltInTests.cs | Enhanced NodeId comparison with proper default case handling |
| Stack/Opc.Ua.Core/Types/Utils/Utils.cs | Reorganized switch statements and replaced manual exception construction |
| Stack/Opc.Ua.Core/Types/Utils/TypeInfo.cs | Added bounds checking and proper default cases for BuiltInType handling |
| Stack/Opc.Ua.Core/Types/Utils/ServiceResultException.cs | Added Unexpected() utility method for consistent exception handling |
| Stack/Opc.Ua.Core/Types/Encoders/*.cs | Enhanced encoder/decoder classes with proper bounds checking |
| Libraries/Opc.Ua.Server/Session/Session.cs | Added bounds checking for RequestType enum values |
Comments suppressed due to low confidence (4)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Stack/Opc.Ua.Core/Security/Certificates/SecurityConfiguration.cs
Outdated
Show resolved
Hide resolved
| case BuiltInType.Integer: | ||
| case BuiltInType.UInteger: | ||
| throw new ServiceResultException( | ||
| StatusCodes.BadEncodingError, |
There was a problem hiding this comment.
is this true?
UInteger ->Type '{0}' is not allowed in an Variant.
There was a problem hiding this comment.
The original behavior is in place, this threw before on default, but default: now throws unexpected because e.g. (BuiltInType)333 should be an unexpected error (or bad code). I was thinking that it should still throw EncodingError, but on encoding path it at least is bad code, on decoding path it is debateable because we read ints from wire, and it could result in default: to enter. Now we do not get a decoderError, but BadUnexpected. However - the latter can be handled centrally better, because it is really "unexpected" if this was encoded with our encoders.
There was a problem hiding this comment.
Actually all unhandled cases threw on exit at the bottom of the method.
…he exception message
Updated the handling of diagnostic information by changing `ReturnDiagnostics` from `DiagnosticsMasks.All` to `DiagnosticsMasks.SymbolicIdAndText` across multiple files to reduce verbosity. Enhanced error handling by including `StatusCodes.BadNoCommunication` in retry conditions for `ServiceResultException`. Removed redundant `Session.ReturnDiagnostics` setting in `GlobalDiscoveryServerClient.cs`. Fixed a typo in XML documentation in `ClientBase.cs`. Ensured consistent diagnostic settings in `ClientFixture.cs` and related files.
| /// </summary> | ||
| /// <param name="request">The request.</param> | ||
| /// <param name="useDefaults">if set to <c>true</c> use defaults].</param> | ||
| /// <param name="useDefaults">if set to <c>true</c> use defaults.</param> |
There was a problem hiding this comment.
Shouldnt we document use defaults has no effect any more
There was a problem hiding this comment.
It can be overridden (method is virtual). So I would keep it as is. But hope we are ok with setting diagnostics mask for all requests even if a requestHeader was passed as long as that was passed with None, correct? Because you can set the ReturnDiagnostics on the client, and if you set it there to None you would get the default behavior. And it is by default None.
|
@marcschier thanks for investigating the CI Issues. Can you put the final fixes into a separate PR for better separation? |
@romanett - I am waiting whether I can get tests to pass more stably with this one, and then sort out separating this and stability into 2 PRs. I used this one to incrementally debug because it was the biggest and furthest one ahead compared to the other 2 PR, which now are merged, so yes, makes sense. |
5 participants
Proposed changes
Describe the changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Related Issues
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...