Flurl icon indicating copy to clipboard operation
Flurl copied to clipboard

SetQueryParams doesn't know about JToken

Open EynsherKiel opened this issue 6 years ago • 6 comments

Please, fix that )

using Flurl;
using Newtonsoft.Json.Linq;
using System;

namespace FlurlTest
{
   class Program
   {
       static void Main(string[] args)
       {
           var obj = new { Test = "test" };
           Console.WriteLine("https://soundcloud.com/discover".SetQueryParams(obj)); // https://soundcloud.com/discover?Test=test
           Console.WriteLine("https://soundcloud.com/discover".SetQueryParams(JToken.FromObject(obj))); // https://soundcloud.com/discover
       }
   }
}

EynsherKiel avatar Sep 16 '19 13:09 EynsherKiel

Unlike Flurl.Http, Flurl the URL builder has no knowledge of Json.NET objects. JSON is rarely relevant to URL building, and I'm not sure why you are using JTokens here. Could you use a plain C# object with that property name instead? Or a dictionary? If you must use a JToken, you could cast it to a JObject and this will work:

SetQueryParams(jobj.ToObject<ExpandoObject>())

tmenier avatar Sep 16 '19 14:09 tmenier

So, you think that don't possibilities for work with jtoken for creating uri?

I'm using jtokens how nodes, my top level object it's just object type )

EynsherKiel avatar Sep 17 '19 08:09 EynsherKiel

Flurl is dependency-free. JToken is defined in a 3rd-party library. You shouldn't expect Flurl to know what a JToken is. So this is not a bug.

However, SetQueryParams does its best to determine if the thing you passed it is dictionary-like. I stepped through this case and found that it did parse the JObject into key/value pairs. However, if the value side looks like a collection, it's going to try and break that out, i.e. key=value1&key=value2&key=value3. In this case the value side is a JToken, which implements IEnumerable, so to Flurl it looks like a collection. In the case of a simple type like a string or number, that collection is empty. So that's why it ends up returning nothing.

I still don't want to take a dependency on Json.NET, but considering the popularity of that library, I will consider modifying that algorithm to handle this case. Even if it's a little bit of a hack (like reflecting on the type name), I think that's better than a breaking change or taking the dependency, so I will likely do this.

It's low on the priority list to be honest though, so I still recommend using JObject.ToObject<ExpandoObject>() to get you past this for now.

tmenier avatar Sep 20 '19 18:09 tmenier

I really don't know why Flurl should have to know how to serialize complex objects as a query string parameter. That sounds like something the caller should do first.

rcollette avatar Sep 21 '19 03:09 rcollette

@rcollette I agree that it shouldn't "have" to, but it's so close to being able to parse a JObject already (because it implements IDictionary) that I see no harm in getting it over the hump. I don't know the exact use case here, but having key-value pairs serialized as JSON that you want to transfer to a query string doesn't seem like a far-fetched scenario to me, and Json.NET is the most obvious tool to use as the go-between. So I think having JObject "just work" the way a POCO or dictionary does would be a nice little enhancement.

I won't open the floodgates to support a bunch of other 3rd-party types - JObject might be the only one on the planet I'm willing to make special accommodations for. :)

tmenier avatar Sep 21 '19 14:09 tmenier

I was also used System.Linq.Dynamic.Core, and that thing know how work with jtoken. And brokens when work with ExpandoObject ))) And with object ) But with jtoken ok )))

EynsherKiel avatar Sep 30 '19 13:09 EynsherKiel

Sorry for letting this slide for so long. In the 4 years since this was logged, my backlog has only grown and Json.NET has only gotten less ubiquitous, making me less inclined to address it. There's some pretty obvious manual workarounds so hopefully you moved on long ago.

tmenier avatar Sep 19 '23 22:09 tmenier