Flurl icon indicating copy to clipboard operation
Flurl copied to clipboard

Support [DataContract] (or custom attribute(s)) with UrlEncodedSerializer and possibly query params

Open ZombiesWithCoffee opened this issue 6 years ago • 7 comments

My class uses the JsonProperty/DataMember attributes to rename the data during post calls.

public class TokenTest
{
    [Required]
    [DataMember(Name="key")]
    public string KeyData { get; set; }
    }

My calls to 'PostUrlEncodedAsync' were failing, because the class name 'KeyData' was being used instead of the desired 'key' field name

Upon digging, I realized that UrlEncodedSerializer ignores the JsonProperty/DataMember attributes.

        var request = new FlurlRequest(url);
        var urlEncodedContent = new CapturedUrlEncodedContent(request.Settings.UrlEncodedSerializer.Serialize(token));
        var jsonSerialized = new CapturedUrlEncodedContent(request.Settings.JsonSerializer.Serialize(token));

The results show that JsonSerializer uses the attribute names, but UrlEncodedSerializer does not.

      urlEncodedContent = "KeyData=ewr1-QuQ5jo88WfC"
      jsonSerializedContent = "{"key":"ewr1-

Is this intentional/by design?

ZombiesWithCoffee avatar Jan 31 '19 17:01 ZombiesWithCoffee

Answered here: https://stackoverflow.com/questions/54462684/flurls-posturlencodedasync-is-ignoring-jsonproperty-names/54466972

tmenier avatar Feb 01 '19 16:02 tmenier

:) That was for JsonProperty. This is 'DataMember'.

ZombiesWithCoffee avatar Feb 01 '19 16:02 ZombiesWithCoffee

Ah, ok. That was easy to miss from your title. ;)

I think you'd agree this is a feature request and not a bug? DataMemberAttribute is part of .NET's serialization framework that dates way back to their XML-based DataContractSerializer. I don't think it's reasonable to expect it to work with Flurl, but it's not an unreasonable request either, especially given Json.NET's decision to support it.

There's a couple finer points to consider about the semantics though:

  1. The class needs to be marked with [DataContract], otherwise the serializer is supposed to ignore the [DataMember]s.

  2. If the class is marked with [DataContract], all properties you want serialized must be marked with [DataMember], or the serializer should not serialize them.

Not to mention, DataMemberAttribute has other properties, and the DataContract framework in general has other features/semantics that I'd need to make sure are fully covered, otherwise I'm exposing Flurl to legit bugs due to unexpected (non)behavior. I'm not sure if I want to go down that rabbit hole to support something so simple.

Another option is to create a couple custom attributes. That way I'm in control of what features are supported and how they work, and if they work as documented then they're not bugs. :)

tmenier avatar Feb 02 '19 15:02 tmenier

You're absolutely right, it's a feature request. It just seemed like a bug since I normally have no issues at all with the naming of the properties until I hit the 'recurvy' API that required 20 parameters as URL parameters.

And yes, the [DataContract] is required for the class for this to work. I'm quite used to using it with Newtonsoft requiring these conditions.

I think the best approach to maintain backwards compatibility is to have a serializer class within Flurl that can be enabled via configuration, or by setting `Settings.UrlEncodedSerializer' equal to an instance of that class.

ZombiesWithCoffee avatar Feb 03 '19 12:02 ZombiesWithCoffee

I'm thinking if it's supported with the default UrlEncodedSerializer, it should probably be supported here too:

url.SetQueryParams(object)

These technically live in different libraries (Flurl and Flurl.Http), so it would probably be smart to keep the logic in Flurl where it can be used by both. Mostly just a note to self.

tmenier avatar Feb 03 '19 16:02 tmenier

I'm actively gathering feedback to help prioritize issues for 3.0. If this one is important to you, please vote for it here!

tmenier avatar Apr 06 '19 14:04 tmenier

Made a custom serializer which supports the DataContract and DataMember attributes - at least the Name property: https://gist.github.com/MikaelElkiaer/52851beee9cefaa31339ebfcdfe1730c

@tmenier If I were to implement this (including the other properties of DataMember), would you prefer this as a separate nuget that I maintain, or via a merge request to include it in Flurl.Http?

MikaelElkiaer avatar Aug 07 '19 06:08 MikaelElkiaer