-
Notifications
You must be signed in to change notification settings - Fork 183
Fix/MultiFile pattern (Issue #696) #697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: Development
Are you sure you want to change the base?
Changes from all commits
54b2891
3989ae2
15c3d96
905c979
1fcb262
568bb04
2a5e1ed
ae20df6
00d4c26
d23d228
99d14e8
a1320e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,4 +55,5 @@ private static void MigrateToV1 (Settings settings) | |
| } | ||
| #pragma warning restore CS0618 | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,6 @@ public class RolloverFilenameBuilder | |
| #region Fields | ||
|
|
||
| private string _condContent; | ||
| private Group _condGroup; | ||
| private string _currentFileName; | ||
|
|
||
| private Group _dateGroup; | ||
|
|
@@ -41,6 +40,7 @@ public class RolloverFilenameBuilder | |
|
|
||
| private bool _hideZeroIndex; | ||
| private Group _indexGroup; | ||
| private Group _indexPartGroup; | ||
| private Regex _regex; | ||
|
|
||
| #endregion | ||
|
|
@@ -89,7 +89,7 @@ public void SetFileName (string fileName) | |
| Index = _indexGroup.Value.Length > 0 ? int.Parse(_indexGroup.Value) : 0; | ||
| } | ||
|
|
||
| _condGroup = match.Groups["cond"]; | ||
| _indexPartGroup = match.Groups["indexPart"]; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -116,15 +116,15 @@ public string BuildFileName () | |
|
|
||
| if (_indexGroup != null && _indexGroup.Success) | ||
| { | ||
| fileName = fileName.Remove(_indexGroup.Index, _indexGroup.Length); | ||
| fileName = fileName.Remove(_indexPartGroup.Index, _indexPartGroup.Length); | ||
|
|
||
| if (!_hideZeroIndex || Index > 0) | ||
| { | ||
| var format = "D" + _indexGroup.Length; | ||
| fileName = fileName.Insert(_indexGroup.Index, Index.ToString(format)); | ||
| fileName = fileName.Insert(_indexPartGroup.Index, Index.ToString(format)); | ||
| if (_hideZeroIndex && _condContent != null) | ||
| { | ||
| fileName = fileName.Insert(_indexGroup.Index, _condContent); | ||
| fileName = fileName.Insert(_indexPartGroup.Index, _condContent); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -141,14 +141,14 @@ public string BuildFileName () | |
| private void ParseFormatString (string formatString) | ||
| { | ||
| var fmt = EscapeNonvarRegions(formatString); | ||
| var datePos = formatString.IndexOf("$D(", StringComparison.Ordinal); | ||
| var datePos = fmt.IndexOf("$D(", StringComparison.Ordinal); | ||
|
|
||
| if (datePos != -1) | ||
| { | ||
| var endPos = formatString.IndexOf(')', datePos); | ||
| var endPos = fmt.IndexOf(')', datePos); | ||
| if (endPos != -1) | ||
| { | ||
| _dateTimeFormat = formatString.Substring(datePos + 3, endPos - datePos - 3) | ||
| _dateTimeFormat = fmt.Substring(datePos + 3, endPos - datePos - 3) | ||
| .ToUpperInvariant() | ||
| .Replace('D', 'd') | ||
| .Replace('Y', 'y'); | ||
|
|
@@ -176,12 +176,17 @@ private void ParseFormatString (string formatString) | |
| } | ||
| } | ||
|
|
||
| fmt = fmt.Replace("*", ".*", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("*", ".*?", StringComparison.Ordinal); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .→.? and :194 \A…\z anchoring change every mask, not just ones with trailing literals. It holds because RolloverFilenameHandler.cs:39 passes a bare filename, not a path — but that invariant is undocumented and untested; a full path with any non-*-prefixed mask now fails where it previously matched.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept the end anchor needed to parse an index before fixed suffix, but removed the start anchor. |
||
| _hideZeroIndex = fmt.Contains("$J", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("$I", "(?'index'[\\d]+)", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("$J", "(?'index'[\\d]*)", StringComparison.Ordinal); | ||
|
|
||
| _regex = new Regex(fmt); | ||
| const string indexPattern = "(?'indexPart'(?'index'[\\d]+))"; | ||
| const string hiddenZeroIndexPattern = "(?'indexPart'(?'index'[\\d]*))"; | ||
| var conditionalIndexPattern = _condContent != null | ||
| ? $"(?'indexPart'(?:{Regex.Escape(_condContent)}(?'index'[\\d]+)|(?'index')))" | ||
| : hiddenZeroIndexPattern; | ||
| fmt = fmt.Replace("$I", indexPattern, StringComparison.Ordinal); | ||
| fmt = fmt.Replace("$J", conditionalIndexPattern, StringComparison.Ordinal); | ||
|
|
||
| _regex = new Regex(fmt + @"\z"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. $I digit class went from one-or-more to zero-or-more. A plain app.log now matches *$I.log as index 0, so $I behaves like $J when the opened file has no digits. *$J(.) no longer behaves exactly as before. The lazy wildcard plus the end anchor mean opening engine.log.1 now yields index 1. Previously it yielded index 0 and generated engine.log.1.1. This is a fix, but it is an unstated behaviour change. The FlushEscaped method named in the PR description does not exist. The helpers are AppendEscapedSegment and AppendSegment.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (2) Restored $I behavior:
Note: I cannot find FlushEscaped in the current PR description. |
||
| } | ||
|
|
||
| private string EscapeNonvarRegions (string formatString) | ||
|
|
@@ -199,17 +204,15 @@ private string EscapeNonvarRegions (string formatString) | |
| case 0: // looking for $ | ||
| if (fmt[i] == '$') | ||
| { | ||
| _ = result.Append(Regex.Escape(segment.ToString())); | ||
| segment = new StringBuilder(); | ||
| AppendEscapedSegment(result, segment); | ||
| state = 1; | ||
| } | ||
|
|
||
| _ = segment.Append(fmt[i]); | ||
| break; | ||
| case 1: // the char behind $ | ||
| _ = segment.Append(fmt[i]); | ||
| _ = result.Append(segment); | ||
| segment = new StringBuilder(); | ||
| AppendSegment(result, segment); | ||
| state = 2; | ||
| break; | ||
| case 2: // checking if ( or other char | ||
|
|
@@ -229,18 +232,30 @@ private string EscapeNonvarRegions (string formatString) | |
| _ = segment.Append(fmt[i]); | ||
| if (fmt[i] == ')') | ||
| { | ||
| _ = result.Append(segment); | ||
| segment = new StringBuilder(); | ||
| AppendSegment(result, segment); | ||
| state = 0; | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| } | ||
|
|
||
| AppendEscapedSegment(result, segment); | ||
| fmt = result.ToString().Replace('\xFFFD', '*'); | ||
| return fmt; | ||
| } | ||
|
|
||
| private static void AppendEscapedSegment (StringBuilder result, StringBuilder segment) | ||
| { | ||
| _ = result.Append(Regex.Escape(segment.ToString())); | ||
| _ = segment.Clear(); | ||
| } | ||
|
|
||
| private static void AppendSegment (StringBuilder result, StringBuilder segment) | ||
| { | ||
| _ = result.Append(segment); | ||
| _ = segment.Clear(); | ||
| } | ||
|
|
||
| #endregion | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,11 @@ public void TestFilename1(string expectedResult, string formatString) | |
| [TestCase("engine.log", "engine1.log","engine$J.log")] | ||
| [TestCase("engine1.log", "engine2.log","engine$J.log")] | ||
| [TestCase("engine.log", "engine.log.1","*$J(.)")] | ||
| [TestCase("engine.log", "engine.1.log", "*$J(.).log")] | ||
| [TestCase("engine.log.1", "engine.log.2", "*$J(.)")] | ||
| [TestCase("engine.1.log", "engine.2.log", "*$J(.).log")] | ||
| [TestCase("engine1.log", "engine2.log", "*$I.log")] | ||
| [TestCase("engine_2010-06-12.1.log", "engine_2010-06-12.2.log", "*$D(yyyy-MM-dd)$J(.).log")] | ||
| [TestCase("engine_2010-06-12.log", "engine_2010-06-12.log.1", "*$D(yyyy-MM-dd).log$J(.)")] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. $I untested. For "$J, $I" support, the fix itself generalizes, but no test covers $I or $D followed by a trailing literal (e.g. *$D(yyyy-MM-dd)$J(.).log).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added cases which cover:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ParseFormatString computes datePos from the original string but edits the escaped one — a literal with regex metachars before $D (e.g. app.$D(yyyy-MM-dd)) corrupts the regex. Not introduced here, but the new trailing-literal support makes such patterns likelier.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed by locating and parsing $D(...) in the escaped format string. Offsets remain correct when literal regex metacharacters appear before the date placeholder. |
||
| public void TestFilenameAnd1(string fileName, string expectedResult, string formatString) | ||
| { | ||
|
|
@@ -45,6 +50,7 @@ public void TestFilenameAnd1(string fileName, string expectedResult, string form | |
| [Test] | ||
| [TestCase("engine.log", "engine.log.2","*$J(.)")] | ||
| [TestCase("engine.log", "engine.log.2","*.log$J(.)")] | ||
| [TestCase("engine.log", "engine.2.log", "*$J(.).log")] | ||
| public void TestFilenameAnd2(string fileName, string expectedResult, string formatString) | ||
| { | ||
| RolloverFilenameBuilder fnb = new(formatString); | ||
|
|
@@ -54,6 +60,27 @@ public void TestFilenameAnd2(string fileName, string expectedResult, string form | |
| Assert.That(name, Is.EqualTo(expectedResult)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TestFilenameDateAfterRegexMetacharacter () | ||
| { | ||
| RolloverFilenameBuilder fnb = new("app.$D(yyyy-MM-dd).log"); | ||
| fnb.SetFileName("app.2010-06-12.log"); | ||
|
|
||
| fnb.IncrementDate(); | ||
|
|
||
| Assert.That(fnb.BuildFileName(), Is.EqualTo("app.2010-06-13.log")); | ||
| } | ||
|
|
||
| [Test] | ||
| public void BuildFileName_FullPathWithNonWildcardMask_IncrementsIndex () | ||
| { | ||
| RolloverFilenameBuilder fnb = new("engine$J.log"); | ||
| fnb.SetFileName(@"C:\logs\engine1.log"); | ||
|
|
||
| fnb.Index += 1; | ||
|
|
||
| Assert.That(fnb.BuildFileName(), Is.EqualTo(@"C:\logs\engine2.log")); | ||
| } | ||
|
|
||
| [Test] | ||
| [TestCase("engine1.log", "engine.log","engine$J.log")] | ||
|
|
@@ -65,4 +92,4 @@ public void TestFilenameMinus1(string fileName, string expectedResult, string fo | |
| var name = fnb.BuildFileName(); | ||
| Assert.That(name, Is.EqualTo("engine.log")); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,24 @@ public void SingleFileCtor_MultiFileTrue_ExpandsRollover () | |
| }); | ||
| } | ||
|
|
||
| [Test] | ||
| public void SingleFileCtor_MultiFileTrue_LoadsIndexBeforeExtension () | ||
| { | ||
| var options = new MultiFileOptions { FormatPattern = "*$J(.).log" }; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The $I case is only tested from the active file, despite the PR description claiming a rotated-file $I test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (3) Updated the $I test to start from a rotated filename: |
||
| File.Copy(Path.Combine(_testDataDirectory, "app.1.log"), Path.Combine(_testDirectory, "app.1.log")); | ||
| File.Copy(Path.Combine(_testDataDirectory, "app.2.log"), Path.Combine(_testDirectory, "app.2.log")); | ||
| using var reader = CreateSingleFileReader(multiFile: true, options); | ||
|
|
||
| reader.ReadFiles(); | ||
|
|
||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(reader.IsMultiFile, Is.True); | ||
| Assert.That(reader.GetLogFileInfoList().Select(file => Path.GetFileName(file.FullName)), | ||
| Is.EqualTo(new[] { "app.2.log", "app.1.log", "app.log" })); | ||
| }); | ||
| } | ||
|
|
||
| [Test] | ||
| public void MultiFileCtor_AlwaysMultiFile () | ||
| { | ||
|
|
@@ -91,15 +109,15 @@ public void MultiFileCtor_AlwaysMultiFile () | |
| }); | ||
| } | ||
|
|
||
| private LogfileReader CreateSingleFileReader (bool multiFile) | ||
| private LogfileReader CreateSingleFileReader (bool multiFile, MultiFileOptions? options = null) | ||
| { | ||
| return new LogfileReader( | ||
| _logFile, | ||
| new EncodingOptions { Encoding = Encoding.UTF8 }, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. opens the active file only. Opening a rotated file directly never picks up lower siblings because the name-list walk only goes upward. That is pre-existing, but the spec is only met when the active file is opened. No round-trip test for *$J(.).log.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that this is intended behavior: when a rotated file such as app.1.log is opened, MultiFile loads that file and higher indexes only. To load the complete chain including lower indexes, the user should open the active file (app.log). If user want to open log file from specified date only and further, i.e. app.8.log, this is a useful behaviour and in this case app will load app.8.log -> app.9.log etc. Please confirm whether this is correct. If MultiFile should also discover lower-index siblings when a rotated file is opened directly, I can extend the handler and add coverage for that behavior. |
||
| multiFile, | ||
| bufferCount: 40, | ||
| linesPerBuffer: 50, | ||
| new MultiFileOptions(), | ||
| options ?? new MultiFileOptions(), | ||
| ReaderType.System, | ||
| PluginRegistry.PluginRegistry.Instance, | ||
| maximumLineLength: 500, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| app.1.log |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| app.2.log |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A saved mask *$J(.).log previously behaved as *$J(.) and matched app.log.1; it now means app.1.log. That's what the spec wants, but existing user settings change
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the corrected syntax remain this way?
If so, should I migrate existing saved *$J(.).log masks to *$J(.)?
(7)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added settings migration for corrected MultiFile mask behavior.
To preserve compatibility, settings version 2 migrates existing saved *$J(.).log preferences to *$J(.). New version-2 settings keep both patterns distinct:
Added tests for migrating old settings and for ensuring new settings are not changed.