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

Handle JSON trimming better #118

Open
masojus opened this issue Aug 3, 2017 · 0 comments
Open

Handle JSON trimming better #118

masojus opened this issue Aug 3, 2017 · 0 comments

Comments

@masojus
Copy link
Contributor

masojus commented Aug 3, 2017

We should trim in some places when going over the wire, and based on a flag or in DEBUG or while running tests or some combination, pretty-print. We should also not compare against raw strings in tests, but rather build up a JSON object and stringify before comparing--again with or without pretty-printing based on a flag or the config or something. There are also potential issues with URL query strings in terms of format and length.

Most recently embedding "\r\n" in tests caused failures on non-Windows platforms.

Here are my notes from a few months back:

	- The fact that we don't trim out JSON strings makes for garbage URLs when performing queries:
		? Here's a sample query string with a not_contains filter: "?event_collection=testCollection&timeframe=this_month&filters=%5B%0D%0A%20%20%7B%0D%0A%20%20%20%20%22property_name%22%3A%20%22propertyName%22%2C%0D%0A%20%20%20%20%22operator%22%3A%20%22not_contains%22%2C%0D%0A%20%20%20%20%22property_value%22%3A%20%22four%22%0D%0A%20%20%7D%0D%0A%5D"  
		? Which comes from this: 
		{[filters, [
		  {
		    "property_name": "propertyName",
		    "operator": "not_contains",
		    "property_value": "four"
		  }
		]]}
		? Most of it is "\r\n" and whitespace, which wastes precious characters in the URL and also looks gross and uses unnecessary bits of bandwidth.
		? Look at parms.Add(KeenConstants.QueryParmFilters, filters == null ? "" : JArray.FromObject(filters).ToString());
		? We should instead call .ToString(Newtonsoft.Json.Formatting.None)
		? For that matter, does the way we format lists of filters actually even work? Seems to, but instead of doing &filter=…&filter=…, we use EscapeDataString(..the Jarray toString() representation…) which ends up being &filters=%5B….%5D  which is the encoding for […] so it's literally escaping the JSON string for the array. What does curl do? It seems like python requests.get() (according to their docs) should do something different. So does the back end just do something smart and detect either format? Or is one of them wrong?
		? Actually, the Python Requests docs show that if you pass "params" as an object, a list gets turned into multiple query params with the same key. However, Keen's SDK, in get_params() in client.py, turns a group_by of type "list" into a string by using json.dumps(), which seems to indicate that then that string will get url-encoded the way the .NET SDK does by using EscapeDataString() for filters…maybe. So should the same be done when turning group_by into a list?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant