NancyFx / Nancy

Lightweight, low-ceremony, framework for building HTTP based services on .Net and Mono
http://nancyfx.org
MIT License
7.15k stars 1.47k forks source link

JSON request body cannot be bound to a string primitive variable #2807

Open svdHero opened 6 years ago

svdHero commented 6 years ago

Prerequisites

Description

I have the following Nancy module.

    public class MyFooTestModule : NancyModule
    {
        public MyFooTestModule()
        {
            Post["/text"] = _ =>
            {
                //string myNewText = this.Bind<string>(); // Fails, because string has no parameterless constructor
                string myNewText = Request.Body.AsString();

                return Response.AsJson(myNewText);
            };
        }
    }

where I want to bind to a single string variable. However, I cannot use this.Bind<string>() as I would for example use this.Bind<int>(), because during runtime I get an exception telling me:

System.MissingMethodException: No parameterless constructor defined for this object.

If I use Request.Body.AsString() instead, the character escaping is not working properly and I end up with multiple quotes and escaped backslashes in my variable.

More precisely, when I post the JavaScript string

"Hello\r\nWorld Foo!"

from the client, I receive the echo JavaScript string

"\"Hello\\r\\nWorld Foo!\""

from the server.

What is the correct why to bind the JSON request body to a primitive string variable? Does that work at all? I do not want to wrap every primitive type into an otherwise useless container class just to exchange string variables used in my JavaScript client.

I've already asked this question on Stackoverflow, but nobody answered.

Steps to Reproduce

Take above code sample and run it.

System Configuration

cloudhunter89 commented 6 years ago

This is the result of a C# design decision, combined with common practice for serialization within the framework. It doesn't with because the string to is immutable - an instance cannot be changed once created. The default implementation of serialization attempts to create an instance and assign it a new value. @khellang, is it possible to specialize serialization for individual types? If that isn't an option, an alternative would be to pass the value as a parameter Post[/text/{userString:string}] = parameter => { // access the value here with something like string theString = parameter.userString }

Off the top of my head, most other primitives will work, only string is immutable, though there are a several built-in types that won't work such as tuple and datetime.

On Oct 23, 2017 2:41 AM, "svdHero" notifications@github.com wrote:

I have the following Nancy module.

public class MyFooTestModule : NancyModule
{
    public MyFooTestModule()
    {
        Post["/text"] = _ =>
        {
            //string myNewText = this.Bind<string>(); // Fails, because string has no parameterless constructor
            string myNewText = Request.Body.AsString();

            return Response.AsJson(myNewText);
        };
    }
}

where I want to bind to a single string variable. However, I cannot use this.Bind() as I would for example use this.Bind(), because during runtime I get an exception telling me:

System.MissingMethodException: No parameterless constructor defined for this object.

If I use Request.Body.AsString() instead, the character escaping is not working properly and I end up with multiple quotes and escaped backslashes in my variable.

More precisely, when I post the JavaScript string

"Hello\r\nWorld Foo!"

from the client, I receive the echo JavaScript string

"\"Hello\r\nWorld Foo!\""

from the server.

What is the correct why to bind the JSON request body to a primitive string variable? Does that work at all? I do not want to wrap every primitive type into an otherwise useless container class just to exchange string variables used in my JavaScript client.

I've already asked this question on Stackoverflow, but nobody answered.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NancyFx/Nancy/issues/2807, or mute the thread https://github.com/notifications/unsubscribe-auth/AEEiljzPp8PNtNtWJXpXwvbBdsm12Tvmks5svFFPgaJpZM4QCe-U .

khellang commented 6 years ago

Is this using the built-in serializer? I think it should work. If you'd used Newtonsoft.Json, it would be the same as calling JsonConvert.Deserialize<string>(json), which should work. It might be the built-in serializer that doesn't support this scenario.

svdHero commented 6 years ago

Yes, I am using the built-in serializer. I am not using any third-party libraries, I just wanted to experience the super-duper-happy-path. :-/

khellang commented 6 years ago

I'm curious; what are you planning on doing with the string? It doesn't sound really useful to "bind" to a string. Also, you mention escaping; what are you expecting the "deserialization" process to do about this escaping? It should still be valid JSON, right?

svdHero commented 6 years ago

I just want to write a REST api for acessing information on a measurement device where some device parameters are strings. So I would like to access routes like /api/v1/meas-units/4711/config/description. So if I need to set "description", I want to send a post request to that route without having to wrap the description string in an object that wouldn't make sense semantically.

And I don't want the deserialization process to do anything fancy. I only was describing that the naive workaround without using this.Bind<string>() does not work. When I use binding, I merely expect that a valid JavaScript string posted to the server via JSON will correctly be translated to a valid C# string. The same way a valid JavaScript object posted to the server via JSON will correctly be translated to a valid C# object.

In particular, if the JavaScript string variable on the client contains the value "Hello\r\nWorld Foo!", exactly the same value should end up in my C# string variable when using model binding. When using the naive Request.Body.AsString() workaround that is not the case. That's all I was saying.

Does that make sense? I am happy to provide more information if needed. Could you successfully reproduce the exception by running my toy example above?

khellang commented 6 years ago

When using the naive Request.Body.AsString() workaround that is not the case.

What is the case? AsString doesn't do anything fancy, it just converts the body to a string (using the specified encoding, or UTF8), byte by byte. If you have trouble with the resulting string, it's probably because the input string is off :smile:

I merely expect that a valid JavaScript string posted to the server via JSON will correctly be translated to a valid C# string

That's why I asked what you wanted to happen to the string. What is a "valid C# string"? If we were to add support for Bind<string>, we'd probably just check if T == typeof(string) and return back the body as a string, just like you've already done. I'm not sure what else to do...

svdHero commented 6 years ago

Let me try to clarify further with the extended example below:

    public class Foo
    {
        public string Description { get; set; }
        public int Number { get; set; }
    }

    public class MyFooTestModule : NancyModule
    {
        public MyFooTestModule()
        {
            Post["/description"] = _ =>
            {
                //string description = this.Bind<string>(); // Fails, because string has no parameterless constructor
                string description = Request.Body.AsString(); // Naive workaround. Does not work as expected.

                if (!description.Equals("Hello\nWorld!"))
                    throw new Exception("Does not work as expected!"); // Will be thrown!
                else
                    return HttpStatusCode.OK;
            };

            Post["/foo"] = _ =>
            {
                Foo postedFoo = this.Bind<Foo>(); // Works as expected

                if (!postedFoo.Description.Equals("Hello\nWorld!"))
                    throw new Exception("Does not work as expected!"); // Will not be thrown.
                else
                    return HttpStatusCode.OK;
            };
        }
    }

However, if I post the JSON object

{
    "description": "Hello\nWorld!",
    "number": 4711
}

to the REST endpoint /foo, everything works as expected. The JSON object is converted into a C# object with the correct description property value.

If I post the JSON "object" (yes it's just a string)

"Hello\nWorld!"

to the REST endpoint /description (which is the same use case really), then my user-defined exception is thrown.

The reason is that the variable postedDescription has the value "\"Hello\\nWorld!\"" instead of "Hello\nWorld!" (like in the first case with Foo).

Do you understand what I mean now? The use case is the same: I want to turn JavaScript objects/variables into C# objects/variables that represent the same information. That does not work with a plain Request.Body.AsString(). There needs to be another "dejsonfy" step from the built-in (???) JSON deserializer.

khellang commented 6 years ago

Can you post the entire requests here? What's the Content-Type of the requests?

svdHero commented 6 years ago

The content-type is application/json and I use Postman to test it. I will post the entire requests the week after next week. I am away from keyboard for a while. Thank you very much for your help so far. Great support here. I will come back to you.

cloudhunter89 commented 6 years ago

After some experimentation, it seems that Response.AsJson on a string performs escaping on characters such as the backslash and quotes. If I hit an endpoint that echoes the request

Post["/echo"] _ =>
{
  Nancy.Response response = new Nancy.Response();
  string requestAsString = Nancy.Extensions.RequestStreamExtensions.AsString(Request.Body);
  return Response.AsJson(requestAsString);
}

I see extra escaping on anything that might be a special character however, if I use return Response.AsText(requestAsString); There is no additional escaping in the echo. Have you tried using the string that AsString returns for anything besides the json echo?

svdHero commented 6 years ago

I don't even use the json echo for checking the posted body anymore. Instead I use the Visual Studio Debugger and check the posted variable values directly by means of breakpoints once the request from Postman comes in. Also note my latest example above where an Exception is thrown, because the C# code asserts that the strings are not equal.

I edited the example slightly to highlight that I do not rely on the echo string. Just copy my example, run the code, issue an application/json post request from some client and check it yourself with Visual Studio Debugger. Can you confirm that an Exception is thrown, because the strings are not equal?

Like said, once I'm back in the office the week after next week I will post the full requests. Thanks for all the support and for your guys' effort to help me. I really appreciate that.

cloudhunter89 commented 6 years ago

So, the issue comes down the combination of the AsString method parsing the stream byte-for-byte, as was said earlier, and the HTTP body being the exact byte sequence that is given. There are no escape characters defined for an HTTP request body -- it contains exactly the bytes provided.

Using your example code, if I post (from Postman)

Hello
World!

It returns 200; however,

"Hello
 World!"

OR

"Hello\nWorld!"

OR

Hello\nWorld!

all throw the exception. The request body is the string as bytes, and there is no way to tell it to reinterpret "\n" as a linefeed, instead it is read and sent as the bytes 0x5c and 0x6E (or 0x005c and 0x006E in the case of big endian Unicode) rather than the byte 0x0B (0x000B).

khellang commented 6 years ago

As I mentioned, AsString just reads the bytes directly off the wire.

That's why I think something's mangling the request on the way to Nancy, and why I asked to see a dump of the entire request. Something is 🐟 here, and I don't think it's in any Nancy code 😉

This works perfectly fine:

var expected = "Hello\nWorld!";

var bytes = Encoding.UTF8.GetBytes(expected);

var stream = new MemoryStream(bytes);

var value = stream.AsString();

value.Equals(expected); // true
cloudhunter89 commented 6 years ago

@khellang I think the fishiness you smell is a misunderstanding about what is actually being sent when an http body contains the string "Hello\nWorld!" Nothing is being mangled, it is not being reinterpreted as I (being accustomed to escape sequences) would have initially expected. The string "\n" is parsed from the byte stream as the C# string "\n" which is \n not a newline because those are the bytes that were sent. The lovely confusion that is strings and what kind of strings and how those strings are interpreted. I'm feeling quite strung out...

svdHero commented 6 years ago

I am happy that you like my :fish: parfum and I agree that string interpretation is tricky. Hopefully I interpret the string escaping correctly here. I am not sure whether I used the proper vocabulary to explain myself, in particular when saying "does not work as expected". Sorry for the confusion.

Let me try again:

  1. I never said that Body.AsString is buggy. It works as expected and does exactly what I want it to do.
  2. This issue is about not being able to call Bind<string>.
  3. What I meant was that a mere Body.AsString() cannot be used as a workaround for Bind<string>(), because it does not do the same as Bind<string>() (if that was available).
  4. The reason for this is the content-type being json. So the raw body content returned by Body.AsString() obviously (and by definition) is not the same as the json content (e.g. because of the quotation marks that have to be removed).

Let me also post both some client code and yet another version of the server code to boil it further down by removing the newline from the equation. The quotation marks alone should already show the behaviour I am describing.

My example JavaScript code on client:

function postData(url, data){
    fetch(url,
    {
        headers: {
          'Accept': 'application/json',
          'Content-Type': 'application/json'
        },
        method: 'POST',
        body: JSON.stringify(data)
    })
    .then(function(res){console.log("From"+url+":\n"+res+"\n")})
    .catch(function(res){console.log("From"+url+":\n"+res+"\n")});
}

const foo = {
    description: "Hello World",
    number: 4711
};
postData('/api/foo', foo); // returns http status 200

const description = "Hello World";
postData('/api/description', description); // returns http status 500

My example C# code on server:

    public class Foo
    {
        public string Description { get; set; }
        public int Number { get; set; }
    }

    public class MyFooTestModule : NancyModule
    {
        public MyFooTestModule()
        {
            Post["/api/foo"] = _ =>
            {
                Foo postedFoo = this.Bind<Foo>(); // Works as expected

                if (!postedFoo.Description.Equals("Hello World!"))
                    return HttpStatusCode.InternalServerError;
                else
                    return HttpStatusCode.OK;
            };

            Post["/api/description"] = _ =>
            {
                //string description = this.Bind<string>(); // Fails, because string has no parameterless constructor
                string description = Request.Body.AsString(); // Naive workaround that does not work as Bind<string>() should.

                if (!description.Equals("Hello World!"))
                    return HttpStatusCode.InternalServerError;
                else
                    return HttpStatusCode.OK;
            };
        }
    }

Not having a Windows PC at hand right now, I cannot test this at the moment, but I am pretty certain, the first postData call returns an 200 while the second postData call returns a 500. Nevertheless, both examples clearly are of the same use case type, namely: "Please post this JavaScript data entity to the server".

So my question really is: Could you implement Bind<string> such that it behaves in an analogous manner to Bind<Foo>, please?

cloudhunter89 commented 6 years ago

Not while adhering to the JSON standard... The specification does not work well for representing a string. The basic structures of JSON are an object or an array. An object is a collection of key value pairs, while an array is a collection of values... so.... is a string a key-value pair or an array...?

It could be a single element array, but then how do you differentiate string and string[]?

It could be an array of its characters, but then most implementations would think you had an array of one character strings...

If you treat it as an object { key; value } what is the key?

The team or a contributor could pick some representation, but would likely end up with issues trying to interpret some other frameworks implementation of a JSON string valued object thing.

In my opinion, it would be faster, and probably more appropriate, for you to have a client method that performs a plain text http request. If you want a string send the plain text string, if you want an object or array, send a JSON string.

cloudhunter89 commented 6 years ago

Having said that... I was curious and decided to play around with things that don't really fit the JSON definition -- integers, floating point, characters -- and Bind<> works for all of them as invalid JSON strings i.e. a request with body 1 will work with Bind<int> etc... So I am probably just blowing smoke and should keep my opinions to myself in the future...

svdHero commented 6 years ago

Actually, I beg to differ. According to the Wikipedia article

JSON's basic data types are Number [...], String [...], Boolean [...], Array [...], Object [...], null [...],

So I expect a plain text string to be a valid JSON object and, thus, it makes (IMHO) perfect sense that Bind<> works for all basic types and should work for string, too.

When I send a request to my latest backend example code above, Postman logs the following requests: postman_log

Like I assumed, the first request succeeds while the second one produces a 500 internal server error.

cloudhunter89 commented 6 years ago

My mistake for reading an outdated specification, RFC 4627 ( http://www.ietf.org/rfc/rfc4627.txt):

A JSON text is a serialized object or array.
JSON-text = object / array

from RFC 7159 (https://tools.ietf.org/html/rfc7159):

   JSON Grammar
   A JSON text is a sequence of tokens.  The set of tokens includes six
   structural characters, strings, numbers, and three literal names.
   A JSON text is a serialized value.  Note that certain previous
   specifications of JSON constrained a JSON text to be an object or array

Standard aside, I recall stating that the behavior at current was inconsistent with my opinions and

therefore said opinions probably are not valid. I'm not an authority, simply a community member.

Sorry had quoted text open in the email.

svdHero commented 6 years ago

No worries. I appreciate any community member's opinion and support. Thank you again for your ongoing efforts in discussing the matter. Maybe @khellang has further comments on the latest insights?

svdHero commented 6 years ago

@khellang Do you have any thoughts on this considering the latest discussions?

svdHero commented 6 years ago

Anybody? I still think, Bind should be implemented, since - as pointed out - a JSON string is a valid JSON data type.

khellang commented 6 years ago

I don't really have any thoughts...

I don't think there has been any requests for this functionality previously, and there's tons of workarounds. I'm not even sure if I understand the problem properly. I've never used or seen raw strings as JSON in any API and as @cloudhunter89 suggested, I was certain that it wasn't valid JSON.

If someone's willing to make a pull request with accompanying unit tests, it might give a better basis for discussion?