Skip to content

Don't try to JSON serialize binary data - #91

Merged
grahamgilbert merged 1 commit into
salopensource:mainfrom
korylprince:main
Mar 2, 2021
Merged

grahamgilbert merged 1 commit into
salopensource:mainfrom
korylprince:main

Conversation

@korylprince

Copy link
Copy Markdown
Contributor

Profiles (plist) allow binary data, but JSON does not. An example of this in the wild is using a profile to set the logo for NoMAD Login.

Interestingly, this issue is masked, at least in my case, with a ValueError: Circular reference detected. Only after digging into the code did I discover the issue was actually with trying to JSON encode binary data. Fixing this issue also fixed the Circular reference error (for me), so the issues with PR #90 may be solved with this instead.

This PR just replaces the binary data with the literal "BINARY DATA" using the existing serializer.

@sheagcraig

Copy link
Copy Markdown
Contributor

I like this; awesome detective work.

I think we have another option too, which would be easy to implement. I'm not sure one is a clear winner.
If we base64 encode the binary data (and then decode the bytes to str) we could have the actual binary data available. Not sure this is useful, but it's an option.

@korylprince

Copy link
Copy Markdown
Contributor Author

I thought about that, and I can see it from both sides. Having the data might be useful if say you wanted to check that the data matches an expected value. But including it could possibly make the request quite a bit bigger, and would cause the database to grow.

I would think that if someone really wanted that binary data to do something with, they could write an additional plugin to specifically fetch it.

I'm happy to do a PR with either method, just let me know which way you want to go.

@kbotnen

kbotnen commented Nov 25, 2020

Copy link
Copy Markdown

I had the exact same problem as you with: ValueError: Circular reference detected , I built a new version which included this fix and now my machine finally start ship the reports to sal. Hope it will be merged as soon as possible so I can use the sal-scripts through autopkg instead of my homebuilt package.

Anyway: Great work of you on an already great product 👍

@grahamgilbert
grahamgilbert merged commit 25e0b9d into salopensource:main Mar 2, 2021
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.

4 participants