hex the bytes value before smart text in bytes fields

[gal432]

[gal432]

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

[codecov-io]

Codecov Report

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

@@            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%)	:arrow_down:

---

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]

@jjkester Hi, can you look on this one?

[jjkester]

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]

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]

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]

@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]

Take your time. I’m just going through the PRs to try and make sense of them, and hopefully clear out the list.Sent from my iPhoneOn Dec 15, 2022, at 7:49 PM, Hasan Ramezani @.***> wrote: @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.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>