[logback-dev] [JIRA] (LOGBACK-1257) Invalid files in case of append=true and Encoder with non-null headerBytes() / footerBytes()

[QOS.CH (JIRA)]

Title: Message Title

Joern Huxhorn commented on LOGBACK-1257   Re: Invalid files in case of append=true and Encoder with non-null headerBytes() / footerBytes() This commit implements your suggested fix. The relevant code looks like this: // suggested LOGBACK-1257 workaround below private OutputStreamAppender<?> parent; public void setParent(OutputStreamAppender<?> parent) { this.parent = parent; } private boolean isSupposedToGenerateHeader() { if(parent instanceof FileAppender) { FileAppender fileAppender = (FileAppender) parent; if(!fileAppender.isAppend()) { return true; } OutputStream outputStream = fileAppender.getOutputStream(); if(outputStream instanceof ResilientFileOutputStream) { ResilientFileOutputStream fileOutputStream = (ResilientFileOutputStream) outputStream; File file = fileOutputStream.getFile(); if(file.length() != 0) { // appending and file with already existing content => no header. return false; } } } return true; } // not necessary for this encoder since it doesn't require a footer /* private boolean isSupposedToGenerateFooter() { if(parent instanceof FileAppender) { FileAppender fileAppender = (FileAppender) parent; if(fileAppender.isAppend()) { return false; } } return true; } */ // suggested LOGBACK-1257 workaround above The headerBytes() is returning null if !isSupposedToGenerateHeader() while footerBytes() would return null if !isSupposedToGenerateFooter() (not necessary in my case because I don't need/have a footer). This works but I wholeheartedly disagree that this is the way this should be implemented for several reasons: Every single Encoder in need of header or footer would have to re-implement this identical functionality. The logic above is non-trivial to test which is a good indicator that something is wrong about the approach. Testing something simple like an Encoder should be entirely straight-forward. It should be the responsibility of Appender implementations to know whether or not or in which cases headers or footers should be written. If someone decided to implement a TapeDriveAppender unrelated to FileAppender, the above logic would have to be expanded accordingly, for each and every new Appender implementation. Each special handling would further increase the complexity of the Encoder tests. Single responsibility principle and Dependency inversion principle are violated by the above approach. Because of this, the above code is very fragile in nature and likely to break in case of otherwise internal changes to Logback. It would break if ch.qos.logback.core.FileAppender, ch.qos.logback.core.OutputStreamAppender, ch.qos.logback.core.recovery.ResilientFileOutputStream or ch.qos.logback.core.encoder.EncoderBase changed in an incompatible way while only ch.qos.logback.core.encoder.EncoderBase should have any impact. The above mentioned fragility isn't a theoretical issue either because the tests of the previous workaround implementation already broke when you replaced ResilientFileOutputStream(File file, boolean append) with ResilientFileOutputStream(File file, boolean append, long bufferSize) between 1.1.9 and 1.1.10 releases. See the related commit. While I implemented the above workaround, I was taking a look at ch.qos.logback.classic.log4j.XMLLayout because surely some similar logic would have to be present in that class. Well... it isn't. The reason for this is that XMLLayout does not (in contrast to what the javadoc is saying) write XML documents compliant to log4j.dtd. It is omitting the (optional) <?xml version="1.0" encoding="UTF-8"?> XML prolog as well as the (mandatory) enclosing <eventSet></eventSet> root element, instead simply concatenating <event></event> elements. At that point I remembered the workaround I implemented years ago to parse those files. A proper implementation (i.e. one that is creating well-formed XML documents compliant to the DTD) of an XMLEncoder would need the exact same logic implemented in the above workaround to decide if header and footers should be written. Add Comment   This message was sent by Atlassian JIRA (v7.3.1#73012-sha1:68837e3)

_______________________________________________
logback-dev mailing list
logback-dev@qos.ch
http://mailman.qos.ch/mailman/listinfo/logback-dev