hidingmyname opened a new issue, #340:
URL: https://github.com/apache/logging-log4net/issues/340
### Summary
`src/log4net/Appender/IAppender.cs`:
```csharp
/// <summary>
/// Gets or sets the name of this appender.
/// </summary>
/// <value>The name of the appender.</value>
/// <remarks>
/// <para>The name uniquely identifies the appender.</para>
/// </remarks>
string Name { get; set; }
```
Nothing in log4net makes that true. `Name` is a plain settable property, and
attaching appenders doesn't check it: `AppenderAttachedImpl.AddAppender`
dedupes by reference, not by name:
```csharp
public void AddAppender(IAppender appender)
{
appender.EnsureNotNull();
_appenderArray = null;
_appenderList ??= new(1);
if (!_appenderList.Contains(appender))
{
_appenderList.Add(appender);
}
}
```
So two appenders with the same `Name` can both be attached to the same
logger.
When that happens, lookup silently picks the first one:
```csharp
public IAppender? GetAppender(string? name)
{
if (_appenderList is not null && name is not null)
{
foreach (IAppender appender in _appenderList)
{
if (name == appender.Name)
{
return appender;
}
}
}
return null;
}
```
and `RemoveAppender(string name)` is `RemoveAppender(GetAppender(name))`, so
it
removes the first match and leaves the other attached.
### Why it matters
The doc reads as a guarantee you can build on. "Name uniquely identifies the
appender" invites you to treat `GetAppender(name)` / `RemoveAppender(name)`
as
addressing *the* appender. With a duplicated name (easy to produce from
programmatic configuration, or by assembling config from fragments) you get
appenders you can't reach or remove by name, and no warning anywhere. Stating
that the name is a label rather than a key sets the right expectation.
### Suggested fix
```diff
/// <value>The name of the appender.</value>
/// <remarks>
- /// <para>The name uniquely identifies the appender.</para>
+ /// <para>Used to look the appender up; uniqueness is not enforced,
+ /// and lookup returns the first match.</para>
/// </remarks>
string Name { get; set; }
```
and the implementation side, `src/log4net/Appender/AppenderSkeleton.cs`:
```diff
/// <summary>
- /// Gets or sets the name that uniquely identifies this appender.
+ /// Gets or sets the name of this appender.
/// </summary>
public string Name { get; set; } = string.Empty;
```
### Upstream reference
log4j 1.x had the same wording on `Appender.getName()` and corrected it under
Bug 43849 ("Javadoc for Appender.getName implies return value will be unique
and
not null"), apache/logging-log4j1@e3fecab964 (2008-08-12):
```diff
--- a/src/main/java/org/apache/log4j/Appender.java
+++ b/src/main/java/org/apache/log4j/Appender.java
@@ -74,8 +74,8 @@ public interface Appender {
/**
- Get the name of this appender. The name uniquely identifies the
- appender. */
+ Get the name of this appender.
+ @return name, may be null.*/
public
String getName();
```
The same commit also touched `AppenderSkeleton`:
```diff
--- a/src/main/java/org/apache/log4j/AppenderSkeleton.java
+++ b/src/main/java/org/apache/log4j/AppenderSkeleton.java
@@ -189,7 +189,8 @@ public abstract class AppenderSkeleton implements
Appender, OptionHandler {
/**
- Returns the name of this FileAppender.
+ Returns the name of this appender.
+ @return name, may be null.
*/
public
final
```
Only the uniqueness half of that carries over to log4net: `Name` here is
declared `string Name { get; set; }` in a nullable-annotated file, so "may be
null" would be wrong. Hence the suggested fix drops the uniqueness claim and
says what the name is actually good for, rather than copying log4j's text
verbatim.
log4net's implementation side has the same claim, so it's the same two files
log4j touched. `src/log4net/Appender/AppenderSkeleton.cs` additionally
defaults
`Name` to `string.Empty`, which is another reason it can't be treated as an
identifier: two appenders that were never named both answer `""`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
