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

Fix placeholder resolving in FileWriter #70

Merged
merged 1 commit into from Feb 23, 2018

Conversation

3 participants
@f4lco

f4lco commented Feb 23, 2018

I relied pretty much on the resolving of environment variables in filenames supplied to FileWriter and noticed that occasionally the placeholders would not be replaced at all.

My analysis is as follows:

  1. Previously not all constructors of FileWriter resolved the path.

  2. org.pmw.tinylog.PropertiesLoader#loadWriter makes use of java.lang.Class#getDeclaredConstructors, which, according to the Javadoc, does not return constructors in a particular order, and org.pmw.tinylog.PropertiesLoader#areCompatible, which is responsible for making the two signatures

     FileWriter(java.lang.String, boolean, boolean)                     (1)
     FileWriter(java.lang.String, java.lang.Boolean, java.lang.Boolean) (2)
    

    compare identical.

  3. The constructor was chosen at random (with higher chance of selecting (1)), replacing environment variables most of the time, but leaving the filename as-is in some instances using (2).

My initial fix is adding the missing call to PathResolver.resolve.
I doubt there is actually a need for maintaining both constructors. Maybe there are even more defects due to the fact that PropertiesLoader does not consistently select the same constructor, like missing optional values causing an NPE when coerced to one of the primitive types. I'll leave it up to you to amend the PR with a removal.

@codecov-io

This comment has been minimized.

codecov-io commented Feb 23, 2018

Codecov Report

Merging #70 into v1.3 will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##               v1.3      #70   +/-   ##
=========================================
  Coverage     93.01%   93.01%           
  Complexity     1325     1325           
=========================================
  Files            66       66           
  Lines          3464     3464           
  Branches        505      505           
=========================================
  Hits           3222     3222           
  Misses          154      154           
  Partials         88       88
Impacted Files Coverage Δ Complexity Δ
...inylog/src/org/pmw/tinylog/writers/FileWriter.java 92.5% <100%> (ø) 13 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57695be...363fff7. Read the comment docs.

@pmwmedia

This comment has been minimized.

Owner

pmwmedia commented Feb 23, 2018

Thank you for reporting and fixing the bug!

@pmwmedia pmwmedia merged commit 6589773 into pmwmedia:v1.3 Feb 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pmwmedia

This comment has been minimized.

Owner

pmwmedia commented Feb 23, 2018

I have just released the fix: http://www.tinylog.org/news#89

@f4lco f4lco deleted the f4lco:fix-filewriter-resolving branch Mar 5, 2018

@f4lco

This comment has been minimized.

f4lco commented Mar 5, 2018

Thank you very much, @pmwmedia!
When can I expect 1.3.4 to appear in Maven Central?

@pmwmedia

This comment has been minimized.

Owner

pmwmedia commented Mar 5, 2018

It seems that the automatic upload to Maven Central did not work during releasing. I'll analyze it and upload the artifacts manually to Maven Central tonight, if necessary.

@pmwmedia

This comment has been minimized.

Owner

pmwmedia commented Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment