LoggingConfigurationFileLoader - Handle unknown type loading for App.config#6072
LoggingConfigurationFileLoader - Handle unknown type loading for App.config#6072
Conversation
WalkthroughThe changes refactor NLog's App.config loading mechanism by introducing a new NETFRAMEWORK-specific helper method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NLog/Config/LoggingConfigurationFileLoader.cs (1)
93-116: Method naming doesn't follow TryParse pattern.The method name
TryLoadFromAppConfigsuggests the TryParse pattern (returnsnullon failure, doesn't throw exceptions except fatal ones). However, this method throws exceptions conditionally on lines 104, 110, and 113, which violates that pattern and may confuse maintainers.Consider either:
- Option A: Rename to
LoadFromAppConfigto indicate it can throw- Option B: Refactor to never throw (return
nullon all errors) and remove the outer try/catch inLoad
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/NLog/Config/ConfigSectionHandler.cssrc/NLog/Config/LoggingConfigurationFileLoader.cssrc/NLog/LogFactory.cs
💤 Files with no reviewable changes (1)
- src/NLog/LogFactory.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Config/ConfigSectionHandler.cs (1)
src/NLog/Config/LoggingConfigurationFileLoader.cs (5)
LoggingConfiguration(56-91)LoggingConfiguration(94-116)LoggingConfiguration(121-133)LoggingConfiguration(172-194)LoggingConfiguration(196-199)
🔇 Additional comments (2)
src/NLog/Config/LoggingConfigurationFileLoader.cs (1)
118-118: Good practice: Centralized error message.The constant
FailedToLoadFromAppConfigMessagepromotes consistency in error reporting across the configuration loading paths.src/NLog/Config/ConfigSectionHandler.cs (1)
60-67: AppConfig property is already internal with proper error handling delegation.The
AppConfigproperty is currently marked asinternal static, and verification confirms there are no external code references to this property outside the NLog library (search returned no matches outside src/NLog/**). The only internal usage is inLoggingConfigurationFileLoader.cs:98within theTryLoadFromAppConfigmethod, which properly wraps the call with comprehensive error handling (lines 94-116). Exception handling includes separate catches forNLogConfigurationExceptionand generalExceptiontypes, with appropriate logging viaInternalLoggerand conditional re-throwing based onThrowExceptionsandThrowConfigExceptionsflags. The simplified getter using theascast operator is appropriate design since error handling is properly delegated to the caller.
| try | ||
| { | ||
| var appConfig = TryLoadFromAppConfig(logFactory); | ||
| if (appConfig != null) | ||
| return appConfig; | ||
| } | ||
| catch (NLogConfigurationException) | ||
| { | ||
| throw; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| if (ex.MustBeRethrownImmediately() || logFactory.ThrowExceptions || LogManager.ThrowExceptions) | ||
| throw; | ||
|
|
||
| InternalLogger.Error(ex, FailedToLoadFromAppConfigMessage); // NetStandard does not support .NET Framework types | ||
| } |
There was a problem hiding this comment.
Redundant error handling creates dead code.
The try/catch block wrapping TryLoadFromAppConfig appears redundant because TryLoadFromAppConfig already implements comprehensive exception handling (lines 96-116).
The condition on lines 73-74 is identical to the condition on lines 109-110 in TryLoadFromAppConfig. This means:
- When
TryLoadFromAppConfigthrows from line 110, the condition wastrue - Load catches it and evaluates the same condition (lines 73-74), which must also be
true - Therefore, line 76 (the error logging) is unreachable dead code
Additionally, the catch-and-rethrow of NLogConfigurationException (lines 67-70) serves no purpose.
🔎 Proposed fix: Remove redundant outer try/catch
#if NETFRAMEWORK
if (string.IsNullOrEmpty(filename))
{
- try
- {
- var appConfig = TryLoadFromAppConfig(logFactory);
- if (appConfig != null)
- return appConfig;
- }
- catch (NLogConfigurationException)
- {
- throw;
- }
- catch (Exception ex)
- {
- if (ex.MustBeRethrownImmediately() || logFactory.ThrowExceptions || LogManager.ThrowExceptions)
- throw;
-
- InternalLogger.Error(ex, FailedToLoadFromAppConfigMessage); // NetStandard does not support .NET Framework types
- }
+ var appConfig = TryLoadFromAppConfig(logFactory);
+ if (appConfig != null)
+ return appConfig;
}
#endif🤖 Prompt for AI Agents
In src/NLog/Config/LoggingConfigurationFileLoader.cs around lines 61 to 77, the
outer try/catch around TryLoadFromAppConfig is redundant and creates dead code
because TryLoadFromAppConfig already handles and rethrows exceptions
appropriately; remove the entire outer try/catch (including the
NLogConfigurationException catch and the general Exception catch that logs
FailedToLoadFromAppConfigMessage) and replace it with a simple call that returns
the result if non-null, allowing TryLoadFromAppConfig to perform its own
exception logic and rethrowing as designed.
|
snakefoot
changed the title
1 participant
Improve type-load-exception-handling if by accident using NetFramework DLL on NetStandard/NetCore-target.
Similar to what is done for AppEnvironmentWrapper in Safe-methods.