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

hex the bytes value before smart text in bytes fields #195

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

hex the bytes value before smart text in bytes fields #195

wants to merge 4 commits into from

Conversation

gal432
Copy link

@gal432 gal432 commented Nov 21, 2018

No description provided.

@gal432
Copy link
Author

gal432 commented Nov 21, 2018

I saw just now that its so broken in python2 so I decline the PR for now

@gal432 gal432 closed this Nov 21, 2018
@gal432 gal432 reopened this Nov 21, 2018
@codecov-io
Copy link

codecov-io commented Nov 21, 2018

Codecov Report

Merging #195 into master will decrease coverage by 0.03%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
- Coverage   83.43%   83.39%   -0.04%     
==========================================
  Files          19       19              
  Lines         513      518       +5     
==========================================
+ Hits          428      432       +4     
- Misses         85       86       +1
Impacted Files Coverage Δ
src/auditlog/diff.py 86.3% <83.33%> (-0.47%) ⬇️

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 a22978e...902d007. Read the comment docs.

@gal432
Copy link
Author

gal432 commented Nov 29, 2018

@jjkester Hi, can you look on this one?

@jjkester
Copy link
Collaborator

In which situation is this fix necessary? It seems like this adds support for having byte-typed field names, but that seems odd to me, maybe you can elaborate a bit.

@gal432
Copy link
Author

gal432 commented Dec 27, 2018

In case of binary fields, the audit crash on python3.
I have some binary fields that controlled from textual fields, and I want to audit those binary changes with the textual changes.

@aqeelat
Copy link
Member

aqeelat commented Dec 15, 2022

I think there's a valid use case for comparing the hex of the fields. Because:

  • The binary fields get converted to strings, which means that they might not be displayed correctly on the admin site (we're not using safe_str).
  • Using safe_str for user input is a bad idea.

However:

  1. I think this should be an opt-in feature.
  2. I don't think writing the hex to the DB is the best idea. I'm in favor of converting them in runtime. However, since we're storing them as strings, there's no way of knowing if we should hex them or not. To get around this, we need to store the BinaryField data as bytes instead of calling smart_str on them.

Thoughts, @hramezani?

@hramezani
Copy link
Member

@aqeelat Thanks for checking and providing information and clarity here.
I don't have any answer ready for now. Need to investigate it more. Probably later.
It would be great to have some test here.

@aqeelat
Copy link
Member

aqeelat commented Dec 15, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants