Use the archive-filename as name of file-entry in the compressed zip file

[TalAloni]

This PR sets the entry name for the filename inside the zip (when compression is enabled) according to the archiveFileName format.

Motivation: I have recently enabled compression and found myself in a situation where I cannot mass-extract archived log files to the same directory because they are all using the same filename inside the archive. it seems to me that the rules for the archive filename should also apply to the entry name inside the archive.
Simply put, I expect that once I extract the zip files, I will have the same filenames that I had before enabling compression.

Additional information:
I have worked with the following configuration

<target name="file" xsi:type="File" fileName="${basedir}/logs/log-latest.csv" archiveFileName="${basedir}/logs/log.{#####}.csv">

Now I wish to enable compression using the following configuration:

<target name="file" xsi:type="File" fileName="${basedir}/logs/log-latest.csv" archiveFileName="${basedir}/logs/log.{#####}.zip" enableArchiveFileCompression="true">

[welcome]

Thanks for opening this pull request!
We will try to review this soon! Please note that pull requests with unit tests are earlier accepted 👼

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[snakefoot]

This change makes a lot of sense, but is ofcourse a breaking change, and will have to wait for NLog 5.0

I guess the FileTarget can eat one more bullet, but I think it would be nice if the FileTarget was stripped from any copy/move/delete/compress-logic. Instead the file-manipulation-logic should be implemented using one (or more) FileTargetWrapper. See also #2732 and #1249

[snakefoot]

Closing and reopening to rebuild on Travis.

[snakefoot]

Besides being a behavior breaking change, then it also breaks the IFileCompressor-interface.

I guess very few have made override of the IFileCompressor-interface, but would prefer if the upgrade-path was less breaking. Thus introducing a new interface, and make a wrapper for the old interface-property and mark the old interface as obsolete.

[TalAloni]

Thanks,
There's already a FileCompressor property of type IFileCompressor.
It seems to me you're suggesting we add an "IArchiveFileCompressor" that inherits from IFileCompressor, and then check if we're dealing with IArchiveFileCompressor or the now obsolete IFileCompressor.

This seems a bit excessive, but I'll do it if you think it's necessary.
IMO given that not many inherited the interface maybe it's best to just reduce noise - it will be simple enough for implementing classes to add the new parameter to the implementation - given that it is targeted for a major version I think that's acceptable.
I don't think that it will be clearly understood that you can implement IArchiveFileCompressor if the property type is IFileCompressor - so I find the approach you're describing less appealing.

[snakefoot]

I was thinking 3 items:

• IArchiveFileCompressor - New interface
• LegacyArchiveFileCompressor - Implements new interface and takes legacy IFileCompressor as input-parameter
• ZipArchiveFileCompressor - Implements new interface

The do the following:

• Mark IFileCompressor as obsolete
• Mark FileTarget FileCompressor-property as obsolete
• Change FileTarget FileCompressor-property implementation. Let the setter allocate LegacyArchiveFileCompressor and give the propery-value as constructor-parameter. Let the getter return the IFileCompressor inside the LegacyArchiveFileCompressor.

[TalAloni]

I see,
We can also just change the implementation of ZipArchiveFileCompressor - so it would contain the Path.GetFileNameWithoutExtension(archiveFileName) + Path.GetExtension(fileName)
logic - this way the interface will remain the same - if that's important to you than maybe we should settle for this simple solution.
Let me know what you think is best. thanks!

[snakefoot]

Like that work-around. Replacing interface and guiding users to the new-interface generates a lot of code-bloat.

Notice that you can also assign your own IFileCompressor for NLog 4.5 with the wanted behavior. While waiting for NLog 5.

[TalAloni]

Great, I've made the change - the interface remain unchanged. this is tested and working as expected.

[snakefoot]

Very elegant solution. I like it.

Tried to figure out whether there was any special reasons for the initial solution (with static filename). But it seems it was just hacked together until it worked, without much consideration about entry-filename. But still breaking change so waiting for NLog 5.0

[snakefoot]

@TalAloni Could you extend one of the existing unit-test to verify that the entry-filename is as expected? (Then you will also have better gurantee that it doesn't break in the future).

[snakefoot]

@TalAloniI would prefer that AssertZipFileContents was used for the unit-test to verify the contents of final zip-file (Having the expected the Entry-Name)

[TalAloni]

This would mean adding an additional parameter to AssertZipFileContents (archiveFileName or entryName) and update a few dozens existing tests.
Instead we can add a new AssertZipFileEntryName which would be less obtrusive, which do you prefer?

[snakefoot]

I can now see that I have given wrong advice. The FileCompressor-property is a static property, so having a single FileTarget changing properties on this static-instance will not work well, when using multiple FileTargets in the same NLog.config.

So the entry-name must be given as input-parameter when calling the IFileCompressor. To provide safe passage, then a V2-interface must be introduced (that includes the method with entry-name-parameter). And then call this people when the static-instance implements the V2-interface (else call the old interface-method).

[TalAloni]

"So the entry-name must be given as input-parameter when calling the IFileCompressor"
Yes, this approach is healthier.

"To provide safe passage, then a V2-interface must be introduced"
Again, I think it's too cumbersome, I think it's extremely rare for users to implement this interface, we add a lot of noise for background compatibility that nobody really care for, and that could easily be updated to the new interface.
I will do it, but only to please you, I think that it's a messy and confusing approach. using the existing interface and property would be much cleaner and easier for most users, if not all.

[snakefoot]

This will also do:

if (FileCompressor is ZipArchiveFileCompressor defaultCompressor && ArchiveNumbering != ArchiveNumberingMode.Rolling)
{
   string entryFileName = Path.GetFileNameWithoutExtension(archiveFileName) + Path.GetExtension(fileName);
   defaultCompressor.CompressFile(fileName, archiveFileName, entryFileName);   // internal method
}
else
{
   FileCompressor.CompressFile(fileName, archiveFileName);
}

Skipping the V2-interface

[TalAloni]

I've made the requested changes. I've added a new interface extending the existing interface to improve extendibility.

[TalAloni]

Thanks,
The idea beyond making IArchiveFileCompressor public is to allow developers to set FileCompressor to an IArchiveFileCompressor implementation, this way they will be able to have access to the 'entryName' argument.

[snakefoot]

The NLog FileTarget has so many door-knobs that it is difficult for normal users to figure out how to use them. Several of the door-knobs are counter intuitive, and make normal users think the NLog FileTarget is broken. Adding more public door-knobs will not improve the situation.

Like I said initially "I guess the FileTarget can eat one more bullet, but I think it would be nice if the FileTarget was stripped" (down).

[sonarcloud]

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

62.5% Coverage
0.0% Duplication

[welcome]

Hooray your first pull request is merged! Thanks for the contribution! Looking for more? 👼 Please check the up-for-grabs issues - thanks!

[snakefoot]

Sorry for taking so long to get this pull-request merged. But very grateful for the unit-tests, they have been missing since the zip-feature was introduced.

Maybe one day a champion will arrive that will carve the NLog FileTarget into smaller pieces, so it will be easier to maintain and with improved platform compability.

[TalAloni]

Thanks for the great and very useful project!

[snakefoot]

Updated the wiki: https://github.com/NLog/NLog/wiki/File-target