keenlabs / keen-sdk-net

A .NET SDK for the Keen IO API
MIT License
37 stars 24 forks source link

Funnel queries drop timeframes on FunnelSteps #50

Closed masojus closed 7 years ago

masojus commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Fixed in commit 72c30a0.