-
Notifications
You must be signed in to change notification settings - Fork 125
Reject Empty path values #258
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
Conversation
Note: I also changed a few string == null checks to call string.IsNullOrEmpty() so that empty strings also throw (as I think those are error cases as well). Fixes serilog#257.
Looks like the only places where the issue is is FileSink and SharedFileSink.
On Merge please squash this to a single commit that matches the new name for this PR. |
Thanks for the PR!
|
Alright, will do. |
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.
Might have to update tests a little after this change.
Hi @AraHaan I'm doing some spring cleaning here If you're intending to resolve the issues and take this PR to it's conclusion, it'd be good to get things resolved sooner than later If you don't have capacity to do that in the short to medium term, can I request that this gets closed and then revisited from first principles In general my review would align what's gone so far - having |
I will look into this later on either today or tomorrow in order to get this resolved as soon as possible. |
Sorry about the late response, will get ready for some progress later on today. |
Sorry for being late again, something came up and I had to do that before making the changes here. Although I feel that output template check code could be optimized further to something like this: outputTemplate ??= string.Empty; Because it is more optimized then an Documentation on the |
@@ -337,7 +337,7 @@ public static class FileLoggerConfigurationExtensions | |||
{ | |||
if (sinkConfiguration == null) throw new ArgumentNullException(nameof(sinkConfiguration)); | |||
if (formatter == null) throw new ArgumentNullException(nameof(formatter)); | |||
if (path == null) throw new ArgumentNullException(nameof(path)); | |||
if (string.IsNullOrEmpty(path)) throw new ArgumentException(nameof(path)); |
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.
this (and equivalents) invalidates the comment (and equivalents):
<exception cref="ArgumentNullException">When <paramref name="path"/> is <code>null</code>
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.
Alright, will fix the comments as soon as possible.
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.
I personally think it would be useful for the existing check and comment to remain as-is for clarity
That way, there's no ambiguity as to what can cause the ArgumentException
(I also wonder if there should be a message indicating that the constraint is that Empty is not permitted and/or the xmldoc should mention it? Right now you're handed an exception and the only reasonable way to figure out the constraint is to read the source?)
That's absolutely not a problem - the most important thing is that the code and user experience remains as boring as possible. Please take as much time as you want or need; nobody is tied to any date. (The reason I asked for the state is to simply figure out whether it was on the road to being abandoned, or the road to completion)
I'm not understanding your reasoning. The current code:
Is clear and unambiguous - you MUST supply an output template. The point is not about efficiency, but clarity. Efficiency has nothing to do with it - we are checking in case something went wrong such as not having null checks turned on. Silently replacing a critical input with one that makes it silently write nothing would be very suprising and annoying to my mind. kinda like Am I missing something? |
Sorry to be a pain, but following up on #258 (comment)
Can you explain in short (ideally by editing it into the overview in a clear way for everyone) and/or an extended reason down here why this check is required? i.e. while a path of
In general, the main concern for me is |
It was originally about fixing that issue, but then I realized a simple misconfiguration of the logging config file resulted in the path not properly being used then closed that issue, but left this pr up as I also noticed that rejecting empty path values can be made as inputting an empty string can result in an exception when it tries to use file I/O to write the logs (it would actually try to write to a file with no name and no file extension and fail) the last time I had that issue which was about 1 year ago now, after that I started to do clever ways of defining the log file name by hard coding it in the program. And it was something that the null check missed. |
Sounds like things are stirring around here... |
Closing this one as stale, but thanks for taking the time to check this out 👍 |
Fixes #257.