diff --git a/AGENTS.md b/AGENTS.md index a3cfec08..b81cf63d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,6 +28,10 @@ almost always be doing. interface or abstract class, as in `Func f = () => new MailKitSmtpTransport();`. - `x?.Method() ?? false` rather than `x is not null && x.Method()`, and merge nested guards into one condition. +- In a method whose conditions are hard to follow, give their parts names: small local functions + such as `IsBaseFileRolledByDate()` or `HasNumberedBackups()`, even one-liners and even chained, + so the `if` reads as a sentence. See `RollingFileAppender.DoesDatePatternCollideWithBackupNames`. + In review, do not suggest inlining them to save lines. A simple guard stays inline. - Expression-bodied members whenever the body fits on one line, including constructors (`resharper_constructor_or_destructor_body = expression_body`). - Braces on `if`/`else` bodies even for a single statement. diff --git a/src/changelog/3.5.1/335-short-date-pattern-collides-with-backup-names.xml b/src/changelog/3.5.1/335-short-date-pattern-collides-with-backup-names.xml new file mode 100644 index 00000000..a4ec4d6a --- /dev/null +++ b/src/changelog/3.5.1/335-short-date-pattern-collides-with-backup-names.xml @@ -0,0 +1,13 @@ + + + + With `StaticLogFileName` a `DatePattern` such as `.dd` named dated + files like numbered backups, so a roll could overwrite one with the other. Such an appender is now + reported when activated and uses the dated file name as with `StaticLogFileName` false, so its + backups are numbered after the date, and an existing static file is left untouched, including + its numbered backups. Use a `DatePattern` with a separator, such as the default `.yyyy-MM-dd`, + to keep the static name (reported by @Snotface, fixed by @FreeAndNil) + diff --git a/src/log4net.Tests/Appender/RollingFileAppenderShortDatePatternTest.cs b/src/log4net.Tests/Appender/RollingFileAppenderShortDatePatternTest.cs new file mode 100644 index 00000000..5e24b001 --- /dev/null +++ b/src/log4net.Tests/Appender/RollingFileAppenderShortDatePatternTest.cs @@ -0,0 +1,191 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System; +using System.Collections.Generic; +using System.IO; +using log4net.Appender; +using log4net.Core; +using log4net.Layout; +using log4net.Util; + +using NUnit.Framework; + +using PeanutButter.Utils; + +namespace log4net.Tests.Appender; + +/// +/// What does when a static name and a short date pattern name +/// dated files like numbered backups, see #335. +/// +[TestFixture] +public sealed class RollingFileAppenderShortDatePatternTest +{ + /// + /// With a static name, .dd names the file of the 10th like backup 10, so the appender is + /// reported and uses the dated name instead, with its backups numbered after the date. + /// + [Test] + [NonParallelizable] + public void ShortDatePatternUsesDatedFileName() + { + using AutoTempFolder folder = new(); + string file = Path.Combine(folder.Path, "log.txt"); + RollingFileAppender appender = CreateShortDatePatternAppender(file, ".dd", 10, -1); + appender.MaxFileSize = 100; + List messages = ActivateCapturingInternalMessages(appender); + try + { + Assert.That(messages, Has.Exactly(1).Contains("DatePattern [.dd]")); + for (int i = 0; i < 5; i++) + { + appender.DoAppend(CreateEvent(new string('a', 200))); + } + } + finally + { + appender.Close(); + } + + string[] files = Directory.GetFiles(folder.Path); + Assert.That(files, Does.Not.Contain(file)); + Assert.That(files, Does.Contain(file + ".04").And.Contain(file + ".04.1")); + } + + /// + /// Only a date that a backup name can reach is a collision: counting down to 5 never writes .10, + /// a non-static name keeps its backups apart from the dated files, and five or more digits + /// never count as a backup. + /// + [TestCase(".d", true, 5, -1, true)] + [TestCase(".dd", true, 5, 1, true)] + [TestCase(".dd", true, 5, -1, false)] + [TestCase(".dd", true, 10, -1, true)] + [TestCase(".dd", true, -1, -1, true)] + [TestCase(".dd", false, 10, -1, false)] + [TestCase(".yyyyMMdd", true, 10, 1, false)] + [TestCase(".yyyy-MM-dd", true, 10, -1, false)] + [NonParallelizable] + public void ShortDatePatternIsReportedOnlyWhenBackupsCollide(string datePattern, + bool staticLogFileName, int maxSizeRollBackups, int countDirection, bool reported) + { + using AutoTempFolder folder = new(); + string file = Path.Combine(folder.Path, "log.txt"); + RollingFileAppender appender = CreateShortDatePatternAppender(file, datePattern, + maxSizeRollBackups, countDirection, staticLogFileName); + List messages = ActivateCapturingInternalMessages(appender); + appender.Close(); + + Assert.That(messages, + Has.Exactly(reported ? 1 : 0).Contains("names dated files like numbered backups")); + } + + /// + /// Date rolling that appends makes no numbered backups, so there is nothing to collide with. + /// + [Test] + [NonParallelizable] + public void DateRollingWithAppendIsNotReported() + { + using AutoTempFolder folder = new(); + string file = Path.Combine(folder.Path, "log.txt"); + RollingFileAppender appender = CreateShortDatePatternAppender(file, ".d", 10, -1, + rollingStyle: RollingFileAppender.RollingMode.Date); + List messages = ActivateCapturingInternalMessages(appender); + appender.Close(); + + Assert.That(messages, Is.Empty); + } + + /// + /// Date rolling without appending rolls the existing file to .1 at startup, which with .d is the + /// name of the file of the 1st, so the appender leaves it alone and opens the dated file. + /// + [Test] + [NonParallelizable] + public void ShortDatePatternRestartWithoutAppendKeepsExistingFile() + { + using AutoTempFolder folder = new(); + string file = Path.Combine(folder.Path, "log.txt"); + File.WriteAllText(file, "previous run"); + // the same period as the mocked clock, so the start-up does not roll by date + File.SetLastWriteTime(file, _mockedNow); + RollingFileAppender appender = CreateShortDatePatternAppender(file, ".d", 3, -1, + rollingStyle: RollingFileAppender.RollingMode.Date, appendToFile: false); + List messages = ActivateCapturingInternalMessages(appender); + appender.Close(); + + Assert.That(messages, Has.Exactly(1).Contains("DatePattern [.d]")); + Assert.That(Directory.GetFiles(folder.Path), Is.EquivalentTo(new[] { file, file + ".4" })); + Assert.That(File.ReadAllText(file), Is.EqualTo("previous run")); + } + + /// + /// With a dated name and .d, the files of the 11th start with the name of the file of the 1st + /// and must not count as its backups. + /// + [Test] + public void DatedNameCountsOnlyItsOwnBackups() + { + RollingFileAppender appender = CreateShortDatePatternAppender("log.txt", ".d", 5, 1, + staticLogFileName: false); + appender.DateTimeStrategy = new Integration.MockDateTime(new(2026, 10, 1)); + appender.InitializeRollBackups("log.txt", + ["log.txt.1", "log.txt.1.2", "log.txt.11", "log.txt.11.7"]); + + Assert.That(appender.CurrentSizeRollBackups, Is.EqualTo(2)); + } + + private static readonly DateTime _mockedNow = new(2026, 10, 4, 9, 0, 0); + + private static RollingFileAppender CreateShortDatePatternAppender(string file, + string datePattern, int maxSizeRollBackups, int countDirection, bool staticLogFileName = true, + RollingFileAppender.RollingMode rollingStyle = RollingFileAppender.RollingMode.Composite, + bool appendToFile = true) => new() + { + File = file, + Layout = new PatternLayout("%message%newline"), + RollingStyle = rollingStyle, + DatePattern = datePattern, + StaticLogFileName = staticLogFileName, + MaxSizeRollBackups = maxSizeRollBackups, + CountDirection = countDirection, + AppendToFile = appendToFile, + DateTimeStrategy = new Integration.MockDateTime(_mockedNow) + }; + + private static List ActivateCapturingInternalMessages(RollingFileAppender appender) + { + List messages = []; + LogLog.ExecuteWithoutEmittingInternalMessages(() => + { + using LogLog.LogReceivedAdapter _ = new(messages); + appender.ActivateOptions(); + }); + return messages.ConvertAll(m => m.Message); + } + + private static LoggingEvent CreateEvent(string message) => new(new LoggingEventData + { + Level = Level.Info, + Message = message, + LoggerName = "ShortDatePattern" + }); +} diff --git a/src/log4net/Appender/RollingFileAppender.cs b/src/log4net/Appender/RollingFileAppender.cs index 0cbdd144..15dc84a7 100644 --- a/src/log4net/Appender/RollingFileAppender.cs +++ b/src/log4net/Appender/RollingFileAppender.cs @@ -87,7 +87,9 @@ namespace log4net.Appender; /// /// /// Changing or without clearing -/// the log file directory of backup files will cause unexpected and unwanted side effects. +/// the log file directory of backup files will cause unexpected and unwanted side effects. +/// The appender itself sets to when the +/// names dated files like numbered backups, see there. /// /// /// @@ -271,6 +273,12 @@ public RollingFileAppender() /// This property determines the rollover schedule when rolling over /// on date. /// + /// + /// With a pattern that formats as a dot and up to four digits, + /// such as .dd, names dated files like numbered backups. Such an appender is reported + /// when activated and then reads as false, so the live file + /// is the dated one and its backups are numbered after the date. + /// /// public string? DatePattern { get; set; } = ".yyyy-MM-dd"; @@ -865,36 +873,15 @@ private void InitializeFromOneFile(string baseFile, string curFileName) string dir = string.IsNullOrEmpty(baseFile) ? "" : Path.GetDirectoryName(baseFile); string baseFileWithoutExtension = Path.Combine(dir ?? "", Path.GetFileNameWithoutExtension(baseFile)); - if (curFileName.StartsWith(baseFileWithoutExtension) == false) + if (!IsLogFile() || IsBaseFile()) { - return; // This is not a log file, so ignore - } - - if (curFileName.Equals(baseFile, StringComparison.Ordinal)) - { - return; // Base log file is not an incremented logfile (.1 or .2, etc.) + return; } // Only look for files in the current roll point - if (_rollDate && !StaticLogFileName) + if (_rollDate && !StaticLogFileName && !IsBackupOfDatedFile(Path.GetFileName(curFileName))) { - string date = DateTimeStrategy.Now.ToString(DatePattern, DateTimeFormatInfo.InvariantInfo).ToLowerInvariant(); - // the date pattern may supply the extension, so take it from the dated name - string dated = Path.GetFileName(CombinePath(baseFile, date)); - string suffix = PreserveLogFileNameExtension ? Path.GetExtension(dated) : ""; - string prefix = dated.Substring(0, dated.Length - suffix.Length); - string curFileNameWithoutDir = Path.GetFileName(curFileName); - if (!curFileNameWithoutDir.StartsWith(prefix) || !curFileNameWithoutDir.EndsWith(suffix)) - { - LogLog.Debug(_declaringType, $"Ignoring file [{curFileName}] because it is from a different date period"); - return; - } - - // the dated file itself has no index, and a date like .yyyyMMdd would parse as one - if (curFileNameWithoutDir.Length <= dated.Length) - { - return; - } + return; } try @@ -939,6 +926,31 @@ private void InitializeFromOneFile(string baseFile, string curFileName) //when staticLogFileName == false LogLog.Debug(_declaringType, $"Encountered a backup file not ending in .x [{curFileName}]"); } + + bool IsLogFile() => curFileName.StartsWith(baseFileWithoutExtension); + + // the base log file is not an incremented log file (.1 or .2, etc.) + bool IsBaseFile() => curFileName.Equals(baseFile, StringComparison.Ordinal); + + bool IsBackupOfDatedFile(string fileName) + { + string date = DateTimeStrategy.Now.ToString(DatePattern, DateTimeFormatInfo.InvariantInfo) + .ToLowerInvariant(); + // the date pattern may supply the extension, so take it from the dated name + string dated = Path.GetFileName(CombinePath(baseFile, date)); + string suffix = PreserveLogFileNameExtension ? Path.GetExtension(dated) : ""; + string prefix = dated.Substring(0, dated.Length - suffix.Length); + // the dot keeps log.txt.11 out of the scan for log.txt.1 + if (!fileName.StartsWith(prefix + ".") || !fileName.EndsWith(suffix)) + { + LogLog.Debug(_declaringType, + $"Ignoring file [{curFileName}] because it is from a different date period"); + return false; + } + + // the dated file itself has no index, and a date like .yyyyMMdd would parse as one + return fileName.Length > dated.Length; + } } /// @@ -1000,6 +1012,37 @@ public void InitializeRollBackups(string baseFile, IList arrayFiles) } } + /// + /// With a static name, a date that formats as a dot and up to four digits (.dd writes + /// .10) is also the name of backup 10. Counting down to a limit below the smallest such + /// name never reaches it. + /// + private bool DoesDatePatternCollideWithBackupNames() + { + if (DatePattern is not string datePattern || !IsBaseFileRolledByDate() || !HasNumberedBackups()) + { + return false; + } + + string sample = _sDate1970.ToString(datePattern, DateTimeFormatInfo.InvariantInfo); + return IsDotAndUpToFourDigits(sample) && CanBackupNumberEqualDate(sample); + + bool IsBaseFileRolledByDate() => _rollDate && StaticLogFileName; + + bool HasNumberedBackups() => MaxSizeRollBackups != 0 && DoesRollBySizeOrAtStart(); + + bool DoesRollBySizeOrAtStart() => _rollSize || !AppendToFile; + + static bool IsDotAndUpToFourDigits(string sample) + => sample.Length is >= 2 and <= 5 && sample[0] == '.' + && sample.Skip(1).All(c => c is >= '0' and <= '9'); + + // 1970-01-01 00:00 writes every field at its narrowest, so dated names start at 10^(digits - 1) + bool CanBackupNumberEqualDate(string sample) + => CountDirection >= 0 || MaxSizeRollBackups < 0 + || MaxSizeRollBackups >= (int)Math.Pow(10, sample.Length - 2); + } + /// /// Calculates the RollPoint for the datePattern supplied. /// @@ -1088,6 +1131,17 @@ public override void ActivateOptions() } } + if (DoesDatePatternCollideWithBackupNames()) + { + // LogLog, not ErrorHandler: OnlyOnceErrorHandler would report nothing after this one + LogLog.Warn(_declaringType, $""" + DatePattern [{DatePattern}] of [{Name}] names dated files like numbered backups, so the + appender uses a dated file name as with StaticLogFileName false. Use a DatePattern with + a separator, such as the default .yyyy-MM-dd, to keep the static name. + """); + StaticLogFileName = false; + } + SecurityContext ??= SecurityContextProvider.DefaultProvider.CreateSecurityContext(this); using (SecurityContext.Impersonate(this)) diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/rollingfileappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/rollingfileappender.adoc index fd169688..645e3fe7 100644 --- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/rollingfileappender.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/rollingfileappender.adoc @@ -88,6 +88,13 @@ For each day only the last 10 files of 1MB will be kept. ---- +[NOTE] +==== +With `staticLogFileName` a `datePattern` that formats as a dot and up to four digits, such as `.dd`, names dated files like numbered backups. +Such an appender is reported when activated and uses the dated file name as with `staticLogFileName` false, so its backups are numbered after the date. +Use a pattern with a separator, such as the default `.yyyy-MM-dd`, to keep the static name. +==== + This example show how to configure the `RollingFileAppender` to roll log files once per program execution. The `appendToFile` property is set to false to prevent the appender from overwriting the existing files.