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

Funnel queries drop timeframes on FunnelSteps #50

Closed
masojus opened this issue Apr 9, 2017 · 3 comments
Closed

Funnel queries drop timeframes on FunnelSteps #50

masojus opened this issue Apr 9, 2017 · 3 comments

Comments

@masojus
Copy link
Contributor

masojus commented Apr 9, 2017

Most places where we use timeframe parameters, (which are now required, and so should no longer be optional in the SDK interface going forward) we handle JSON-ifying the timeframe by doing something like this:

parms.Add(KeenConstants.QueryParmTimeframe, timeframe.ToSafeString());

However, when we take an IEnumberable<FunnelStep> and try to serialize it, we do this:

var jObs = steps.Select(i => JObject.FromObject(i));

...which results in the FunnelStep.Timeframe property being written as {}.

We should either customize JSON formatting for the Timeframe property, or for all QueryTimeframe instances across the board, and therefore not need to call ToString() everywhere we encounter one, or at least here in Queries.Funnel() manually write the JSON for each FunnelStep.

@masojus
Copy link
Contributor Author

masojus commented Apr 17, 2017

Note that QueryAbsoluteTimeframe probably does work, since the start and end properties have JsonProperty attributes, so it's likely only QueryRelativeTimeframe instances that get dropped because they have no properties Json.NET knows about.

@masojus
Copy link
Contributor Author

masojus commented Apr 17, 2017

Actually, yes, writing QueryAbsoluteTimeframe to the request works fine, and adding a custom write-only converter is easy enough to write out the list of FunnelSteps as a JSON-encoded URL complete with QueryRelativeTimeframes written out--that's a quick fix to get requests working.

However, the problem runs deeper, because as it turns out we don't properly parse timeframes when deserializing to FunnelResultStep. Since the base QueryTimeframe is a class with a default constructor, JSON just creates a blank instance of the base class when it comes across each step's timeframe property in the result. For absolute timeframes, it has no knowledge of the structure of the nested object, and since QueryTimeframe has no JsonProperty attributes (or any properties at all, actually) it ignores the start and end properties. However, once we are properly writing out relative timeframes, then we get back something like "timeframe": "previous_30_days", and JSON's default deserialization protocol is to try to find a conversion from string to QueryTimeframe or maybe a ctor that takes string or something along those lines, which it can't, so we get an error like this:

{"Error converting value \"previous_30_days\" to type 'Keen.Core.Query.QueryTimeframe'. Path 'steps[0].timeframe', line 1, position 109."}

So, we should probably make it IQueryTimeframe and create a custom JsonConverter that can detect the structure of the timeframe property and deserialize to the correct concrete type.

masojus added a commit that referenced this issue Apr 17, 2017
- Change the base class `QueryTimeframe` to interface `IQueryTimeframe`.
- Add a `TimeframeConverter` to help with parsing JSON to the appropriate concrete implementation of `IQueryTimeframe`.
- Use TimeframeConverter` to also help write `QueryRelativeTimeframe` to JSON.
- We are using this new converter to create a URL-encoded JSON array to stuff in the query params for a funnel query, so we no longer drop relative timeframes.
- Now we actually parse the timeframe in the `FunnelResultStep`, whether absolute or relative.
@masojus
Copy link
Contributor Author

masojus commented Jun 8, 2017

Fixed in commit 72c30a0.

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