-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Client core: new logging exceptions checks #45433
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: main
Are you sure you want to change the base?
Client core: new logging exceptions checks #45433
Conversation
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 introduces new logging exception checks by refactoring exception throwing in deserialization and HTTP response handling, and it updates code to use a dedicated ClientLogger for better context and consistency.
- Refactored exception throwing in Geo model classes to use ClientLogger.
- Updated HTTP serializer and REST proxy classes to log additional context on errors.
- Adjusted checkstyle suppressions and added new tests for exception logging checks.
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
sdk/clientcore/core/models/geo/GeoLineStringCollection.java | Updated exception throws to use ClientLogger with added key/value context. |
sdk/clientcore/core/models/geo/GeoLineString.java | Updated exception management to use ClientLogger for improved logging. |
sdk/clientcore/core/models/geo/GeoCollection.java | Enhanced logging when type or required properties are not as expected. |
sdk/clientcore/core/implementation/http/serializer/HttpResponseBodyDecoder.java | Switched exception wrapping to include logger context. |
sdk/clientcore/core/implementation/http/rest/RestProxyImpl.java | Replaced helper methods with inline logging via ClientLogger for better diagnostics. |
sdk/clientcore/core/implementation/http/rest/LengthValidatingInputStream.java | Added logging to validate stream lengths with detailed key/value pairs. |
sdk/clientcore/core/implementation/ReflectionSerializable.java | Changed exception handling to catch Exception and log via ClientLogger instead of catching all Throwables. |
eng/code-quality-reports/… | Updated test and checkstyle files to validate and enforce logging exception patterns. |
Comments suppressed due to low confidence (3)
sdk/clientcore/core/implementation/ReflectionSerializable.java:126
- [nitpick] Consider adding a descriptive message to the logging call to improve diagnostics when deserialization fails.
throw LOGGER.throwableAtError().log(e, IOException::new);
eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/LogNewExceptionCheck.java:95
- [nitpick] Review whether the check for the core exception factory method is sufficiently robust or if additional conditions should be considered.
if (CORE_EXCEPTION_FACTORY_NAME.equals(FullIdent.createFullIdentBelow(methodCallToken).getText()))
sdk/clientcore/core/implementation/http/rest/RestProxyImpl.java:141
- [nitpick] Ensure that the logged message clearly explains the failure scenario, and consider verifying that all necessary context is provided.
throw LOGGER.throwableAtError()
...ports/src/main/java/com/azure/tools/checkstyle/checks/ExceptionCreatedButNotThrownCheck.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/com/azure/tools/checkstyle/checks/StringFormattedExceptionMessageCheck.java
Show resolved
Hide resolved
...re-data-appconfiguration-v2/src/main/java/com/azure/v2/data/appconfiguration/models/Key.java
Outdated
Show resolved
Hide resolved
sdk/clientcore/core/src/main/java/io/clientcore/core/utils/CompositeChallengeHandler.java
Show resolved
Hide resolved
...ports/src/main/java/com/azure/tools/checkstyle/checks/ExceptionCreatedButNotThrownCheck.java
Outdated
Show resolved
Hide resolved
...ports/src/main/java/com/azure/tools/checkstyle/checks/ExceptionCreatedButNotThrownCheck.java
Outdated
Show resolved
Hide resolved
...-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/RawExceptionThrowCheck.java
Outdated
Show resolved
Hide resolved
...-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/RawExceptionThrowCheck.java
Outdated
Show resolved
Hide resolved
...-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/RawExceptionThrowCheck.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/com/azure/tools/checkstyle/checks/StringFormattedExceptionMessageCheck.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/com/azure/tools/checkstyle/checks/StringFormattedExceptionMessageCheck.java
Show resolved
Hide resolved
...uration-v2/src/main/java/com/azure/v2/data/appconfiguration/AzureAppConfigurationClient.java
Outdated
Show resolved
Hide resolved
sdk/clientcore/core/src/main/java/io/clientcore/core/utils/CompositeChallengeHandler.java
Show resolved
Hide resolved
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
} | ||
} | ||
|
||
private boolean hasGeneratedAnnotation(DetailAST methodDefAst) { |
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.
let's remove those so we catch not-so-good practices in codegen
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.
Awesome work - thanks Liudmila!
No description provided.