-
Notifications
You must be signed in to change notification settings - Fork 1k
Check bounds for switched on values and roslyn cleanup #3264
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
Conversation
…xpected enum values Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
…entType values Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3264 +/- ##
==========================================
- Coverage 57.88% 57.61% -0.28%
==========================================
Files 365 365
Lines 79723 80076 +353
Branches 13852 13859 +7
==========================================
- Hits 46150 46138 -12
- Misses 29428 29737 +309
- Partials 4145 4201 +56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 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
|
@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. |
Libraries/Opc.Ua.Configuration/ApplicationConfigurationBuilder.cs
Outdated
Show resolved
Hide resolved
Libraries/Opc.Ua.Configuration/ApplicationConfigurationBuilder.cs
Outdated
Show resolved
Hide resolved
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.
I was a bit late to complete review. :)
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...