Skip to content
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

LogMaskedAttribute to handle int,long and guid values #126

Merged
merged 7 commits into from
Dec 15, 2024

Conversation

andreas-moneygate
Copy link
Contributor

No description provided.

@github-actions github-actions bot added documentation Improvements or additions to documentation tests Pull request that adds new or changes existing tests labels Dec 9, 2024
@sungam3r
Copy link
Member

Please see #122 (comment) . What do you think?

@andreas-moneygate
Copy link
Contributor Author

Please see #122 (comment) . What do you think?

How about having a property in LogMaskedAttribute allowing other types besides string to be masked?

@sungam3r
Copy link
Member

I thought again about it. All non-string properties marked with this attribute ended in default case with ScalarValue.Null. So my reasoning from #122 (comment) is wrong. If now anyone marks int property with that attribute then it effectively becames null.

@sungam3r
Copy link
Member

I added LogMaskedAttribute_Nullify_Bool_Property test to verify. Please resolve conflicts and I'll review again.

@sungam3r
Copy link
Member

Also you may include changes from #122.

@sungam3r sungam3r mentioned this pull request Dec 12, 2024
@andreas-moneygate andreas-moneygate changed the title LogMaskedAttribute to handle int and long values LogMaskedAttribute to handle int,long and guid values Dec 13, 2024
@andreas-moneygate
Copy link
Contributor Author

@sungam3r
Conflicts resolved

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@sungam3r sungam3r added enhancement New feature or request and removed documentation Improvements or additions to documentation labels Dec 15, 2024
@sungam3r sungam3r merged commit 9c7ee1d into destructurama:master Dec 15, 2024
8 checks passed
Copy link

codecov bot commented Dec 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (88a8732) to head (3f95dbc).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #126   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        11           
  Lines          239       242    +3     
  Branches        36        36           
=========================================
+ Hits           239       242    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mickeyperlstein
Copy link

Although GUID's are not strings, we treat them as such, and do sometimes put in them secure data, not PII but machine II for sure.

@mickeyperlstein
Copy link

I also did not appreciate that it took you so long to merge my solution, and that you changed it so it's not my solution is even less appreciative.

It was MY idea, it was MY solution - and yet I get no credit in your product, last time im adding source to your code

@sungam3r
Copy link
Member

@sungam3r
Copy link
Member

Sorry, it was not intentional, I included you in release notes.

@sungam3r
Copy link
Member

With #141 you can mask property of any type, no more need to add custom cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tests Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants