Add Regex Functionality

destructurama / attributed

Use attributes to control how complex types are logged to Serilog.

Apache License 2.0

263 stars 32 forks source link

Add Regex Functionality #36

Closed ydemetriades closed 4 years ago

ydemetriades commented 4 years ago

Introduced LogRegexAttribute

This functionality now allows you to define and apply regex operations.

[x] Add UnitTests
[x] Update README

ydemetriades commented 4 years ago

Can this PR be merged, or more reviewers are required?

ydemetriades commented 4 years ago

@nblumhardt can you please review this PR? Thanks!

nblumhardt commented 4 years ago

Thanks for the PR! Couple of questions-

What scenario are you hitting where this is needed? Would be great to have some supporting use cases so that we can evaluate how the API should work and where it fits into the bigger picture.
Would it make sense to integrate a "regex" flag or pattern into [LogMasked]? Seems like this is closely related to that feature, so either integrating regex in there, or modelling the API of this one on the existing feature might help keep the use experience cohesive

ydemetriades commented 4 years ago

Good questions!

Imagine a scenario where you have a Session string which consists of some public and some sensitive information and separated by a character (eg. | ). Let's take an example like this: Public1|Sensitive2|Public3. So, I would like to log the 2 public ones but not the sensitive one.
I thought about integrating the regex functionality into LogMaskedAttribute but that would add properties and logic in that attribute. I think it's better to keep LogMaskedAttribute as simple as possible and introduce the LogRegexAttribute which has a single purpose!

ydemetriades commented 4 years ago

@nblumhardt bump

nblumhardt commented 4 years ago

Thanks for the nudge, @ydemetriades - super busy week!

I like the feature; I am still processing where this fits in; I think the naming is probably the main thing - wondering if we can come up with something that's a verb phrase like the other attributes (e.g. NotLogged -> "not logged", LogMasked -> "log masked") - maybe LogReplaced or LogReplacement?

Or perhaps we could tie it to LogMasked with a name like LogMaskedPattern? Still thinking :-)

I also wonder whether the default pattern/replacement are useful, since this would essentially be covered by LogMasked. Perhaps requiring the pattern and replacement as constructor parameters would resolve this? E.g.:

[LogReplacement("secret(thing)", "***$1")]

ydemetriades commented 4 years ago

Hey @nblumhardt! Good points!

Read your comment! Totally agree with you! I think LogReplaced fits better with the rest attributes

Also, yes default values are unnecessary! So we can force them with constructor parameters! So should I proceed to the necessary changes with this one?

[LogReplaced("secret(thing)", "***$1")]

nblumhardt commented 4 years ago

Sounds good! 👍

ydemetriades commented 4 years ago

I have changed the Attribute as we have discussed! Let's review it again! 😀

ydemetriades commented 4 years ago

@nblumhardt I have pushed your PR suggestions! 😁

nblumhardt commented 4 years ago

Great, thanks!

ydemetriades commented 4 years ago

Thank you @nblumhardt! And @sungam3r as well!