AddJsonBody handles strings in a usually user-friendly but incorrect way

[MattiasMartens]

Describe the bug

When calling AddJsonBody() with a string, what is added is the literal string supplied without serialization. This makes it impossible to handle the case where the value is a JSON object whose full and complete structure is "String".

To Reproduce

The RestSharp code involved is trivial, see the body of AddJsonBody():

        request.RequestFormat = DataFormat.Json;
        return obj is string str ? request.AddStringBody(str, DataFormat.Json) : request.AddParameter(new JsonParameter("", obj, contentType));

In our case, the API we're hitting has this type stanza:

"requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "type": "string"
              }
            }
          }
        },

Importantly, the backend expects this input to be the serialized version of a JSON object (I know, I know). So to call it properly, naively we expected to be able to do:

  request.AddJsonBody(JsonConvert.SerializeObject(myObject))

But of course this does not work -- it hits the first part of the ternary above and treats it as "already serialized", sending the string as-is. This causes it to be parsed as an object by the middleware, not as a string as intended.

Instead, this is currently what we have to do:

  request.AddJsonBody(JsonConvert.SerializeObject(JsonConvert.SerializeObject(myObject)))

(Note that in the real world it's not as simple as this -- we are calling RestSharp through a generated client, so we don't have the option of calling a different more appropriate method even if there is one.)

Expected behavior

There should be a way to add a top-level string as JSON, serializing it as expected. Top-level strings are valid JSON!

I understand that the current implementation probably avoids some common gotchas, but it is important to handle this case as well, because there really are APIs in the wild that require JSON-formatted top-level strings as input. In our case, this issue required a lot of debugging to figure out.

[alexeyzimarev]

Yeah, what you want was the original behaviour. However, the number of people complaining about it was just overwhelming, and I had to add the type check. I don't like it as people like you adding a string instance as JSON body apparently get it serialised wrong. I am considering to revert it to what was there before in (maybe) the next major release.

[MattiasMartens]

Thanks for your reply @alexeyzimarev; to be honest my main goal here is just to document the issue so I have something to point to to explain the weird code that has to be written on our side.

The decision is yours to make of course, but I would also be pretty happy with a method like AddJsonBodyLiteral() that can be used by auto-generated code.

[alexeyzimarev]

Right now, Postman codegen asked me to review their changes where they will use AddStringBody. So, the need for AddJsonBody to distinguish between string or object would disappear. Still, the same issue is there with Swagger, but I am not sure what to do about it.

[alexeyzimarev]

I think you can work around the issue by calling request.AddParameter(new JsonParameter("", obj)). The JsonParameter constructor doesn't do anything with the passed object, so you can pass a string there and it will be properly serialised.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[repo-ranger]

⚠️ This issue has been marked wontfix and will be closed in 3 days

[alexeyzimarev]

@MattiasMartens do you think this will work?

https://github.com/restsharp/RestSharp/pull/2043/files#diff-fd5240c0eeb1a04bc0c9ea9b5210c8a8ce6703011459c72d49ee4b0c08e61224

[MattiasMartens]

@MattiasMartens do you think this will work?

https://github.com/restsharp/RestSharp/pull/2043/files#diff-fd5240c0eeb1a04bc0c9ea9b5210c8a8ce6703011459c72d49ee4b0c08e61224

No, I don’t think that would work because the type of value we might be sending over the wire is something like "{ \"myKey\": \"myValue\" }" with an expectation this would be serialized as "\"{ \\\"myKey\\\": \\\"myValue\\\" }\"".

The distinction unfortunately is not about whether the string "looks like" JSON, but an invisible business decision about whether the JSON should be serialized or considered already serialized. One or the other might be appropriate depending on the case. (I don’t think it requires explanation why an API specifying that it takes a string that’s implicitly supposed to represent serialized JSON is a bad idea; unfortunately that side of things is outside of our control.)

The change you propose would help with a different issue where someone wants to put e.g. "Hello world" in a JSON field (at the moment this would be a parse error for sure, right?). My two cents is, I don’t think this change would be a good idea. It makes the behaviour more opaque to fix a relatively rare problem while opening the door to even worse, even rarer bugs.

On the other hand it is good to know that I can ensure literal interpretation of the parameter by calling:

request.AddParameter(new JsonParameter("", obj))

Our code was generated by some library that consumes OpenAPI code and produces a spec. That library knows exactly what the type should be, so the helpful conversion feature of AddJsonBody() only causes bugs. That library should use request.AddParameter(new JsonParameter("", obj)) instead of request.AddJsonBody(...), but apart from that, I see no fix here.

I will close this issue. Thank you for your help!

[alexeyzimarev]

Ok, you are right. I wanted to create an unobtrusive overload with a string argument but the generic constraint class is not about "classes", it accepts all reference types and I got an ambiguous signature even though it compiles.

As a compromise, I added this:

RestRequest AddJsonBody(this RestRequest request, string jsonString, bool forceSerialize, ContentType? contentType = null)

I had to put forceSerialize before content type to prevent ambiguity of method signatures. If you set it to true, the string will be serialised.