XmlParser to replace XmlReader to support AOT #5712
Conversation
WalkthroughThis update introduces a custom XML parser to replace the dependency on Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Env as AppEnvironmentWrapper
participant Loader as LoggingConfigurationFileLoader
participant Config as XmlLoggingConfiguration
participant Parser as XmlParser
App->>Loader: LoadXmlLoggingConfigurationFile(configFile)
Loader->>Env: LoadTextFile(configFile)
Env-->>Loader: TextReader
Loader->>Config: new XmlLoggingConfiguration(TextReader, configFile)
Config->>Parser: new XmlParser(TextReader)
Parser->>Parser: Parse XML document
Parser-->>Config: XmlParserElement (parsed XML)
Config->>Config: Wrap in XmlParserConfigurationElement
Config-->>Loader: XmlLoggingConfiguration instance
Loader-->>App: LoggingConfiguration
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (24)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (21)
🧰 Additional context used🧬 Code Graph Analysis (1)tests/NLog.UnitTests/Internal/XmlParserTests.cs (3)
🔇 Additional comments (15)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
src/NLog/Config/XmlLoggingConfiguration.cs (2)
239-241: Encourage explicit null-check on input parameters
While usingLoadTextFileis helpful for AOT compatibility, consider adding explicit checks forfileNameandtextReaderto guard against possible null references. This can help avoid confusion if the file path is invalid or if the environment fails to provide a validTextReader.
334-334: Match case-insensitivity more transparently
Usingcontent.Name.ToUpperInvariant()works, but a direct case-insensitive comparison may be more consistent for config names (e.g.,string.Equals(content.Name, "NLOG", StringComparison.OrdinalIgnoreCase)).src/NLog/Internal/XmlParser.cs (3)
34-60: Constructors promote AOT-friendly parsing
Providing overloads for bothTextReaderandstringensures flexibility in various environments, including AOT constraints. Make sure to handle potential null or empty values for both modes.
129-226: Processing instructions and element boundaries
The combined logic ofTryReadProcessingInstructions,TryReadStartElement, andTryReadEndElementappears thorough. Be mindful of supported XML features (e.g.DOCTYPE) that might appear in real-world configs, ensuring either graceful handling or explicit documentation of limitations.
586-605: XmlParserElement structure
StoringName,Attributes, andChildrenneatly encapsulates each node. The simple list model is straightforward, yet you might benefit from lazy-initializingChildrenor making it a read-only collection after parsing is complete.src/NLog/Config/XmlParserConfigurationElement.cs (3)
67-77: Consider caching the LINQ query result in the Values property.The LINQ query in the
Valuesproperty is executed every time the property is accessed, which could impact performance if accessed frequently.public IEnumerable<KeyValuePair<string, string>> Values { get { for (int i = 0; i < Children.Count; ++i) { var child = Children[i]; if (SingleValueElement(child)) { - // Values assigned using nested node-elements. Maybe in combination with attributes - return AttributeValues.Concat(Children.Where(item => SingleValueElement(item)).Select(item => new KeyValuePair<string, string>(item.Name, item.Value))); + // Cache the concatenated results to avoid re-executing LINQ on every access + return AttributeValues.Concat(Children.Where(SingleValueElement) + .Select(item => new KeyValuePair<string, string>(item.Name, item.Value))) + .ToList(); } } return AttributeValues; } }
80-93: Consider caching the LINQ query result in the Children property.Similar to the
Valuesproperty, the LINQ query in theILoggingConfigurationElement.Childrenproperty is executed on every access.IEnumerable<ILoggingConfigurationElement> ILoggingConfigurationElement.Children { get { for (int i = 0; i < Children.Count; ++i) { var child = Children[i]; if (!SingleValueElement(child)) - return Children.Where(item => !SingleValueElement(item)).Cast<ILoggingConfigurationElement>(); + return Children.Where(item => !SingleValueElement(item)) + .Cast<ILoggingConfigurationElement>() + .ToList(); } return ArrayHelper.Empty<ILoggingConfigurationElement>(); } }
113-171: Consider optimizing the XML namespace handling.The current implementation handles XML namespaces by string manipulation. Consider using a more robust approach for namespace handling.
private void Parse(XmlParser.XmlParserElement xmlElement, bool nestedElement, out IList<KeyValuePair<string, string>> attributes, out IList<XmlParserConfigurationElement> children) { - var namePrefixIndex = xmlElement.Name.IndexOf(':'); - Name = namePrefixIndex >= 0 ? xmlElement.Name.Substring(namePrefixIndex + 1) : xmlElement.Name; + // Extract local name from qualified name + Name = GetLocalName(xmlElement.Name); Value = xmlElement.InnerText; attributes = xmlElement.Attributes; if (attributes?.Count > 0) { if (!nestedElement) { for (int i = attributes.Count - 1; i >= 0; --i) { var attributeName = attributes[i].Key; if (IsSpecialXmlRootAttribute(attributeName)) { attributes.RemoveAt(i); } } } for (int j = 0; j < attributes.Count; ++j) { - var attributePrefixIndex = attributes[j].Key.IndexOf(':'); - if (attributePrefixIndex >= 0) - attributes[j] = new KeyValuePair<string, string>(attributes[j].Key.Substring(attributePrefixIndex + 1), attributes[j].Value); + attributes[j] = new KeyValuePair<string, string>(GetLocalName(attributes[j].Key), attributes[j].Value); } } // ... rest of the method } +private static string GetLocalName(string qualifiedName) +{ + var namePrefixIndex = qualifiedName.IndexOf(':'); + return namePrefixIndex >= 0 ? qualifiedName.Substring(namePrefixIndex + 1) : qualifiedName; +}tests/NLog.UnitTests/Internal/XmlParserTests.cs (1)
290-298: Consider adding memory usage assertion to the deep nesting test.While testing for stack overflow is good, it would be valuable to also verify that memory usage remains reasonable.
[Fact] public void XmlParse_DeeplyNestedXml_DoesNotThrowStackOverflowException() { + var initialMemory = GC.GetTotalMemory(true); string deeplyNestedXml = "<root>" + string.Join("", Enumerable.Repeat("<a>", 10000).ToArray()) + string.Join("", Enumerable.Repeat("</a>", 10000).ToArray()) + "</root>"; var xmlDocument = new XmlParser(deeplyNestedXml).LoadDocument(out var _); Assert.NotNull(xmlDocument); Assert.Equal("root", xmlDocument.Name); Assert.Single(xmlDocument.Children); + var finalMemory = GC.GetTotalMemory(true); + Assert.True((finalMemory - initialMemory) < 10_000_000, "Memory usage should be reasonable"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(4 hunks)src/NLog/Config/XmlLoggingConfigurationElement.cs(0 hunks)src/NLog/Config/XmlParserConfigurationElement.cs(1 hunks)src/NLog/Config/XmlParserException.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/IFileSystem.cs(2 hunks)src/NLog/Internal/XmlParser.cs(1 hunks)tests/NLog.AutoReloadConfig.Tests/NLog.AutoReloadConfig.Tests.csproj(1 hunks)tests/NLog.Database.Tests/NLog.Database.Tests.csproj(1 hunks)tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj(1 hunks)tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj(1 hunks)tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj(1 hunks)tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj(1 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(1 hunks)tests/NLog.UnitTests/Config/XmlConfigTests.cs(1 hunks)tests/NLog.UnitTests/ConfigFileLocatorTests.cs(6 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs(3 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs(1 hunks)tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs(3 hunks)tests/NLog.UnitTests/NLog.UnitTests.csproj(1 hunks)tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj(1 hunks)
💤 Files with no reviewable changes (1)
- src/NLog/Config/XmlLoggingConfigurationElement.cs
✅ Files skipped from review due to trivial changes (1)
- tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj
🔇 Additional comments (32)
tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj (1)
14-15: Updated Testing Framework DependenciesThe package references for
Microsoft.NET.Test.Sdkandxunithave been updated to versions17.13.0and2.9.3respectively. This aligns with the coordinated upgrade across test projects. Please ensure these versions are compatible with all tests in this suite.tests/NLog.AutoReloadConfig.Tests/NLog.AutoReloadConfig.Tests.csproj (1)
14-15: Upgraded Test Package VersionsThe dependencies for
Microsoft.NET.Test.Sdkandxunithave been updated to17.13.0and2.9.3respectively. This standardization should help maintain consistency across projects. Confirm that these changes do not introduce incompatibilities.tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj (1)
19-20: Revised Testing Dependencies for Mail TargetsThe
Microsoft.NET.Test.Sdkandxunitpackage versions have been updated to17.13.0and2.9.3respectively. This update is essential for keeping the test framework current. Please run a full test suite to verify that integration with the mail target functionality remains unaffected.tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj (1)
19-20: Consistent Upgrade of Test DependenciesThe references for
Microsoft.NET.Test.Sdkandxunitare now at versions17.13.0and2.9.3respectively. This ensures consistency across testing projects. Verify that this update integrates smoothly with the Windows Registry test scenarios.tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj (1)
19-20: Standardized Test Dependency VersionsThe test project now references
Microsoft.NET.Test.Sdkversion17.13.0andxunitversion2.9.3. Additionally, the project includes a reference toNSubstituteversion5.1.0. Please confirm that these versions work well together and that all network target tests pass successfully.tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj (2)
19-19: Verify the updated test SDK package version.The
Microsoft.NET.Test.Sdkpackage version has been updated to17.13.0. Please confirm that this version is fully compatible with the overall test infrastructure and any potential integration requirements.
20-20: Verify the updated xUnit package version.The
xunitpackage version has been updated to2.9.3, which should include the latest fixes and features. Ensure that this update aligns with the project’s testing strategy and remains consistent with dependencies across related test projects.tests/NLog.Database.Tests/NLog.Database.Tests.csproj (2)
27-27: Confirm the updated test SDK package version.The
Microsoft.NET.Test.Sdkpackage reference is now at17.13.0. This update promotes consistency across the test projects—please verify that it integrates well with the existing test setup.
28-28: Confirm the updated xUnit package version.The
xunitpackage reference has been revised to2.9.3. Ensure that this update is compatible with the project's tests and matches the other test project configurations.tests/NLog.UnitTests/NLog.UnitTests.csproj (1)
46-46: Standardize the test SDK package version.The
Microsoft.NET.Test.Sdkpackage is updated from17.12.0to17.13.0, aligning it with the other test projects in the solution. This promotes a consistent testing environment across the board.tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs (1)
130-130:Details
❓ Verification inconclusive
LGTM! Simplified stack trace setting.
The change removes the frame index parameter from
SetStackTrace, making the API simpler to use while maintaining the same functionality.Let's verify that this change is consistent across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining usages of SetStackTrace with two parameters ast-grep --pattern $'SetStackTrace($_, $_)'Length of output: 44
LGTM! Simplified stack trace handling.
The removal of the frame index parameter from
SetStackTraceappears to have eliminated any two-parameter calls. However, our initial automated search using AST grep yielded no output, which might indicate either that no such usages remain or the pattern match didn’t capture all cases. Please manually verify that there are no edge cases or unintended two-parameter calls lingering in the codebase.tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs (1)
108-108: LGTM! Consistent API usage.The change aligns with the updated
SetStackTraceAPI signature, maintaining consistency across test files.tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs (1)
114-114: LGTM! Consistent API update.The change follows the same pattern of simplifying the
SetStackTraceAPI usage, maintaining consistency across the test suite.tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs (1)
256-256: LGTM! Consistent API simplification.The change completes the systematic update to the
SetStackTraceAPI usage across all test files.src/NLog/Config/XmlLoggingConfiguration.cs (4)
249-249: Maintain consistency in method naming
The new invocationParseFromTextReaderfits well with the rest of the refactoring. Good job aligning it with the text-based parsing approach.
283-305: Ensure thorough error handling and potential edge cases
The newParseFromTextReadermethod introduces robust exception handling viaNLogConfigurationException. However, consider verifying scenarios where the XML content might be empty or only contain comments/processing instructions. It may be beneficial to add at least one test covering these edge cases.
330-330: Good interface-based design
ReplacingXmlLoggingConfigurationElementwithILoggingConfigurationElementas a parameter fosters better abstraction and maintains flexibility for evolving parsing strategies.
352-352: Improved modularity through interface usage
This change further unifies the interface-driven approach for parsing. The refactoring aroundILoggingConfigurationElementindicates a cleaner architectural separation.src/NLog/Internal/XmlParser.cs (4)
62-127: Robust root element detection
TheLoadDocumentmethod properly validates and detects the root start-tag. The stack-based approach is a clear solution for nested parsing. However, consider adding checks for multiple root-level elements or none at all.
228-322: CDATA, comments, and inline text
These parsing functions handle complex XML constructs well. Verify that large CDATA sections or unexpectedly placed comments also behave as intended (e.g. in the middle of text or nested in unusual configurations).
324-556: Attribute reading and character handling
The parsing logic for attributes, special tokens, and character references is quite detailed. Consider performing thorough testing for numerical references ({) and hex-based references ({) to ensure correct decoding, especially if encountering uncommon Unicode characters.
607-684: CharEnumerator
This hand-crafted enumerator properly tracks both current character and peeked character, along with line numbers. Great care is taken to detect end-of-file conditions. Just remain cautious that newlines may arise in different forms (\r\nvs\n).src/NLog/Internal/IFileSystem.cs (2)
47-49: Specific XML loading documentation
Renaming the summary forLoadXmlFileto emphasize XML usage is a positive clarification. This explicit naming better conveys its intended purpose.
51-52: New text-reading method
IntroducingLoadTextFilealigns with the revised parsing strategy and fosters consistency across different input scenarios. It's also helpful for future expansions that rely on plain text.src/NLog/Config/XmlParserException.cs (1)
41-68: LGTM! Well-structured exception class.The implementation follows best practices for custom exceptions with proper inheritance, constructors, and documentation.
tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs (1)
44-53: LGTM! Well-structured mock improvements.The changes enhance the mock's flexibility with:
- Optional parameters with sensible defaults
- Clear error messages for unsupported operations
- Consistent implementation of file system operations
Also applies to: 87-95
tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs (1)
91-91: LGTM! Simplified mock instantiation.The changes appropriately leverage the new optional parameters in AppEnvironmentMock's constructor.
Also applies to: 105-105, 123-123
src/NLog/Config/XmlParserConfigurationElement.cs (1)
41-78: LGTM! Well-structured class with clear property definitions.The class is well-organized with appropriate XML documentation and clear property definitions. The
Valuesproperty efficiently handles both attribute values and single-value child elements.tests/NLog.UnitTests/Internal/XmlParserTests.cs (1)
43-127: LGTM! Comprehensive test coverage for invalid XML documents.The test cases cover a wide range of invalid XML scenarios, including malformed tags, mismatched quotes, and invalid characters.
tests/NLog.UnitTests/Config/XmlConfigTests.cs (1)
249-249: LGTM! Appropriate test update for XML namespace support.The change from
type='Debug'toxsi:type='Debug'correctly tests the handling of namespaced attributes.tests/NLog.UnitTests/ConfigFileLocatorTests.cs (1)
146-146: LGTM! Consistent refactoring for AOT support.The changes consistently remove the XmlReader-based file loading parameter from
AppEnvironmentMockconstructor across all test methods, aligning with the PR's objective of replacing XmlReader with XmlParser for AOT support.Also applies to: 164-164, 191-191, 219-219, 246-246, 277-277
tests/NLog.UnitTests/Config/LogFactorySetupTests.cs (1)
709-709: LGTM! Consistent with XmlReader to XmlParser migration.The change aligns with the removal of XmlReader-based file loading from
AppEnvironmentMockconstructor, maintaining consistency with other test files.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/NLog/Config/LoggingConfigurationElementExtensions.cs (2)
129-134:⚠️ Potential issueResolve merge conflict and include null checks.
There's an unresolved merge conflict in the code. The stashed changes contain important null checks that should be included to prevent potential NullReferenceException.
Resolve the conflict by keeping the stashed changes:
-<<<<<<< Updated upstream -======= - if (elementName is null || element?.Children is null) - yield break; - ->>>>>>> Stashed changes + if (elementName is null || element?.Children is null) + yield break;
137-138:⚠️ Potential issueAdd null check for childElement.Name to prevent NullReferenceException.
The current implementation assumes childElement.Name is not null.
Apply this change:
- if (childElement.Name.Equals(elementName, StringComparison.OrdinalIgnoreCase)) + if (childElement?.Name?.Equals(elementName, StringComparison.OrdinalIgnoreCase) == true)
🧹 Nitpick comments (1)
src/NLog/Config/LoggingConfigurationElementExtensions.cs (1)
124-126: Enhance method documentation.While the current documentation provides a basic description, it would be more helpful to include:
- Parameter descriptions
- Return value description
- Example usage
Consider expanding the documentation:
/// <summary> /// Returns children elements with the specified element name. /// </summary> +/// <param name="element">The configuration element to filter children from.</param> +/// <param name="elementName">The name of the child elements to filter by.</param> +/// <returns>An enumerable of child elements matching the specified name.</returns> +/// <example> +/// var targets = config.FilterChildren("targets"); +/// </example>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NLog/Config/XmlLoggingConfiguration.cs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/NLog/Internal/AppEnvironmentWrapper.cs (1)
105-108:⚠️ Potential issueAdd proper error handling and resource management.
The current implementation doesn't handle file access errors and resource cleanup properly. Also, the path should be normalized using
FixFilePathWithLongUNCfor consistency.Apply this diff to fix the issues:
public TextReader LoadTextFile(string path) { - return new StreamReader(path); + try + { + path = FixFilePathWithLongUNC(path); + return new StreamReader(path); + } + catch (Exception ex) + { + if (ex.MustBeRethrownImmediately()) + throw; + InternalLogger.Debug(ex, "LoadTextFile Failed"); + throw; + } }
🧹 Nitpick comments (4)
src/NLog/Internal/IFileSystem.cs (1)
46-48: Consider clarifying usage contract for the new text-file loading method.
Returning aTextReaderis flexible, but ensure callers understand their responsibility to dispose of the reader properly (e.g., via ausingblock). An asynchronous variant (e.g.,LoadTextFileAsync) could be beneficial for large file reads.src/NLog/Config/LoggingConfigurationFileLoader.cs (1)
158-197: Consider using a more robust XML validation approach.The current implementation uses string scanning to detect XML attributes, which could be fragile. Consider using a more robust approach for detecting XML attributes.
Apply this diff to improve the validation:
- if (invalidXml) - { - // Avoid reacting to throwExceptions="true" that only exists in comments, only check when invalid xml - if (ScanForBooleanParameter(fileContent, "throwExceptions", true)) - { - logFactory.ThrowExceptions = true; - return true; - } - - if (ScanForBooleanParameter(fileContent, "throwConfigExceptions", true)) - { - logFactory.ThrowConfigExceptions = true; - return true; - } - } + if (invalidXml) + { + try + { + using var stringReader = new StringReader(fileContent); + var config = new XmlParserConfigurationElement(stringReader); + var throwExceptions = config.GetOptionalBooleanValue("throwExceptions", false); + var throwConfigExceptions = config.GetOptionalBooleanValue("throwConfigExceptions", false); + if (throwExceptions) + { + logFactory.ThrowExceptions = true; + return true; + } + if (throwConfigExceptions) + { + logFactory.ThrowConfigExceptions = true; + return true; + } + } + catch + { + // Fallback to string scanning if XML parsing fails + if (ScanForBooleanParameter(fileContent, "throwExceptions", true)) + { + logFactory.ThrowExceptions = true; + return true; + } + if (ScanForBooleanParameter(fileContent, "throwConfigExceptions", true)) + { + logFactory.ThrowConfigExceptions = true; + return true; + } + } + }src/NLog/Internal/AppEnvironmentWrapper.cs (1)
105-105: Add XML documentation for the public method.The public method lacks XML documentation which is important for API consumers.
Add XML documentation:
+/// <summary> +/// Loads a text file from the specified path and returns a TextReader. +/// </summary> +/// <param name="path">The path to the text file.</param> +/// <returns>A TextReader for reading the file contents.</returns> +/// <exception cref="Exception">Thrown when the file cannot be loaded.</exception> public TextReader LoadTextFile(string path)src/NLog/Config/XmlLoggingConfiguration.cs (1)
325-347: Enhance error handling with more detailed messages.The error message could be more descriptive to help users diagnose configuration issues.
Apply this diff to improve error messages:
- var configurationException = new NLogConfigurationException($"Exception when loading configuration {fileName}", exception); + var configurationException = new NLogConfigurationException( + $"Failed to load NLog configuration from {(string.IsNullOrEmpty(fileName) ? "stream" : $"file: {fileName}")}. " + + $"Error: {exception.Message}", exception);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/NLog/Config/ConfigSectionHandler.cs(1 hunks)src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(3 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(6 hunks)src/NLog/Config/XmlLoggingConfigurationElement.cs(3 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/IFileSystem.cs(2 hunks)src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/NLogTraceListener.cs(1 hunks)src/NLog/SetupBuilderExtensions.cs(2 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(5 hunks)tests/NLog.UnitTests/ConfigFileLocatorTests.cs(8 hunks)tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs(3 hunks)tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/NLog/Config/ConfigSectionHandler.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs
- tests/NLog.UnitTests/ConfigFileLocatorTests.cs
- tests/NLog.UnitTests/Config/LogFactorySetupTests.cs
- tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs
🔇 Additional comments (19)
tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs (1)
57-57: LGTM! Simplified XML configuration loading.The change to use
XmlLoggingConfiguration.CreateFromXmlStringsimplifies the code by removing intermediate XML parsing steps while maintaining the same functionality. This aligns well with the PR's objective of replacing XmlReader to support AOT compilation.src/NLog/SetupBuilderExtensions.cs (4)
38-38: LGTM!The added namespace import is necessary for using StreamReader and is correctly placed in alphabetical order.
211-211: LGTM!The simplified logging call improves readability by removing the redundant namespace prefix.
219-219: LGTM!The simplified logging calls improve readability by removing the redundant namespace prefix.
Also applies to: 224-224, 228-228
212-215:Details
❓ Verification inconclusive
LGTM! Verify constructor compatibility.
The change from XmlReader to StreamReader aligns with the PR objective to support AOT compilation. The StreamReader is properly disposed using a
usingstatement.Let's verify that the XmlLoggingConfiguration constructor accepting TextReader exists and is properly implemented:
🏁 Script executed:
#!/bin/bash # Search for XmlLoggingConfiguration constructor that accepts TextReader ast-grep --pattern 'class XmlLoggingConfiguration { $$$ public XmlLoggingConfiguration(TextReader $_, $_, $_) { $$$ } $$$ }'Length of output: 138
LGTM—Pending Manual Verification of Constructor Signature
The change from XmlReader to StreamReader meets the PR objective for AOT compatibility and ensures proper resource disposal with the using statement. However, our automated search for the
XmlLoggingConfiguration(TextReader, …)constructor did not return any results. Please manually verify that this constructor exists and accepts aTextReaderas its first parameter along with the expected additional parameters.
- Action: Confirm that
XmlLoggingConfigurationhas a constructor with the following signature (or equivalent):public XmlLoggingConfiguration(TextReader reader, object unusedParam, LogFactory logFactory)- Location:
src/NLog/SetupBuilderExtensions.cs, Lines: 212-215src/NLog/Internal/XmlHelper.cs (6)
45-50: Surrogate constant declarations appear correct.
These range definitions for Unicode surrogates align with standard Unicode ranges for high and low surrogate characters.
51-66: Ensure the XML character validation logic is accurate.
TheXmlConvertIsXmlChar(char chr)method checks for valid characters based on standard XML requirements, including allowing\t,\n, and\r. Double-check that this aligns with the most recent XML 1.0 specification and your requirements for special control characters.
68-81: Surrogate helper methods are straightforward and well-defined.
XmlConvertIsHighSurrogate,XmlConvertIsLowSurrogate, andXmlConvertIsXmlSurrogatePairneatly encapsulate surrogate pair checks. Consistent naming clarifies their purpose.
255-270: Consistent numeric string conversions.
UsingEnsureDecimalPlacealongside"R"or invariant formatting provides a reliable representation for float, double, and decimal values. Verified that this approach avoids losing precision while ensuring at least one decimal place is shown.
272-276: UTC conversion for DateTime may be unexpected in some contexts.
Converting all DateTime values to UTC before formatting ensures a consistent time zone representation. Confirm that you intend to override local times for all scenarios, as this might strip local context.
413-429: Switch-case numeric handling looks consistent.
UsingToString(null, NumberFormatInfo.InvariantInfo)for integer types and returning lowercase for booleans is coherent. DateTime handling also delegates to the newXmlConvertToString(DateTime). Ensure any new numeric types (e.g., BigInteger) receive similar handling if introduced later.Also applies to: 437-437
src/NLog/Targets/ConsoleTargetHelper.cs (1)
53-53: Validate .NET framework condition.
The switch from#if !NETSTANDARDto#if NETFRAMEWORKnarrows the compilation scope. Confirm that any .NET Core or future .NET versions requiring the special Mono check won't be affected by this new condition.src/NLog/Internal/AssemblyHelpers.cs (1)
67-73:Details
❓ Verification inconclusive
Verify the warning suppression for obsolete Uri.TryCreate usage.
The warning suppression is necessary as
Uri.CodeBaseis obsolete. However, let's verify if there's a recommended alternative.
🌐 Web query:
What is the recommended alternative for Uri.CodeBase in .NET?💡 Result:
The recommended alternative for
Uri.CodeBasein modern .NET (Core 5+) is to useAssembly.Locationinstead[8][9]. Microsoft has markedCodeBaseandEscapedCodeBaseas obsolete for .NET Framework compatibility only, advising developers to switch toLocation[8].Key differences:
Assembly.Location: Returns the absolute path to the loaded assembly file in standard filesystem formatAssembly.CodeBase: Returns a URI string (file:// URI scheme) which requires additional parsing and decodingExample replacement:
// Obsolete approach var codeBase = typeof(MyClass).Assembly.CodeBase; // Modern approach var location = typeof(MyClass).Assembly.Location;Important considerations:
- For assemblies loaded from byte arrays (in-memory),
Locationreturns an empty string whileCodeBasewould throw exceptions[8]- Special characters in paths are handled more reliably with
Location[4]- No URI decoding required when using
LocationFor ASP.NET Core applications needing URL building capabilities, consider using dedicated builder patterns like
UrlHelperor custom implementations like the Immutable Fluent Builder pattern shown in search results[10].Citations:
- 1: https://endjin.com/blog/2022/10/high-performance-csharp-uritemplates
- 2: https://vjeko.com/2020/11/23/how-to-replace-dotnet-in-al/
- 3: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/url-rewriting?view=aspnetcore-9.0
- 4: https://corengen.wordpress.com/2011/08/03/assembly-location-and-codebase/
- 5: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2234
- 6: https://jeremydmiller.com/2018/05/21/an-alternative-style-of-routing-for-asp-net-core/
- 7: https://learn.microsoft.com/en-us/dotnet/api/system.uritemplate?view=netframework-4.8.1
- 8: https://learn.microsoft.com/en-us/dotnet/api/system.reflection.assembly.codebase?view=net-9.0
- 9: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1056
- 10: https://blog.ploeh.dk/2020/08/10/an-aspnet-core-url-builder/
Attention: Consider Migrating from
Uri.CodeBasetoAssembly.LocationThe current suppression for the obsolete call is valid given that
Uri.CodeBaseis deprecated. However, modern .NET guidance recommends usingAssembly.Locationinstead, which provides the absolute path without requiring additional URI decoding. Please verify whether switching toAssembly.Locationis feasible in this context—especially considering that assemblies loaded from byte arrays might return an empty string withLocation—or if the existing warning suppression remains the best option.
- File:
src/NLog/Internal/AssemblyHelpers.cs- Lines: 67-73
- Suggestion: Evaluate replacing the obsolete usage:
- Obsolete:
if (!Uri.TryCreate(assembly.CodeBase, UriKind.RelativeOrAbsolute, out var assemblyCodeBase)) { … }- Modern: Consider using
Assembly.Locationto obtain the assembly's path, ensuring that any special handling for in-memory assemblies is preserved.src/NLog/Config/LoggingConfigurationElementExtensions.cs (1)
124-139: LGTM! Null checks have been added as suggested.The implementation is robust with proper null checks and case-insensitive comparison.
src/NLog/Config/XmlLoggingConfigurationElement.cs (1)
34-48: LGTM! Class correctly marked as obsolete.The changes align with the PR objective to replace XmlReader with TextReader. The conditional compilation ensures backward compatibility for .NET Framework.
src/NLog/Config/LoggingConfigurationFileLoader.cs (1)
134-150:Details
✅ Verification successful
Verify error handling for XmlParserException.
The error handling has been simplified to use a boolean flag for invalid XML. However, we should verify that all XML parsing errors are properly caught.
🏁 Script executed:
#!/bin/bash # Description: Search for all XML parsing error handling in the codebase # Search for XML parsing error handling rg -A 5 'XmlParserException'Length of output: 16156
XML Parser Error Handling – Verified
After reviewing the XML parser error handling across the repository, all XML parsing issues that throw an
XmlParserExceptionare correctly caught and processed. In particular, the code insrc/NLog/Config/LoggingConfigurationFileLoader.cs:
- Catches any exception and then uses a boolean check (
ex is XmlParserException) to determine if it should invokeThrowXmlConfigExceptions, ensuring that XML-specific errors are identified.- Appropriately either rethrows the exception when necessary or falls back to creating an empty default configuration.
Furthermore, the definition and usage of
XmlParserExceptionacross the codebase (e.g., insrc/NLog/Internal/XmlParser.cs) and the corresponding unit tests intests/NLog.UnitTests/Internal/XmlParserTests.csconfirm that XML parsing errors are properly raised and handled.No additional changes are required based on the current verification.
src/NLog/NLogTraceListener.cs (1)
579-579: LGTM! Simplified boolean parsing.The change from
XmlConvert.ToBooleantobool.Parseremoves unnecessary XML dependency while maintaining the same functionality.src/NLog/Config/XmlLoggingConfiguration.cs (2)
89-115: LGTM! Well-structured TextReader support.The new constructors provide a clean way to initialize configuration from TextReader while maintaining backward compatibility through obsolete attributes.
276-281: LGTM! Proper validation and resource management.The implementation correctly validates the fileName parameter and ensures proper disposal of the TextReader through the using statement.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/NLog/Internal/XmlHelper.cs (2)
86-128: Add documentation for surrogate pair handling.While the implementation is correct and efficient, it would be helpful to add a comment explaining why surrogate pairs are skipped in
CreateValidXmlString. This would help future maintainers understand the design decision.private static string CreateValidXmlString(string text) { + // Note: Surrogate pairs are handled by RemoveInvalidXmlChars, so we can safely skip them here var sb = new StringBuilder(text.Length); for (int i = 0; i < text.Length; ++i) {
272-279: Add XML documentation for DateTime conversion method.The implementation correctly handles UTC conversion and ISO 8601 formatting, but would benefit from XML documentation explaining the behavior.
+ /// <summary> + /// Converts DateTime to ISO 8601 format in UTC timezone. + /// </summary> + /// <param name="value">The DateTime value to convert.</param> + /// <returns>ISO 8601 formatted string in UTC timezone.</returns> internal static string XmlConvertToString(DateTime value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/NLogTraceListener.cs(2 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(5 hunks)tests/NLog.UnitTests/Config/ReloadTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/NLog.UnitTests/Config/LogFactorySetupTests.cs
- src/NLog/NLogTraceListener.cs
🔇 Additional comments (4)
tests/NLog.UnitTests/Config/ReloadTests.cs (1)
682-684: LGTM! The test data has been improved.The changes to the invalid XML configuration string are beneficial:
- Added
autoReload='true'to test auto-reload functionality- Fixed XML syntax by correcting the closing tag from
</targets>to</target>- Removed duplicate
internalLogLevelattributesrc/NLog/Internal/XmlHelper.cs (3)
45-81: LGTM! Well-structured XML character validation implementation.The implementation correctly handles XML character validation according to Unicode standards, with clear separation of concerns between high surrogate, low surrogate, and general XML character validation.
251-270: LGTM! Efficient numeric type conversion implementation.The implementation correctly handles numeric type conversions with proper decimal place handling and culture-invariant formatting. The removal of unnecessary NaN checks improves efficiency.
415-439: LGTM! Consistent use of invariant culture in type conversions.The implementation correctly handles all type conversions with proper culture handling and consistent use of
NumberFormatInfo.InvariantInfo. The explicit lowercase boolean values and proper DateTime handling are nice touches.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/NLog/Internal/XmlHelper.cs (2)
86-128: Add XML documentation for better maintainability.While the implementation is correct, adding XML documentation for the
CreateValidXmlStringmethod would improve maintainability.+ /// <summary> + /// Creates a new string containing only valid XML characters. + /// </summary> + /// <param name="text">The input text to clean.</param> + /// <returns>A new string with only valid XML characters.</returns> private static string CreateValidXmlString(string text)
251-279: Extract DateTime format string into a constant.Consider extracting the DateTime format string into a constant for better maintainability and reusability.
+ private const string DateTimeFormat = "yyyy-MM-ddTHH:mm:ss.FFFFFFFK"; + internal static string XmlConvertToString(DateTime value) { if (value.Kind == DateTimeKind.Unspecified) value = new DateTime(value.Ticks, DateTimeKind.Utc); else value = value.ToUniversalTime(); - return value.ToString("yyyy-MM-ddTHH:mm:ss.FFFFFFFK"); + return value.ToString(DateTimeFormat); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Internal/XmlHelper.cs(6 hunks)
🔇 Additional comments (2)
src/NLog/Internal/XmlHelper.cs (2)
45-81: LGTM! Well-structured XML character validation.The implementation correctly handles XML character validation according to Unicode standards, with clear separation of concerns between different validation methods.
415-439: LGTM! Comprehensive type conversion implementation.The implementation correctly handles all primitive types with proper invariant culture formatting and XML compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/NLog.UnitTests/Internal/XmlParserTests.cs (1)
1-315: Consider adding more test cases.While the current test coverage is good, consider adding tests for:
- XML with mixed content (text and elements)
- Processing instructions with complex content
- Error cases for surrogate pairs
- Performance tests with large XML documents
- Memory usage tests
src/NLog/Internal/XmlHelper.cs (1)
251-270: Consider optimizing float/double/decimal string conversion.The current implementation creates intermediate strings. Consider using a StringBuilder pool for better performance with high-volume logging.
-return EnsureDecimalPlace(value.ToString(null, NumberFormatInfo.InvariantInfo)); +return StringBuilderPool.Instance.Acquire(out var builder) + .AppendDecimal(value) + .EnsureDecimalPlace() + .Return();src/NLog/Internal/XmlParser.cs (2)
574-586: Consider using a readonly field for special tokens.The
_specialTokensdictionary should be marked as readonly since it's initialized only once.-private static Dictionary<string, string> _specialTokens = new Dictionary<string, string>() +private static readonly Dictionary<string, string> _specialTokens = new Dictionary<string, string>()
516-552: Consider adding bounds checking for Unicode values.The Unicode parsing methods should validate that the resulting value is within valid Unicode ranges.
private int TryParseUnicodeValue() { int unicode = '\0'; while (_xmlSource.MoveNext()) { if (_xmlSource.Current == ';') break; if (_xmlSource.Current < '0' || _xmlSource.Current > '9') throw new XmlParserException("Invalid XML document. Cannot parse unicode-char digit-value"); unicode *= 10; unicode += _xmlSource.Current - '0'; } + if (unicode > 0x10FFFF) + throw new XmlParserException("Invalid XML document. Unicode value exceeds maximum allowed value"); + return unicode; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/Internal/XmlParser.cs(1 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)
🔇 Additional comments (14)
tests/NLog.UnitTests/Internal/XmlParserTests.cs (7)
43-144: LGTM! Comprehensive test coverage for invalid XML documents.The test cases thoroughly cover various edge cases and invalid XML formats, including:
- Empty or whitespace-only content
- Malformed tags and processing instructions
- Invalid attribute formats
- Mismatched case in tags
- Nested tag errors
146-174: LGTM! Good coverage of empty document scenarios.Test cases verify proper handling of:
- Self-closing tags
- Empty elements with different whitespace
- Case sensitivity
- Processing instructions
176-199: LGTM! Thorough attribute parsing tests.Test cases verify proper handling of:
- Single and double quotes
- Whitespace variations
- Line breaks in attributes
219-243: LGTM! Comprehensive attribute token tests.Test cases verify proper handling of special characters and paths in attributes:
- Empty values
- File paths with special characters
- URLs
- XML entities
245-273: LGTM! Thorough inner text token tests.Test cases verify proper handling of:
- Empty content
- XML entities
- CDATA sections
- Comments
275-287: LGTM! Good coverage of child element scenarios.Test cases verify proper handling of:
- Single child elements
- Attribute-based and element-based child configurations
- Mixed formats
Also applies to: 289-305
307-315: LGTM! Important test for deeply nested XML.The test verifies that the parser can handle deeply nested XML without stack overflow.
src/NLog/Internal/XmlHelper.cs (4)
45-50: LGTM! Well-defined constants for surrogate ranges.Clear and descriptive constant names for Unicode surrogate ranges.
51-66: LGTM! Robust XML character validation.The method correctly validates XML characters according to the XML 1.0 specification:
- Basic ASCII range
- Control characters
- Surrogate ranges
- Invalid Unicode values
68-81: LGTM! Proper surrogate pair validation.Methods correctly identify high and low surrogate characters and validate surrogate pairs.
272-282: LGTM! Proper DateTime handling for XML.The method correctly:
- Handles unspecified DateTimeKind
- Converts to UTC
- Uses ISO 8601 format
src/NLog/Internal/XmlParser.cs (3)
44-47: LGTM! Well-structured parser design.The parser implementation:
- Uses a clean class structure
- Provides clear documentation
- Efficiently manages XML element state
Also applies to: 602-621
62-127: LGTM! Robust document loading with error handling.The LoadDocument method:
- Properly handles processing instructions
- Uses a stack for nested elements
- Provides detailed error messages with line numbers
623-700: LGTM! Efficient character enumeration.The CharEnumerator:
- Tracks line numbers for error reporting
- Efficiently handles peek operations
- Properly implements IEnumerator
|
Discovered that with #5715 that System.Xml doesn't generate AOT-warnings, but do introduce a 3 MByte App-Size-Increase. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
src/NLog/Config/LoggingConfigurationElementExtensions.cs (1)
124-139: Consider null checks within the child iteration logic.While the code checks for
element?.Children, it doesn't perform an explicit null check onchildElement.Name. Defensive coding could avoid potentialNullReferenceExceptionif a malformed element is encountered.if (childElement != null && childElement.Name != null && childElement.Name.Equals(elementName, StringComparison.OrdinalIgnoreCase)) { yield return childElement; }src/NLog/Config/XmlParserConfigurationElement.cs (1)
105-108: Check for deeper error handling or fallback.Parsing attribute names and child elements may throw unexpected exceptions (e.g., invalid encoding in
XmlParserElement). Consider thorough exception handling to maintain robust parsing.src/NLog/Internal/XmlParser.cs (4)
62-127: Consider implementing a depth limit to prevent stack overflowThe current implementation uses a Stack to track nested elements, but there's no check for deeply nested XML which could lead to a stack overflow or excessive memory usage in pathological cases.
While you have a test for deeply nested XML, it's good practice to add a safety mechanism.
public XmlParserElement LoadDocument(out IList<XmlParserElement> processingInstructions) { try { TryReadProcessingInstructions(out processingInstructions); if (!TryReadStartElement(out var rootName, out var rootAttributes)) throw new XmlParserException("Invalid XML document. Cannot parse root start-tag"); Stack<XmlParserElement> stack = new Stack<XmlParserElement>(); + const int MaxNestingDepth = 100; // Define a reasonable limit var currentRoot = new XmlParserElement(rootName, rootAttributes); stack.Push(currentRoot); bool stillReading = true; while (stillReading) { stillReading = false; if (TryReadEndElement(currentRoot.Name)) { stillReading = true; stack.Pop(); if (stack.Count == 0) break; currentRoot = stack.Peek(); } try { if (TryReadInnerText(out var innerText)) { stillReading = true; currentRoot.InnerText += innerText; } if (TryReadStartElement(out var elementName, out var elementAttributes)) { stillReading = true; + if (stack.Count >= MaxNestingDepth) + throw new XmlParserException($"XML nesting too deep (>{MaxNestingDepth}). Possible malicious XML."); currentRoot = new XmlParserElement(elementName, elementAttributes); stack.Peek().AddChild(currentRoot); stack.Push(currentRoot); } } catch (XmlParserException ex) { throw new XmlParserException(ex.Message + $" - Start-tag: {currentRoot.Name}"); } }
269-298: Optimize CDATA reading for better performanceThe current CDATA reading implementation appends characters one by one to the StringBuilder, which can be inefficient for large CDATA sections. Consider reading chunks until you find the potential end marker.
private string ReadCDATA() { string contentValue; if (!SkipChar('!') || !SkipChar('[') || !SkipChar('C') || !SkipChar('D') || !SkipChar('A') || !SkipChar('T') || !SkipChar('A') || !SkipChar('[')) throw new XmlParserException("Invalid XML document. Cannot parse XML CDATA"); _stringBuilder.ClearBuilder(); + // Start position in the string builder + int startPos = 0; + do { if (_xmlSource.Current == ']' && _xmlSource.Peek() == ']') { _xmlSource.MoveNext(); if (_xmlSource.Peek() == '>') { _xmlSource.MoveNext(); _xmlSource.MoveNext(); break; } _stringBuilder.Append(']'); } _stringBuilder.Append(_xmlSource.Current); } while (_xmlSource.MoveNext()); contentValue = _stringBuilder.ToString(); SkipWhiteSpaces(); return contentValue; }Note that for a more significant optimization, you could consider using a buffer for reading larger chunks of data, but that would require more extensive changes to the CharEnumerator class.
356-370: Add null check before accessing attributesWhen reading attribute values, you should check if the
attValueis null before adding it to the attributes list. Although this is unlikely in the current implementation, it would improve robustness.attributes = attributes ?? new List<KeyValuePair<string, string>>(); -attributes.Add(new KeyValuePair<string, string>(attName.ToString(), attValue.ToString())); +attributes.Add(new KeyValuePair<string, string>(attName, attValue ?? string.Empty));
580-592: Consider using case-insensitive comparison for special tokensYour implementation maintains separate entries for uppercase and lowercase versions of the special tokens. A more maintainable approach would be to use a case-insensitive comparison.
-private static readonly Dictionary<string, string> _specialTokens = new Dictionary<string, string>() +private static readonly Dictionary<string, string> _specialTokens = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) { { "amp", "&" }, - { "AMP", "&" }, { "apos", "\'" }, - { "APOS", "\'" }, { "quot", "\"" }, - { "QUOT", "\"" }, { "lt", "<" }, - { "LT", "<" }, { "gt", ">" }, - { "GT", ">" }, };Then update the
TryParseSpecialXmlTokenmethod to use case-insensitive comparison.tests/NLog.UnitTests/Internal/XmlParserTests.cs (1)
309-317: Add memory utilization check to deep nesting testThe test for deeply nested XML checks for stack overflow but doesn't verify memory utilization. Consider adding a memory check to ensure the parser doesn't consume excessive memory.
[Fact] public void XmlParse_DeeplyNestedXml_DoesNotThrowStackOverflowException() { + long memoryBefore = GC.GetTotalMemory(true); string deeplyNestedXml = "<root>" + string.Join("", Enumerable.Repeat("<a>", 10000).ToArray()) + string.Join("", Enumerable.Repeat("</a>", 10000).ToArray()) + "</root>"; var xmlDocument = new XmlParser(deeplyNestedXml).LoadDocument(out var _); + long memoryAfter = GC.GetTotalMemory(false); + long memoryUsed = memoryAfter - memoryBefore; + + // Allow for reasonable memory usage (adjust as needed) + Assert.True(memoryUsed < 100 * 1024 * 1024, $"Memory usage too high: {memoryUsed} bytes"); Assert.NotNull(xmlDocument); Assert.Equal("root", xmlDocument.Name); Assert.Single(xmlDocument.Children); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/NLog/Config/ConfigSectionHandler.cs(1 hunks)src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(3 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(6 hunks)src/NLog/Config/XmlLoggingConfigurationElement.cs(3 hunks)src/NLog/Config/XmlParserConfigurationElement.cs(1 hunks)src/NLog/Config/XmlParserException.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/IFileSystem.cs(2 hunks)src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/Internal/XmlParser.cs(1 hunks)src/NLog/NLogTraceListener.cs(2 hunks)src/NLog/SetupBuilderExtensions.cs(2 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.AutoReloadConfig.Tests/NLog.AutoReloadConfig.Tests.csproj(1 hunks)tests/NLog.Database.Tests/NLog.Database.Tests.csproj(1 hunks)tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj(1 hunks)tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj(1 hunks)tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj(1 hunks)tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj(1 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(5 hunks)tests/NLog.UnitTests/Config/ReloadTests.cs(1 hunks)tests/NLog.UnitTests/Config/XmlConfigTests.cs(1 hunks)tests/NLog.UnitTests/ConfigFileLocatorTests.cs(8 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs(3 hunks)tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs(3 hunks)tests/NLog.UnitTests/NLog.UnitTests.csproj(1 hunks)tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- tests/NLog.UnitTests/NLog.UnitTests.csproj
- src/NLog/Internal/AssemblyHelpers.cs
- src/NLog/SetupBuilderExtensions.cs
- tests/NLog.UnitTests/Config/XmlConfigTests.cs
- tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj
- tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj
- src/NLog/Targets/ConsoleTargetHelper.cs
- tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj
- tests/NLog.AutoReloadConfig.Tests/NLog.AutoReloadConfig.Tests.csproj
- tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj
- tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj
- tests/NLog.UnitTests/Config/ReloadTests.cs
- src/NLog/Config/ConfigSectionHandler.cs
- tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs
- src/NLog/Config/XmlLoggingConfigurationElement.cs
- tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs
- tests/NLog.UnitTests/Config/LogFactorySetupTests.cs
- tests/NLog.UnitTests/ConfigFileLocatorTests.cs
- src/NLog/Config/XmlParserException.cs
- src/NLog/NLogTraceListener.cs
- tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs
- tests/NLog.Database.Tests/NLog.Database.Tests.csproj
- src/NLog/Config/XmlLoggingConfiguration.cs
- tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj
🔇 Additional comments (27)
src/NLog/Config/LoggingConfigurationElementExtensions.cs (1)
35-35: Import needed for IEnumerable support.Good addition of
System.Collections.Genericto enable usage ofIEnumerable<ILoggingConfigurationElement>and related collection types.src/NLog/Internal/IFileSystem.cs (2)
36-36: Ensure that System.IO is the only addition needed.The newly added import appears correct and sufficient for the introduced
TextReader.
46-48: New method signature aligns with text-based loading.Replacing a specialized XML loading method with a generic
LoadTextFileapproach is a beneficial abstraction that meets the AOT requirements and keeps file reading flexible.src/NLog/Internal/AppEnvironmentWrapper.cs (2)
36-36: Approved import for stream I/O operations.No issues with adding
System.IO, as it is required forStreamReader.
105-107: Add proper resource management for StreamReader.Reiterating a prior request to handle exceptions and resource cleanup by wrapping
StreamReaderin a try-catch or using block.public TextReader LoadTextFile(string path) { - return new StreamReader(path); + try + { + path = FixFilePathWithLongUNC(path); + return new StreamReader(path); + } + catch (Exception ex) + { + if (ex.MustBeRethrownImmediately()) + throw; + InternalLogger.Debug(ex, "LoadTextFile Failed"); + throw; + } }src/NLog/Config/XmlParserConfigurationElement.cs (3)
67-75: Clarify the return logic for single-value children.Once the first single-value child is detected, the method returns all single-value children plus attributes, ignoring subsequent non-single-value children. Ensure this is the intended behavior, or consider returning a combined collection of single-value children and attributes more explicitly.
82-91: Consistent approach in handling child elements.The code returns only non-single-value child elements if it encounters at least one. Confirm that skipping single-value children is fully intended, as it parallels the logic in the
Valuesproperty but might lead to confusion if some children partially qualify.
160-171: Approved focused filtering of special XML attributes.Stripping out namespaced attributes and schema references is beneficial. No further concerns spotted here.
src/NLog/Internal/XmlHelper.cs (7)
45-50: LGTM - Clear constants for surrogate character range definitionsThe constants for high and low surrogate range values are well-defined and follow the Unicode standard correctly.
51-66: LGTM - Well-implemented XML character validationThe implementation correctly validates XML characters according to the XML 1.0 specification, which allows characters > U+001F (except for specific whitespace characters U+0009, U+000A, U+000D) and excludes invalid surrogate characters and the special invalid characters U+FFFE and U+FFFF.
251-257: Simplify float conversion and ensure consistent decimal place handlingThe code has been simplified to use
ToString("R", NumberFormatInfo.InvariantInfo)for float values, which is more precise and locale-independent. The condition for handling NaN and infinity values has been preserved correctly.
259-265: Simplify double conversion and ensure consistent decimal place handlingSimilar to the float conversion, the double conversion has been improved to use the "R" format specifier which provides a round-trip conversion, while maintaining special handling for NaN and infinity values.
267-270: Simplify decimal conversion with direct invariant formattingThe decimal conversion has been simplified to use NumberFormatInfo.InvariantInfo directly, which is the correct approach for culture-invariant string conversion.
272-282: LGTM - Well-implemented DateTime conversion to ISO 8601The new DateTime conversion method properly handles the DateTimeKind property and ensures all dates are converted to UTC before formatting, which is essential for consistent XML serialization.
417-443: Consider using enum values for boolean conversionWhen converting Boolean values to strings in
XmlConvertToString, it would be clearer to use the lowercase "true" and "false" consistently, as you're doing now. This matches JavaScript expectations.src/NLog/Config/LoggingConfigurationFileLoader.cs (2)
134-149: LGTM - Enhanced loading with the new XmlParserThe code has been refactored to use a TextReader instead of an XmlReader, which aligns with the new XmlParser implementation. The exception handling is appropriate, specifically checking for XmlParserException to facilitate the transition to the new parser.
158-170: LGTM - Improved error handling logicThe method signature has been updated to accept a boolean parameter instead of an XmlReader instance, which simplifies the error checking process and makes the intent clearer.
src/NLog/Internal/XmlParser.cs (2)
44-60: LGTM - Well-designed constructor optionsThe XmlParser class offers two constructor options that support either a TextReader or a string source, which provides good flexibility for different usage scenarios.
300-322: LGTM - XML comment handlingThe XML comment handling correctly skips over comments and looks for the proper termination sequence (-->). It's important to handle this correctly in an XML parser.
tests/NLog.UnitTests/Internal/XmlParserTests.cs (8)
41-146: LGTM - Comprehensive test cases for invalid XMLThe tests for invalid XML documents cover a wide range of edge cases and error conditions, which is excellent for ensuring the robustness of the parser.
148-176: LGTM - Good coverage of empty document casesThe tests for empty XML documents with various whitespace and casing variations ensure the parser handles these basic cases correctly.
178-201: LGTM - Thorough attribute testingThe tests for XML attributes with different quotation marks and whitespace variations ensure the parser handles attribute parsing correctly.
203-219: LGTM - Multiple attribute testsThe tests for multiple attributes verify that the parser correctly handles elements with multiple attributes.
221-245: LGTM - Good coverage of attribute token handlingThe tests for attribute tokens cover various escaped characters and Unicode representations, ensuring that the parser correctly handles these special cases.
247-275: LGTM - Comprehensive inner text testsThe tests for inner text cover various whitespace, escaped characters, and CDATA sections, ensuring that the parser correctly handles these special cases.
277-289: LGTM - Child element testsThe tests for child elements verify that the parser correctly identifies and processes child elements within an XML structure.
291-307: LGTM - Multiple child testsThe tests for multiple children verify that the parser correctly handles elements with multiple child elements, including mixed content models.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/NLog/Config/XmlParserConfigurationElement.cs (3)
63-78: Performance optimization opportunity in Values propertyThe Values property performs a linear search through all Children elements to check if any are single-value elements, and then creates a new filtered collection. This could be inefficient for large XML structures as it has to scan the entire collection each time the property is accessed.
Consider caching the result of this check and concatenation, or restructuring how single-value elements are identified to avoid the repeated iteration.
80-93: Similar performance concern in ILoggingConfigurationElement.Children implementationThis implementation also performs a linear search through the Children collection each time the property is accessed, with a potentially expensive LINQ operation to filter and cast the elements.
Consider caching the filtered result or restructuring to maintain a separate collection of non-single-value children.
160-167: Potential null comparison issue in IsSpecialXmlRootAttributeThe null check with the conditional access operator (
?.) could be written more clearly:-if (attributeName?.StartsWith("xmlns", StringComparison.OrdinalIgnoreCase) == true) +if (attributeName != null && attributeName.StartsWith("xmlns", StringComparison.OrdinalIgnoreCase))This would be more consistent with the style used in the rest of the codebase and easier to understand.
src/NLog/Internal/XmlHelper.cs (1)
272-282: Consider making DateTime conversion format more explicitThe
XmlConvertToString(DateTime)method uses a format string to convert DateTime to ISO 8601 format. Consider using a named constant for the format string to improve readability and maintainability:+private const string ISO8601_FORMAT = "yyyy-MM-ddTHH:mm:ss.FFFFFFFK"; internal static string XmlConvertToString(DateTime value) { if (value.Kind == DateTimeKind.Unspecified) value = new DateTime(value.Ticks, DateTimeKind.Utc); else value = value.ToUniversalTime(); - return value.ToString("yyyy-MM-ddTHH:mm:ss.FFFFFFFK"); + return value.ToString(ISO8601_FORMAT); }tests/NLog.UnitTests/Internal/XmlParserTests.cs (2)
309-317: Consider reducing the nesting depth in the stack overflow test.While testing against stack overflows is important, 10,000 levels of nesting might unnecessarily slow down test execution. A smaller number (e.g., 1,000) might be sufficient while keeping the test efficient.
- string deeplyNestedXml = "<root>" + string.Join("", Enumerable.Repeat("<a>", 10000).ToArray()) + string.Join("", Enumerable.Repeat("</a>", 10000).ToArray()) + "</root>"; + string deeplyNestedXml = "<root>" + string.Join("", Enumerable.Repeat("<a>", 1000).ToArray()) + string.Join("", Enumerable.Repeat("</a>", 1000).ToArray()) + "</root>";
34-40: Consider adding tests for XML namespaces.The current test suite doesn't verify the parser's behavior with XML namespaces, which are common in XML configurations. Adding namespace tests would enhance coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/NLog.Targets.Trace/NLogTraceListener.cs(1 hunks)src/NLog/Config/ConfigSectionHandler.cs(1 hunks)src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(3 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(6 hunks)src/NLog/Config/XmlLoggingConfigurationElement.cs(3 hunks)src/NLog/Config/XmlParserConfigurationElement.cs(1 hunks)src/NLog/Config/XmlParserException.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/IFileSystem.cs(2 hunks)src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/Internal/XmlParser.cs(1 hunks)src/NLog/SetupBuilderExtensions.cs(2 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.AutoReloadConfig.Tests/NLog.AutoReloadConfig.Tests.csproj(1 hunks)tests/NLog.Database.Tests/NLog.Database.Tests.csproj(1 hunks)tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj(1 hunks)tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj(1 hunks)tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj(1 hunks)tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj(1 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(5 hunks)tests/NLog.UnitTests/Config/ReloadTests.cs(1 hunks)tests/NLog.UnitTests/Config/XmlConfigTests.cs(1 hunks)tests/NLog.UnitTests/ConfigFileLocatorTests.cs(8 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs(3 hunks)tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs(3 hunks)tests/NLog.UnitTests/NLog.UnitTests.csproj(1 hunks)tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- tests/NLog.UnitTests/NLog.UnitTests.csproj
- src/NLog/Internal/AssemblyHelpers.cs
- src/NLog/Config/ConfigSectionHandler.cs
- tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj
- tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs
- src/NLog/SetupBuilderExtensions.cs
- tests/NLog.UnitTests/Config/XmlConfigTests.cs
- tests/NLog.AutoReloadConfig.Tests/NLog.AutoReloadConfig.Tests.csproj
- tests/NLog.UnitTests/Config/ReloadTests.cs
- tests/NLog.UnitTests/Config/LogFactorySetupTests.cs
- tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj
- tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj
- tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj
- tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs
- tests/NLog.Database.Tests/NLog.Database.Tests.csproj
- tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj
- tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj
- src/NLog/Config/XmlParserException.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- src/NLog/Config/XmlLoggingConfigurationElement.cs
- tests/NLog.UnitTests/ConfigFileLocatorTests.cs
- tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs
- src/NLog/Config/XmlLoggingConfiguration.cs
🔇 Additional comments (17)
src/NLog.Targets.Trace/NLogTraceListener.cs (1)
575-575: Removed null-conditional operators from value.Length checks.The removal of null-conditional operators (
?.) in these lines is safe becausevalueis guaranteed to be non-null due to the null-coalescing operator used on line 562:var value = de.Value?.ToString()?.Trim() ?? string.Empty;.Also applies to: 579-579
src/NLog/Config/LoggingConfigurationElementExtensions.cs (1)
124-139: Add null check for childElement.Name to prevent NullReferenceException.The method properly checks for null
elementNameandelement?.Children, but doesn't check ifchildElement.Nameis null inside the loop, which could cause aNullReferenceException.Apply this diff to make the implementation more robust:
public static IEnumerable<ILoggingConfigurationElement> FilterChildren(this ILoggingConfigurationElement element, string elementName) { if (elementName is null || element?.Children is null) yield break; foreach (var childElement in element.Children) { - if (childElement.Name.Equals(elementName, StringComparison.OrdinalIgnoreCase)) + if (childElement?.Name?.Equals(elementName, StringComparison.OrdinalIgnoreCase) == true) { yield return childElement; } } }src/NLog/Internal/IFileSystem.cs (1)
46-48: Replaced XML-specific file loading with generic text file loading.The change from
LoadXmlFiletoLoadTextFileand fromXmlReadertoTextReaderprovides a more general file loading approach that isn't tied to XML. This aligns with the PR's objective of supporting AOT compilation by replacing directXmlReaderusage.src/NLog/Internal/AppEnvironmentWrapper.cs (1)
105-108: Add proper resource management for StreamReader.The current implementation doesn't handle file access errors and resource cleanup properly. It also doesn't use the
FixFilePathWithLongUNCmethod that was used in the previous version.Apply this diff to improve the implementation:
public TextReader LoadTextFile(string path) { - return new StreamReader(path); + try + { + path = FixFilePathWithLongUNC(path); + return new StreamReader(path); + } + catch (Exception ex) + { + if (ex.MustBeRethrownImmediately()) + throw; + InternalLogger.Debug(ex, "LoadTextFile Failed"); + throw; + } }src/NLog/Internal/XmlHelper.cs (2)
78-81: Parameter order is switched in XmlConvertIsXmlSurrogatePair methodThe parameter names in
XmlConvertIsXmlSurrogatePairdon't match their usage. In Unicode, a surrogate pair consists of a high surrogate (first) followed by a low surrogate (second), but your method expects the parameters in the opposite order.Apply this correction:
-public static bool XmlConvertIsXmlSurrogatePair(char lowChar, char highChar) -{ - return XmlConvertIsHighSurrogate(highChar) && XmlConvertIsLowSurrogate(lowChar); -} +public static bool XmlConvertIsXmlSurrogatePair(char highChar, char lowChar) +{ + return XmlConvertIsHighSurrogate(highChar) && XmlConvertIsLowSurrogate(lowChar); +}Also check all usages of this method to ensure the parameters are passed in the correct order.
94-98: Fix parameter order when calling XmlConvertIsXmlSurrogatePairThe parameters are passed in the wrong order based on the current implementation of
XmlConvertIsXmlSurrogatePair. Either fix the method signature as suggested in the previous comment or fix this call.-if (i + 1 < text.Length && XmlConvertIsXmlSurrogatePair(text[i + 1], ch)) +if (i + 1 < text.Length && XmlConvertIsXmlSurrogatePair(ch, text[i + 1]))src/NLog/Config/LoggingConfigurationFileLoader.cs (2)
134-149: Simplified exception handling in LoadXmlLoggingConfigurationFileThe updated method now uses a
TextReaderinstead of anXmlReader, which aligns with the new XML parsing approach. The exception handling is more straightforward, checking if the exception is anXmlParserExceptionto determine if the XML is invalid.This approach improves the code's maintainability while preserving the original functionality.
158-171: Improved XML configuration error detectionThe updated
ThrowXmlConfigExceptionsmethod now takes a booleaninvalidXmlparameter instead of anXmlReader, allowing it to clearly indicate whether there was a parsing error. This simplifies the code and makes the intent clearer.The method also correctly checks for both
throwExceptionsandthrowConfigExceptionsattributes in the file content before throwing exceptions.src/NLog/Internal/XmlParser.cs (3)
516-535: Add boundary check for unicode valuesThe unicode parsing functionality should check for unreasonably large values that could cause issues when converted to a char.
private int TryParseUnicodeValue() { int unicode = '\0'; while (_xmlSource.MoveNext()) { if (_xmlSource.Current == ';') break; if (_xmlSource.Current < '0' || _xmlSource.Current > '9') throw new XmlParserException("Invalid XML document. Cannot parse unicode-char digit-value"); unicode *= 10; unicode += _xmlSource.Current - '0'; + + // Check for overflow or invalid unicode value + if (unicode > 0x10FFFF) + throw new XmlParserException("Invalid XML document. Unicode value exceeds valid range"); } if (unicode >= '\uffff') throw new XmlParserException("Invalid XML document. Unicode value exceeds maximum allowed value"); return unicode; }
629-706: Consider implementing IDisposable properly in CharEnumeratorThe CharEnumerator class implements IDisposable but has an empty Dispose method. It should properly dispose of the TextReader when it's done.
private sealed class CharEnumerator : IEnumerator<char> { private readonly TextReader _xmlSource; private int _lineNumber; private char _current; private char? _peek; private bool _endOfFile; + private bool _disposed; public CharEnumerator(TextReader xmlSource) { _xmlSource = xmlSource; var current = xmlSource.Read(); _current = current < 0 ? '\0' : (char)current; _lineNumber = current == '\n' ? 2 : 1; } public char Current { get { + if (_disposed) + throw new ObjectDisposedException(nameof(CharEnumerator)); if (_endOfFile) throw new XmlParserException($"Invalid XML document. Unexpected end of document."); return _current; } } public int LineNumber => _lineNumber; object IEnumerator.Current => Current; public bool EndOfFile => _endOfFile; public bool MoveNext() { + if (_disposed) + throw new ObjectDisposedException(nameof(CharEnumerator)); if (_peek.HasValue) { _current = _peek.Value; if (_current == '\n') ++_lineNumber; _peek = null; return true; } var current = _xmlSource.Read(); if (current < 0) { _endOfFile = true; return false; } _current = (char)current; if (_current == '\n') ++_lineNumber; return true; } public char Peek() { + if (_disposed) + throw new ObjectDisposedException(nameof(CharEnumerator)); if (_peek.HasValue) return _peek.Value; var current = _xmlSource.Read(); if (current < 0) return '\0'; _peek = (char)current; return _peek.Value; } void IEnumerator.Reset() { // NOSONAR: Nothing to reset } void IDisposable.Dispose() { - // NOSONAR: Nothing to dispose + if (!_disposed) + { + _disposed = true; + _xmlSource.Dispose(); + } } }
44-60: Well-structured XML parser implementation for AOT supportThis implementation provides a minimalistic XML parser that doesn't rely on System.Xml, as mentioned in the class documentation. The design is clean and includes proper error handling with detailed error messages.
The parser handles all essential XML features including elements, attributes, processing instructions, CDATA sections, and comments, while maintaining line number tracking for better error reporting.
tests/NLog.UnitTests/Internal/XmlParserTests.cs (6)
41-146: Comprehensive test coverage for invalid XML documents.This test suite effectively covers a wide range of invalid XML scenarios, ensuring the parser correctly rejects malformed inputs. Good use of [Theory] and [InlineData] to test numerous edge cases efficiently.
148-176: Good coverage of empty document scenarios.The test properly validates that valid but structurally empty XML documents are parsed correctly, with appropriate assertions on the document properties.
178-219: Thorough attribute parsing validation.Both single and multiple attribute tests are well-structured, checking proper name and value extraction under various formatting conditions.
221-245: Excellent handling of special characters in attributes.The test verifies proper handling of path characters, special XML entities, and numeric character references in attribute values.
247-275: Comprehensive inner text token handling.Good coverage of whitespace handling, entity references, and CDATA sections in element content.
277-307: Well-structured child element tests.Tests properly verify both single and multiple child element scenarios with different XML formatting styles.
|
With this custom XML parser, then it shaves off 4 MByte, and removes dependencies on System.Net. Because XmlReader has the ability to load XML document using web-url. While NLog v5 has footprint of 14 Mbytes. Then NLog v6 has footprint of 7 Mbytes. This still larger than default console-app of 2 MByte. This an annoying choice. Should NLog-core package support loading dynamically from NLog.config-file and appsettings.json file out-of-the-box, or should the dynamic-configuration require a bootstrap-call (similar to when loading appsettings.json-file). With explicit bootstrap, then all config-parsers / layout-parsers etc. could be extracted into their own NLog.Configuration-nuget-package. I guess the bootstrap must be implicit until having a fluent-API for building SimpleLayout from layout-renderers. Also all NLog-tutorials expects NLog-core to work without bootstrap and be operational from the start. Maybe NLog v7 will chop off some more fat. |
|
Maybe it should be a hybrid, so LayoutRenderer-parsing for SimpleLayout always works, but full-configuration-file-parsing with targets / filters / etc. is something that must be enabled explicitly. Thus one will only pay for the targets / configuration-options being used. Would prefer that NLog with AOT-support would have a minimal build and startup-overhead, when not depending on dynamic loading from This also includes the initialization of the config-item-repository This means But if going this way then one must change NLog into needing bootstrap-logic at startup to "automatically" load ConfigurationIemFactory must be able to resolve dependencies without adding compile-time-dependency on all config-items. The config-item-type-collections should be lazy evaluated, and should avoid the all-factory-collection. Make sure that SimpleLayout-constructor works without adding dependency on all config-items in ConfigurationIemFactory. New secret sauce is needed for the reflection-logic to recognize thread-context-capture-needs. Could be neat if all relevant NLog Layout + LayoutRenderers could implicitly say that their public-properties should not be stripped. |
|
Tested basic hybrid version where AOT-build only included NLog Layouts + NLog Targets used by Fluent API. And simple console-application changed from 7 MByte to 4 Mbyte. I guess further optimizations can be made, but it seems disabling automatic loading of |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/NLog/Internal/AppEnvironmentWrapper.cs (1)
105-108: 🛠️ Refactor suggestionAdd proper resource management for StreamReader.
The current implementation doesn't handle file access errors and resource cleanup properly. It also doesn't use the
FixFilePathWithLongUNCmethod that was used in the previousLoadXmlFileimplementation, which could cause issues with long paths.public TextReader LoadTextFile(string path) { - return new StreamReader(path); + try + { + path = FixFilePathWithLongUNC(path); + return new StreamReader(path); + } + catch (Exception ex) + { + if (ex.MustBeRethrownImmediately()) + throw; + InternalLogger.Debug(ex, "LoadTextFile Failed"); + throw; + } }src/NLog/Internal/XmlHelper.cs (1)
68-81:⚠️ Potential issueParameter order is switched in XmlConvertIsXmlSurrogatePair method.
The parameter names in
XmlConvertIsXmlSurrogatePairdon't match their usage. In Unicode, a surrogate pair consists of a high surrogate (first) followed by a low surrogate (second), but your method expects the parameters in the opposite order.Apply this correction:
-public static bool XmlConvertIsXmlSurrogatePair(char lowChar, char highChar) -{ - return XmlConvertIsHighSurrogate(highChar) && XmlConvertIsLowSurrogate(lowChar); -} +public static bool XmlConvertIsXmlSurrogatePair(char highChar, char lowChar) +{ + return XmlConvertIsHighSurrogate(highChar) && XmlConvertIsLowSurrogate(lowChar); +}Also check all usages of this method to ensure the parameters are passed in the correct order.
🧹 Nitpick comments (2)
src/NLog/Config/LoggingConfigurationFileLoader.cs (1)
158-158: Note on reading entire config file.The
ThrowXmlConfigExceptionsmethod callsFile.ReadAllText(configFile), which can be costly for very large files. Though this is acceptable for typical NLog configuration sizes, consider whether buffering or partial reads might be beneficial if configuration files grow large.src/NLog/Config/XmlLoggingConfiguration.cs (1)
292-323: Obsolete .NET Framework-only logic could be refactored.
Currently,ParseFromXmlReader()duplicates some logic fromParseFromTextReader(). You could unify these paths to reduce maintenance complexity, while still preserving backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/NLog.Targets.Trace/NLogTraceListener.cs(1 hunks)src/NLog/Config/ConfigSectionHandler.cs(1 hunks)src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(3 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(6 hunks)src/NLog/Config/XmlLoggingConfigurationElement.cs(3 hunks)src/NLog/Config/XmlParserConfigurationElement.cs(1 hunks)src/NLog/Config/XmlParserException.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/IFileSystem.cs(2 hunks)src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/Internal/XmlParser.cs(1 hunks)src/NLog/SetupBuilderExtensions.cs(2 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.AutoReloadConfig.Tests/NLog.AutoReloadConfig.Tests.csproj(1 hunks)tests/NLog.Database.Tests/NLog.Database.Tests.csproj(1 hunks)tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj(1 hunks)tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj(1 hunks)tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj(1 hunks)tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj(1 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(5 hunks)tests/NLog.UnitTests/Config/ReloadTests.cs(1 hunks)tests/NLog.UnitTests/Config/XmlConfigTests.cs(1 hunks)tests/NLog.UnitTests/ConfigFileLocatorTests.cs(8 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs(3 hunks)tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs(3 hunks)tests/NLog.UnitTests/NLog.UnitTests.csproj(1 hunks)tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- tests/NLog.UnitTests/NLog.UnitTests.csproj
- src/NLog/Internal/AssemblyHelpers.cs
- tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- src/NLog/Config/ConfigSectionHandler.cs
- tests/NLog.UnitTests/Config/XmlConfigTests.cs
- src/NLog/SetupBuilderExtensions.cs
- tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj
- tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj
- tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs
- tests/NLog.UnitTests/Config/ReloadTests.cs
- src/NLog.Targets.Trace/NLogTraceListener.cs
- src/NLog/Config/XmlParserConfigurationElement.cs
- tests/NLog.UnitTests/Config/LogFactorySetupTests.cs
- tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj
- src/NLog/Config/XmlLoggingConfigurationElement.cs
- tests/NLog.AutoReloadConfig.Tests/NLog.AutoReloadConfig.Tests.csproj
- tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj
- src/NLog/Config/XmlParserException.cs
- tests/NLog.UnitTests/ConfigFileLocatorTests.cs
- tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs
- tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj
- tests/NLog.Database.Tests/NLog.Database.Tests.csproj
- tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj
🔇 Additional comments (45)
src/NLog/Config/LoggingConfigurationElementExtensions.cs (1)
124-139: Well-implemented FilterChildren extension method.Good implementation with proper null checks and consistent style with other methods in this class. The method enhances the XML configuration system by providing a convenient way to filter child elements by name.
src/NLog/Internal/IFileSystem.cs (1)
46-48: Interface change aligns with AOT support objectives.The change from
LoadXmlFiletoLoadTextFilereturning aTextReaderis appropriate for the goal of replacing XmlReader with a custom parser to improve AOT compilation support.src/NLog/Internal/XmlHelper.cs (6)
45-66: Good implementation of XML character validation.The new character validation constants and method provide a more efficient approach than regex-based validation for XML character checking. Using character ranges is more performant and supports AOT compilation better.
94-105:⚠️ Potential issueFix parameter order when calling XmlConvertIsXmlSurrogatePair.
The parameters are passed in the wrong order based on the current implementation of
XmlConvertIsXmlSurrogatePair. Either fix the method signature as suggested in the previous comment or fix this call.-if (i + 1 < text.Length && XmlConvertIsXmlSurrogatePair(text[i + 1], ch)) +if (i + 1 < text.Length && XmlConvertIsXmlSurrogatePair(ch, text[i + 1]))
251-257: Improved float conversion handling.The refactored code properly handles special float values and ensures decimal place representation for regular values. This implementation is more direct and better aligned with AOT requirements.
259-265: Improved double conversion handling.Similar to the float conversion, this implementation now handles special values directly without relying on XmlConvert, which is more efficient and better for AOT compilation.
267-270: Simplified decimal conversion handling.The removal of the XmlConvert dependency for decimal conversion simplifies the code and improves AOT compatibility.
272-282: Good implementation of DateTime conversion.The new DateTime conversion method properly handles time zone conversion to UTC and formats the date in ISO 8601 format, which is standard for XML. This is a valuable addition for consistent date handling.
src/NLog/Config/LoggingConfigurationFileLoader.cs (1)
134-149: Good use ofusingblock and exception handling.Wrapping
LoadTextFile(configFile)in ausingstatement ensures that theTextReaderis correctly disposed. Throwing an empty default config upon XML parsing failure is a practical fallback. Keep an eye on scenarios where partially loaded configurations might inadvertently override previously loaded settings if this fallback occurs unexpectedly.src/NLog/Internal/XmlParser.cs (18)
52-60: Validate constructor parameters.Neither constructor checks if the provided
xmlSourceis null. To prevent potentialNullReferenceException, consider adding parameter validation, such as:Guard.ThrowIfNull(xmlSource, nameof(xmlSource));
62-127: Stack-based parsing logic looks sound.The
LoadDocumentmethod uses a stack to manage nested elements and breaks correctly once the top element has been closed. This approach effectively enforces a single root element. The error handling is thorough, and exceptions are consistently wrapped inXmlParserException.
129-160: Processing instructions handling appears robust.Using a separate out-parameter
processingInstructionsis a neat approach. The method handles malformed instructions by throwingXmlParserException, which is consistent with the rest of the parser’s error handling strategy.
162-188: Start element parsing is consistent.The parser checks for whitespace and angle brackets, and calls
TryReadAttributesto handle attributes. The overall flow is logical, and any failure to parse triggers an exception with clear contextual information.
190-226: Good handling of end elements and self-closing tags.Properly recognizes
/<tag>and self-closing syntax/>. AnXmlParserExceptionis thrown for mismatched tags, ensuring strict well-formedness. This is a reasonable approach for configuration files.
228-267: Inner text parsing highlights robust XML coverage.Reading text, handling comments, and recognizing CDATA sections are well integrated. The logic gracefully appends text from multiple sections.
269-298: CDATA parsing is straightforward.Reads until the terminating
]]>is encountered, storing unmodified content. Note that nested CDATA blocks are typically not valid XML, so this is acceptable.
300-322: Comment skipping is minimal yet effective.The loop looks for
-->as the closing sequence, with sanity checks along the way. Logging or debugging details for extremely large comments might be beneficial, but this is sufficient for typical config usage.
324-373: Attribute parsing logic is comprehensive.Properly handles both double and single quotes, including whitespace nuances. Throws an exception on invalid constructs. This matches a minimal XML approach while still being strict about attribute formatting.
375-417: Initial start element detection is consistent.Prevents misinterpretation of
</,<!, or<?. Disallows invalid chars in element names. Behavior is in line with typical XML parsing rules for tag names.
419-438:SkipCharandSkipWhiteSpacesprovide clarity.These utility methods keep the parsing code tidy, ensuring that many whitespace scenarios are handled consistently.
440-515: Potential expansions to valid name characters.Although
ReadUntilCharaccepts letters, digits, underscores, etc., strict XML 1.0 sections allow additional name characters (like combining marks or extension characters). For the typical NLog config, this is likely enough.Would you like to confirm that the simplified set of valid name characters covers all intended use cases?
516-535: Add boundary check for greater Unicode values.This code does not check for code points above U+FFFF (e.g., 0x10FFFF). Consider adding:
if (unicode > 0x10FFFF) throw new XmlParserException("Invalid XML document. Unicode value exceeds valid range");
537-558: Add boundary check for hex-based Unicode values.Similar to decimal parsing, values above U+FFFF are technically valid supplementary characters. You may want:
if (unicode > 0x10FFFF) throw new XmlParserException("Invalid XML document. Unicode value exceeds valid range");
560-578: Special-token parsing is straightforward.Recognizing valid entity tokens (
&,<, etc.) from_specialTokensis a concise approach. ThrowsXmlParserExceptionfor unrecognized tokens, preserving strictness.
580-606: Predefined entity dictionary andCharIsSpaceare correct.The
_specialTokensdictionary captures the most common XML entities.CharIsSpacechecks typical whitespace pluschar.IsWhiteSpace, which is suitable for config files.
608-627: Simple container approach inXmlParserElement.Storing attributes, children, and text keeps this lightweight object flexible. The lazy initialization for
Childrenalso helps reduce allocations for self-closing or empty elements.
629-706: Dispose method is empty; consider fully disposing theTextReader.As previously noted, implementing proper disposal for
CharEnumeratoravoids a resource leak. If theTextReaderis no longer needed after parsing, disposing it explicitly can improve resource usage.tests/NLog.UnitTests/Internal/XmlParserTests.cs (6)
43-146: Extensive invalid-document test coverage.These
[Theory]tests with various invalid XML inputs do an excellent job of verifying the parser’s robustness and strictness. They ensureXmlParserExceptionis triggered for malformed structures.
148-176: Empty-document tests confirm minimal valid root structure.Verifies that
<nlog/>or<nlog></nlog>produce a root element with no children or attributes. This helps guarantee correct handling of trivial XML configurations.
178-201: Multiple attribute and attribute token tests are thorough.Tests that attributes read properly in single or multi-attribute scenarios, including advanced token expansions for spaces and special characters. Great coverage for real-world config usage.
Also applies to: 203-219
221-245: Inner-text handling with CDATA and special tokens is validated.Ensures that the parser returns the correct unescaped strings, capturing multiline text, CDATA content, and standard XML entities.
Also applies to: 247-275
277-289: Child elements validated in single and multiple scenarios.The parser’s approach to nesting is tested with
<variable>nodes, confirming correct detection of child elements and their attributes or sub-elements.Also applies to: 291-307
309-317: Deeply nested XML test verifies stack-based parsing reliability.Checks for possible stack overflows by nesting 10,000 elements. This strongly confirms that the parser’s recursion avoidance with an iterative stack is robust.
src/NLog/Config/XmlLoggingConfiguration.cs (12)
47-47: No concerns with updated summary.
The updated summary reflects the file’s purpose accurately.
81-81: Good use of guard clause.
EnsuringfileNameis neither null nor empty helps maintain robust error handling.
85-93: Constructor withTextReaderparameter looks solid.
This setup increases flexibility in loading configurations from in-memory strings or other sources.
94-103: Overload with optionalfileNamealigns with the new design.
It gracefully accommodates scenarios where the file name might be unknown or irrelevant.
104-115: Additional constructor withlogFactoryparameter is well-structured.
UsingGuard.ThrowIfNullforxmlSourceconfirms robust defensive programming.
122-123: Marking theXmlReader-based constructor as obsolete is appropriate.
It clearly communicates the transition away fromXmlReader.
129-132: Consistent deprecation of old constructor.
This matches the overall shift towardTextReaderusage.
139-143: Obsolete constructor retains backward compatibility.
This is a reasonable approach for existing .NET Framework consumers.
276-281: Refined file-based loading usesTextReadernicely.
IncorporatingGuard.ThrowIfNullOrEmptyensures an invalidfileNameis caught early.
288-288: Directly parsing from string reader simplifies the flow.
This change removes the extra indirection ofXmlReader.
325-326: New parsing method usage looks coherent.
The introduction ofXmlParserin this method completes the transition away fromXmlReader.Also applies to: 330-330
358-361: Consistent invocation ofXmlParserinConfigureFromFile.
This maintains the pattern established elsewhere in the configuration parser.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/NLog/Internal/XmlParser.cs (2)
573-585: Consider using a case-insensitive collection for special tokensThe special tokens dictionary contains separate entries for uppercase and lowercase tokens. Consider using a case-insensitive dictionary or a more efficient lookup method.
-private static readonly Dictionary<string, string> _specialTokens = new Dictionary<string, string>() +private static readonly Dictionary<string, string> _specialTokens = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) { { "amp", "&" }, - { "AMP", "&" }, { "apos", "\'" }, - { "APOS", "\'" }, { "quot", "\"" }, - { "QUOT", "\"" }, { "lt", "<" }, - { "LT", "<" }, { "gt", ">" }, - { "GT", ">" }, };
553-571: Optimize special token lookupThe special token lookup performance can be improved by checking the first character before iterating through all tokens.
private bool TryParseSpecialXmlToken(out string xmlToken) { + char firstChar = _xmlSource.Current; foreach (var token in _specialTokens) { - if (_xmlSource.Current == token.Key[0] && _xmlSource.Peek() == token.Key[1]) + if (firstChar == token.Key[0] && _xmlSource.Peek() == token.Key[1]) { foreach (var tokenChr in token.Key) if (!SkipChar(tokenChr)) throw new XmlParserException($"Invalid XML document. Cannot parse special token: {token.Key}"); if (_xmlSource.Current != ';') throw new XmlParserException($"Invalid XML document. Cannot parse special token: {token.Key}"); xmlToken = token.Value; return true; } } xmlToken = null; return false; }tests/NLog.UnitTests/Internal/XmlParserTests.cs (3)
312-313: Consider a more memory-efficient approach for deeply nested XML testCreating a very large string with 10,000 nested elements could cause memory issues. Consider using a StringBuilder or a more efficient approach.
- string deeplyNestedXml = "<root>" + string.Join("", Enumerable.Repeat("<a>", 10000).ToArray()) + string.Join("", Enumerable.Repeat("</a>", 10000).ToArray()) + "</root>"; - var xmlDocument = new XmlParser(deeplyNestedXml).LoadDocument(out var _); + var sb = new StringBuilder("<root>"); + for (int i = 0; i < 10000; i++) + sb.Append("<a>"); + for (int i = 0; i < 10000; i++) + sb.Append("</a>"); + sb.Append("</root>"); + var xmlDocument = new XmlParser(sb.ToString()).LoadDocument(out var _);
312-317: Add verification of nested structure in deeply nested XML testThe test verifies the document doesn't throw a StackOverflowException but doesn't check if the nested structure is correctly parsed. Add assertions to verify the complete structure.
[Fact] public void XmlParse_DeeplyNestedXml_DoesNotThrowStackOverflowException() { string deeplyNestedXml = "<root>" + string.Join("", Enumerable.Repeat("<a>", 10000).ToArray()) + string.Join("", Enumerable.Repeat("</a>", 10000).ToArray()) + "</root>"; var xmlDocument = new XmlParser(deeplyNestedXml).LoadDocument(out var _); Assert.NotNull(xmlDocument); Assert.Equal("root", xmlDocument.Name); Assert.Single(xmlDocument.Children); + + // Verify that all nested elements are parsed correctly + var currentElement = xmlDocument.Children[0]; + int depth = 1; + + while (currentElement.Children != null && currentElement.Children.Count > 0) + { + Assert.Equal("a", currentElement.Name); + currentElement = currentElement.Children[0]; + depth++; + } + + Assert.Equal("a", currentElement.Name); + Assert.Equal(10000, depth); }
138-140: Add test cases for edge case unicode valuesThe test includes invalid unicode values but could benefit from testing edge cases like maximum valid values.
Consider adding test cases for:
- Maximum valid unicode values
- Values just below the threshold
- Valid surrogate pairs
For example:
[InlineData("<nlog></nlog>")] // Maximum value [InlineData("<nlog></nlog>")] // Maximum hex value
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Internal/XmlParser.cs(1 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)
🔇 Additional comments (3)
src/NLog/Internal/XmlParser.cs (3)
509-528: Add boundary check for unicode valuesThe unicode parsing functionality should check for unreasonably large values that could cause issues when converted to a char.
private int TryParseUnicodeValue() { int unicode = '\0'; while (_xmlSource.MoveNext()) { if (_xmlSource.Current == ';') break; if (_xmlSource.Current < '0' || _xmlSource.Current > '9') throw new XmlParserException("Invalid XML document. Cannot parse unicode-char digit-value"); unicode *= 10; unicode += _xmlSource.Current - '0'; + + // Check for overflow or invalid unicode value + if (unicode > 0x10FFFF) + throw new XmlParserException("Invalid XML document. Unicode value exceeds valid range"); } if (unicode >= '\uffff') throw new XmlParserException("Invalid XML document. Unicode value exceeds maximum allowed value"); return unicode; }
530-551: Add boundary check for hex unicode valuesThe hex unicode parser should also check for values exceeding the valid Unicode range, similar to the decimal unicode parser.
private int TryParseUnicodeValueHex() { int unicode = '\0'; while (_xmlSource.MoveNext()) { if (_xmlSource.Current == ';') break; unicode *= 16; if ("abcdef".Contains(char.ToLower(_xmlSource.Current))) unicode += int.Parse(_xmlSource.Current.ToString(), System.Globalization.NumberStyles.HexNumber); else if (_xmlSource.Current < '0' || _xmlSource.Current > '9') throw new XmlParserException("Invalid XML document. Cannot parse unicode-char hex-value"); else unicode += _xmlSource.Current - '0'; + + // Check for overflow or invalid unicode value + if (unicode > 0x10FFFF) + throw new XmlParserException("Invalid XML document. Unicode value exceeds valid range"); } if (unicode >= '\uffff') throw new XmlParserException("Invalid XML document. Unicode value exceeds maximum allowed value"); return unicode; }
622-699: Implement IDisposable properly in CharEnumeratorThe CharEnumerator class implements IDisposable but has an empty Dispose method. It should properly dispose of the TextReader when it's done.
private sealed class CharEnumerator : IEnumerator<char> { private readonly TextReader _xmlSource; private int _lineNumber; private char _current; private char? _peek; private bool _endOfFile; + private bool _disposed; public CharEnumerator(TextReader xmlSource) { _xmlSource = xmlSource; var current = xmlSource.Read(); _current = current < 0 ? '\0' : (char)current; _lineNumber = current == '\n' ? 2 : 1; } public char Current { get { + if (_disposed) + throw new ObjectDisposedException(nameof(CharEnumerator)); if (_endOfFile) throw new XmlParserException($"Invalid XML document. Unexpected end of document."); return _current; } } public int LineNumber => _lineNumber; object IEnumerator.Current => Current; public bool EndOfFile => _endOfFile; public bool MoveNext() { + if (_disposed) + throw new ObjectDisposedException(nameof(CharEnumerator)); if (_peek.HasValue) { _current = _peek.Value; if (_current == '\n') ++_lineNumber; _peek = null; return true; } var current = _xmlSource.Read(); if (current < 0) { _endOfFile = true; return false; } _current = (char)current; if (_current == '\n') ++_lineNumber; return true; } public char Peek() { + if (_disposed) + throw new ObjectDisposedException(nameof(CharEnumerator)); if (_peek.HasValue) return _peek.Value; var current = _xmlSource.Read(); if (current < 0) return '\0'; _peek = (char)current; return _peek.Value; } void IEnumerator.Reset() { // NOSONAR: Nothing to reset } void IDisposable.Dispose() { - // NOSONAR: Nothing to dispose + if (!_disposed) + { + _disposed = true; + _xmlSource.Dispose(); + } } }
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/NLog/Internal/XmlParser.cs (3)
509-528:⚠️ Potential issueUnicode value upper-bound still too low (repeat from last review)
Previous feedback suggested rejecting values >
0x10FFFF; current code only checks>= '\uFFFF'.
This allows surrogate / invalid planes and can yieldcharconversion errors on some runtimes.
530-551:⚠️ Potential issueSame overflow check missing for hexadecimal branch
Hex parser has the identical gap noted above.
638-699:⚠️ Potential issue
CharEnumeratorstill neglects disposal (repeat from last review)
Disposeis empty and no_disposedguard is present. The underlyingTextReaderremains open and object-use-after-dispose cannot be detected.+private bool _disposed; ... void IDisposable.Dispose() { - // NOSONAR: Nothing to dispose + if (!_disposed) + { + _disposed = true; + _xmlSource.Dispose(); + } }Also add
_disposedchecks at the start ofCurrent,MoveNext, andPeek.
🧹 Nitpick comments (3)
src/NLog/Internal/XmlParser.cs (1)
351-357: Minor allocation / boxing nitpick in attribute collection
attNameandattValueare already strings, the extra.ToString()is redundant.-attributes.Add(new KeyValuePair<string, string>(attName.ToString(), attValue.ToString())); +attributes.Add(new KeyValuePair<string, string>(attName, attValue));src/NLog/Config/XmlLoggingConfiguration.cs (2)
371-377: Processing instructions silently discarded
LoadDocument(out var _)ignores the list of processing instructions.
If future versions embed meaningful PIs (e.g.,<?nlog include='…'?>) they will be lost.Consider forwarding the collection (even if presently unused) or explicitly validating that only the standard
<?xml …?>PI is present to avoid silent mis-configuration later.
419-432: Avoid per-element string allocations in case-insensitive comparison
content.Name.ToUpperInvariant()allocates a new string for every element. For large configs this is measurable.
UseEquals(..., StringComparison.OrdinalIgnoreCase)instead:-switch (content.Name.ToUpperInvariant()) +switch (content.Name) { - case "CONFIGURATION": + case var _ when content.Name.Equals("configuration", StringComparison.OrdinalIgnoreCase): ... - case "NLOG": + case var _ when content.Name.Equals("nlog", StringComparison.OrdinalIgnoreCase):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
src/NLog.Targets.Trace/NLogTraceListener.cs(1 hunks)src/NLog/Config/ConfigSectionHandler.cs(1 hunks)src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(3 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(6 hunks)src/NLog/Config/XmlLoggingConfigurationElement.cs(3 hunks)src/NLog/Config/XmlParserConfigurationElement.cs(1 hunks)src/NLog/Config/XmlParserException.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/IFileSystem.cs(2 hunks)src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/Internal/XmlParser.cs(1 hunks)src/NLog/SetupBuilderExtensions.cs(2 hunks)tests/NLog.Database.Tests/NLog.Database.Tests.csproj(1 hunks)tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj(1 hunks)tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj(1 hunks)tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj(1 hunks)tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj(1 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(5 hunks)tests/NLog.UnitTests/Config/ReloadTests.cs(1 hunks)tests/NLog.UnitTests/Config/XmlConfigTests.cs(1 hunks)tests/NLog.UnitTests/ConfigFileLocatorTests.cs(8 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs(3 hunks)tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs(3 hunks)tests/NLog.UnitTests/NLog.UnitTests.csproj(1 hunks)tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/NLog/Config/XmlParserConfigurationElement.cs
🚧 Files skipped from review as they are similar to previous changes (27)
- src/NLog/Internal/AssemblyHelpers.cs
- src/NLog/Config/ConfigSectionHandler.cs
- tests/NLog.UnitTests/NLog.UnitTests.csproj
- tests/NLog.UnitTests/Config/ReloadTests.cs
- tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj
- tests/NLog.UnitTests/Config/XmlConfigTests.cs
- tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj
- src/NLog/Config/LoggingConfigurationElementExtensions.cs
- tests/NLog.UnitTests/Config/LogFactorySetupTests.cs
- tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs
- src/NLog/Config/XmlLoggingConfigurationElement.cs
- tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj
- tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj
- tests/NLog.Database.Tests/NLog.Database.Tests.csproj
- tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs
- tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj
- src/NLog/SetupBuilderExtensions.cs
- tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj
- src/NLog/Internal/IFileSystem.cs
- src/NLog.Targets.Trace/NLogTraceListener.cs
- src/NLog/Internal/AppEnvironmentWrapper.cs
- tests/NLog.UnitTests/ConfigFileLocatorTests.cs
- src/NLog/Config/XmlParserException.cs
- src/NLog/Config/LoggingConfigurationFileLoader.cs
- tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs
- tests/NLog.UnitTests/Internal/XmlParserTests.cs
- src/NLog/Internal/XmlHelper.cs
6968fcc to
184109d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/NLog.UnitTests/Internal/XmlParserTests.cs (3)
173-173: Prefer culture-safe, case-insensitive comparison overToLower()
ToLower()allocates a new string and depends on the current culture.
Xunit already provides an overload that performs a case-insensitive check without an extra allocation:-Assert.Equal("nlog", xmlDocument.Name.ToLower()); +Assert.Equal("nlog", xmlDocument.Name, ignoreCase: true);
174-176: Returningnullfor empty collections complicates consumersAll the “empty”-document tests assert that
ChildrenandAttributesarenull. From an API-design perspective, returning an empty (read-only) collection is usually safer than returningnull, because it removes the need for NRT/defensivenullchecks when iterating:foreach (var child in xmlElement.Children) { … } // no null-guard neededConsider adapting the parser to return
Array.Empty<XmlElement>()(orReadOnlyCollection<T>) for empty results and update the assertions accordingly.
This small change improves usability and aligns with .NET collection guidelines.
312-313: Avoid large intermediate string allocations when building deep XML
string.Join("", Enumerable.Repeat("<a>", 10000).ToArray()):
- Allocates the repeated strings.
- Allocates an array of 10 000 elements.
- Performs two joins, creating multiple temporary strings.
A more memory-friendly alternative is to use
StringBuilderonce:-string deeplyNestedXml = "<root>" + string.Join("", Enumerable.Repeat("<a>", 10000).ToArray()) + - string.Join("", Enumerable.Repeat("</a>", 10000).ToArray()) + "</root>"; +var sb = new StringBuilder(capacity: 80000); +sb.Append("<root>"); +for (int i = 0; i < 10000; i++) sb.Append("<a>"); +for (int i = 0; i < 10000; i++) sb.Append("</a>"); +sb.Append("</root>"); +string deeplyNestedXml = sb.ToString();This keeps the intent while reducing GC pressure and test execution time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Internal/XmlParser.cs(1 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NLog/Internal/XmlParser.cs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/NLog/Config/XmlParserConfigurationElement.cs (3)
63-78: Consider optimizing the Values property for better performance.The current implementation iterates through Children twice - once to find any SingleValueElement and again when creating the returned collection. This could be inefficient for elements with many children.
public IEnumerable<KeyValuePair<string, string>> Values { get { - for (int i = 0; i < Children.Count; ++i) - { - var child = Children[i]; - if (SingleValueElement(child)) - { - // Values assigned using nested node-elements. Maybe in combination with attributes - return AttributeValues.Concat(Children.Where(item => SingleValueElement(item)).Select(item => new KeyValuePair<string, string>(item.Name, item.Value))); - } - } - return AttributeValues; + bool hasSingleValueChildren = Children.Any(SingleValueElement); + if (hasSingleValueChildren) + { + // Values assigned using nested node-elements. Maybe in combination with attributes + return AttributeValues.Concat(Children.Where(item => SingleValueElement(item)).Select(item => new KeyValuePair<string, string>(item.Name, item.Value))); + } + return AttributeValues; } }
80-93: Similar optimization opportunity in ILoggingConfigurationElement.Children implementation.This method has a similar double iteration issue as the Values property.
IEnumerable<ILoggingConfigurationElement> ILoggingConfigurationElement.Children { get { - for (int i = 0; i < Children.Count; ++i) - { - var child = Children[i]; - if (!SingleValueElement(child)) - return Children.Where(item => !SingleValueElement(item)).Cast<ILoggingConfigurationElement>(); - } - - return ArrayHelper.Empty<ILoggingConfigurationElement>(); + var nonSingleValueChildren = Children.Where(item => !SingleValueElement(item)).Cast<ILoggingConfigurationElement>().ToArray(); + return nonSingleValueChildren.Length > 0 ? nonSingleValueChildren : ArrayHelper.Empty<ILoggingConfigurationElement>(); } }
136-139: Consider using a more efficient attribute prefix handling approach.The current implementation creates a new KeyValuePair for each attribute with a prefix. For attributes with namespace prefixes, consider a more efficient approach.
for (int j = 0; j < attributes.Count; ++j) { var attributePrefixIndex = attributes[j].Key.IndexOf(':'); if (attributePrefixIndex >= 0) - attributes[j] = new KeyValuePair<string, string>(attributes[j].Key.Substring(attributePrefixIndex + 1), attributes[j].Value); + { + string key = attributes[j].Key; + attributes[j] = new KeyValuePair<string, string>(key.Substring(attributePrefixIndex + 1), attributes[j].Value); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/NLog.Targets.Network/Targets/NetworkTarget.cs(1 hunks)src/NLog.Targets.Trace/NLogTraceListener.cs(1 hunks)src/NLog/Config/ConfigSectionHandler.cs(1 hunks)src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(3 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(6 hunks)src/NLog/Config/XmlLoggingConfigurationElement.cs(3 hunks)src/NLog/Config/XmlParserConfigurationElement.cs(1 hunks)src/NLog/Config/XmlParserException.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/IFileSystem.cs(2 hunks)src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/Internal/XmlParser.cs(1 hunks)src/NLog/SetupBuilderExtensions.cs(2 hunks)tests/NLog.Database.Tests/NLog.Database.Tests.csproj(1 hunks)tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj(1 hunks)tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj(1 hunks)tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj(1 hunks)tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj(1 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(5 hunks)tests/NLog.UnitTests/Config/ReloadTests.cs(1 hunks)tests/NLog.UnitTests/Config/XmlConfigTests.cs(1 hunks)tests/NLog.UnitTests/ConfigFileLocatorTests.cs(8 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs(3 hunks)tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs(3 hunks)tests/NLog.UnitTests/NLog.UnitTests.csproj(1 hunks)tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/NLog.Targets.Network/Targets/NetworkTarget.cs
- tests/NLog.Database.Tests/NLog.Database.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (28)
- src/NLog/Config/ConfigSectionHandler.cs
- tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs
- src/NLog/Internal/AssemblyHelpers.cs
- tests/NLog.UnitTests/Config/XmlConfigTests.cs
- src/NLog/Config/LoggingConfigurationElementExtensions.cs
- tests/NLog.UnitTests/NLog.UnitTests.csproj
- tests/NLog.UnitTests/Config/ReloadTests.cs
- tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj
- src/NLog.Targets.Trace/NLogTraceListener.cs
- src/NLog/Config/XmlParserException.cs
- tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj
- tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj
- src/NLog/Config/XmlLoggingConfigurationElement.cs
- src/NLog/Internal/IFileSystem.cs
- src/NLog/Internal/AppEnvironmentWrapper.cs
- tests/NLog.UnitTests/ConfigFileLocatorTests.cs
- tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj
- tests/NLog.UnitTests/Config/LogFactorySetupTests.cs
- tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj
- tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj
- src/NLog/SetupBuilderExtensions.cs
- tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs
- src/NLog/Config/LoggingConfigurationFileLoader.cs
- src/NLog/Internal/XmlHelper.cs
- src/NLog/Internal/XmlParser.cs
- tests/NLog.UnitTests/Internal/XmlParserTests.cs
- src/NLog/Config/XmlLoggingConfiguration.cs
🔇 Additional comments (3)
src/NLog/Config/XmlParserConfigurationElement.cs (3)
113-153: Well-designed parsing logic with efficient handling of nested elements.The Parse method correctly handles namespace prefixes, special XML attributes, and builds the element hierarchy in a clean way. Good use of the nestedElement flag to avoid processing the root "nlog" element as nested.
158-171: Comprehensive XML special attribute detection.The IsSpecialXmlRootAttribute method does a thorough job of identifying various XML special attributes that should be filtered. Good use of string comparison methods and null checks.
1-172: Overall well-structured implementation of XML parsing for AOT compatibility.This new class effectively bridges the gap between the low-level XmlParser and the higher-level logging configuration parsing logic, eliminating the dependency on System.Xml.XmlReader. The implementation handles namespaces, special attributes, and nested elements correctly while maintaining memory efficiency by using ArrayHelper.Empty<> for empty collections.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/NLog/Config/XmlLoggingConfiguration.cs (1)
371-393: 🛠️ Refactor suggestionPreserve detailed parser diagnostics
ParseFromTextReadercatchesExceptionand wraps it inNLogConfigurationException, losingXmlParserExceptionline/position information that could aid troubleshooting.Consider a dedicated catch for
XmlParserExceptionthat copies its diagnostic properties onto the wrapper, or simply re-throw that type unchanged.
🧹 Nitpick comments (4)
src/NLog/Config/XmlParserConfigurationElement.cs (2)
63-78: Avoid double iteration when exposingValues
Valuesfirst scansChildrento detect any single-value elements and then re-scans them inside theConcat.
For large configs this needlessly touches the list twice.A single pass using
Any()(or caching the predicate result) would be more efficient and easier to read.
107-111: Clarify handling of empty string values
SingleValueElementreturns true whenValue != null, which classifies
<foo></foo>(empty string) the same as<foo>bar</foo>.If the intent is to treat empty-text elements as “attribute-like”, consider
!string.IsNullOrEmpty(child.Value)instead, or add a comment explaining the choice.src/NLog/Config/XmlLoggingConfiguration.cs (2)
87-105: XML-doc still mentions “Configuration file” although the parameter is aTextReaderThe summary/param text for the two new constructors refers to a file being read; this is misleading when a stream could come from memory, embedded resources, etc.
Update the remarks to “configuration source” (or similar) to avoid confusion.
419-430: Use case-insensitive comparison instead ofToUpperInvariant()allocationInside the root-element switch you build an uppercase copy each time:
switch (content.Name.ToUpperInvariant())Avoid the extra allocation by comparing with
Equals(..., StringComparison.OrdinalIgnoreCase)or aStringComparer.OrdinalIgnoreCaseswitch pattern.switch (content.Name) { case var n when n.Equals("configuration", StringComparison.OrdinalIgnoreCase): …This is minor but free performance / GC saving.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/NLog.Targets.Network/Targets/NetworkTarget.cs(1 hunks)src/NLog.Targets.Trace/NLogTraceListener.cs(1 hunks)src/NLog/Config/ConfigSectionHandler.cs(1 hunks)src/NLog/Config/LoggingConfigurationElementExtensions.cs(2 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(3 hunks)src/NLog/Config/XmlLoggingConfiguration.cs(6 hunks)src/NLog/Config/XmlLoggingConfigurationElement.cs(3 hunks)src/NLog/Config/XmlParserConfigurationElement.cs(1 hunks)src/NLog/Config/XmlParserException.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/IFileSystem.cs(2 hunks)src/NLog/Internal/XmlHelper.cs(6 hunks)src/NLog/Internal/XmlParser.cs(1 hunks)src/NLog/SetupBuilderExtensions.cs(2 hunks)tests/NLog.Database.Tests/NLog.Database.Tests.csproj(1 hunks)tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj(1 hunks)tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj(1 hunks)tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj(1 hunks)tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj(1 hunks)tests/NLog.UnitTests/Config/LogFactorySetupTests.cs(5 hunks)tests/NLog.UnitTests/Config/ReloadTests.cs(1 hunks)tests/NLog.UnitTests/Config/XmlConfigTests.cs(1 hunks)tests/NLog.UnitTests/ConfigFileLocatorTests.cs(8 hunks)tests/NLog.UnitTests/Internal/XmlParserTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs(3 hunks)tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs(3 hunks)tests/NLog.UnitTests/NLog.UnitTests.csproj(1 hunks)tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/NLog.Targets.Network/Targets/NetworkTarget.cs
- tests/NLog.UnitTests/Mocks/AppEnvironmentMock.cs
- tests/NLog.Targets.ConcurrentFile.Tests/NLog.Targets.ConcurrentFile.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (26)
- tests/NLog.UnitTests/Config/XmlConfigTests.cs
- tests/NLog.UnitTests/LayoutRenderers/BaseDirTests.cs
- src/NLog/Config/ConfigSectionHandler.cs
- tests/NLog.UnitTests/NLog.UnitTests.csproj
- tests/NLog.UnitTests/LayoutRenderers/MdlcLayoutRendererTests.cs
- tests/NLog.UnitTests/Config/ReloadTests.cs
- src/NLog.Targets.Trace/NLogTraceListener.cs
- tests/NLog.RegEx.Tests/NLog.RegEx.Tests.csproj
- src/NLog/SetupBuilderExtensions.cs
- src/NLog/Internal/IFileSystem.cs
- src/NLog/Config/LoggingConfigurationElementExtensions.cs
- tests/NLog.Database.Tests/NLog.Database.Tests.csproj
- tests/NLog.Targets.Mail.Tests/NLog.Targets.Mail.Tests.csproj
- src/NLog/Config/XmlLoggingConfigurationElement.cs
- tests/NLog.WindowsRegistry.Tests/NLog.WindowsRegistry.Tests.csproj
- src/NLog/Internal/AppEnvironmentWrapper.cs
- tests/NLog.Targets.Network.Tests/NLog.Targets.Network.Tests.csproj
- tests/NLog.UnitTests/Config/LogFactorySetupTests.cs
- src/NLog/Config/XmlParserException.cs
- tests/NLog.UnitTests/ConfigFileLocatorTests.cs
- src/NLog/Internal/AssemblyHelpers.cs
- src/NLog/Config/LoggingConfigurationFileLoader.cs
- src/NLog/Internal/XmlHelper.cs
- tests/NLog.Targets.WebService.Tests/NLog.Targets.WebService.Tests.csproj
- tests/NLog.UnitTests/Internal/XmlParserTests.cs
- src/NLog/Internal/XmlParser.cs
🔇 Additional comments (1)
src/NLog/Config/XmlLoggingConfiguration.cs (1)
320-328: DisposeTextReaderin all code paths
LoadFromXmlFiledisposes theTextReaderviausing, butConfigureFromFile(404-408) follows the same pattern whileParseFromTextReaderleaves disposal to the caller.
Verify that every entry point which allocates a new reader also wraps it inusing; otherwise streams may leak when callers pass raw file readers.
0f6bdee to
2c883c6
Compare
|
XmlReader doesn't introduce AOT-warnings, but does introduce a lot of dependencies, because XmlReader has the ability to load from network-http-url / codegen / etc.