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

Shortening a dict should not change its type #200

Merged
merged 2 commits into from
Oct 12, 2017
Merged

Shortening a dict should not change its type #200

merged 2 commits into from
Oct 12, 2017

Conversation

rokob
Copy link
Contributor

@rokob rokob commented Oct 3, 2017

Fixes #197

#176 added request.POST to the keys that we might shorten. It turns out that we shorten things basically by turning them into a string representation of themselves with an ellipses. This is problematic as it pertains to types. A string "{'a': 1, ...}" is not a valid JSON object even if it is reified. The problem with shortening dictionaries that they are unordered, so we have to make some arbitrary choice about what to leave in versus out. In this PR I chose to use obj.items()[:max_items] as the selection of the maximum number of keys. The current implementation uses whatever reprlib decides to do. In this way, I construct a new dict by pulling the key/value pairs from the old object.

Problems with this are that the result is not obviously truncated. We could add something like {"truncated": true} to the object or {"...": "..."} to make it obvious. The other alternative is to not truncate request.POST. Any other place that we might truncate a dictionary is probably fine to turn it into a string but the API will reject an item with a string value at request.POST.

@chris-erickson
Copy link

ETA on getting this merged and rolled out?

@brianr brianr changed the title Shortening a dict should not change it's type Shortening a dict should not change its type Oct 12, 2017
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.

2 participants