New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unneeded json string escaping in JsonValueUtils #1181

Closed
maximpashuk opened this Issue Jun 6, 2018 · 9 comments

Comments

4 participants
@maximpashuk
Contributor

maximpashuk commented Jun 6, 2018

I am using Microsoft.AspNetCore.OData beta4, but my issue related to Microsoft.OData.Core.

I see differences in json response containing russian language symbols in normal odata query and batched odata query.
Batched odata response always escaping russian symbols while normal response has no escaping.

"country": "Россия" <-- no escaping in GET request

"country":"\u0420\u043e\u0441\u0441\u0438\u044f" <-- escaping in $batch request

I realize from the docs (OData JSON Format Version 4.0 and RFC4627) that escaping of non-ASCII symbols is not required.
Docs says just that "Any character MAY be escaped"

From source code i realized that $batch endpoint uses JsonValueUtils there ALL characters with code > 127 ALWAYS escaped.

I think forcing escaping is not optimal - 6 bytes (\uXXXX) on each character is growing json size, also deserializing logic is more complicated.
Also ODataOutputFormatter from ODATA WebApi has to forced escaping.

So please refactor JsonValueUtils to escape symbols only required to be escaped (", , etc.)

@xuzhg xuzhg added the P2 label Jun 6, 2018

@mikepizzo

This comment has been minimized.

Member

mikepizzo commented Jun 26, 2018

Can you clarify where the escaping is occurring? Is it just escaping the URL, or is it escaping properties within a (request? response?) payload?

I was able to write a simple test that produced the following output:

{
  "responses": [
    {
      "id": "1",
      "status": 200,
      "headers": {
        "odata-version": "4.0",
        "content-type": "application/json;odata.metadata=minimal;odata.streaming=true;IEEE754Compatible=false;charset=utf-8"
      },
      "body": {
        "@odata.context": "http://testservice/$metadata#customers",
        "value": [
          {
            "@odata.id": "customers/1",
            "id": "Россия"
          }
        ]
      }
    }
  ]
}
@maximpashuk

This comment has been minimized.

Contributor

maximpashuk commented Jun 26, 2018

Hi.
Escaping is occurred in properties in response payload when using /$batch odata endpoint.
Escaping not occurred when using simple GET/POST requests.

Batched responses has special format, different from format you just posted.

@mikepizzo

This comment has been minimized.

Member

mikepizzo commented Jun 26, 2018

I see.
From your example I thought that you were using the new JSON batch format.

Was able to repo for multipart/mixed format.

@igor-moskvitin

This comment has been minimized.

igor-moskvitin commented Jul 16, 2018

@mikepizzo can you provide some exaple of working json batch, please? (related with OData/WebApi#1532)

@AlanWong-MS AlanWong-MS added this to To do in OData project via automation Aug 16, 2018

@xuzhg

This comment has been minimized.

Member

xuzhg commented Aug 29, 2018

It can repro:

  1. Create a model like:
public class Customer
    {
        public int Id { get; set; }

        public string Country { get; set; }
    }
  1. Create a controller
public class CustomersController : ODataController
    {
        [EnableQuery]
        public IActionResult Get()
        {
            Customer c = new Customer
            {
                Id = 1,
                Country = "Россия"
            };
            return Ok(new[] { c });
        }
    }
  1. Add services:
services.AddOData();

…

app.UseODataBatching();
app.UseMvc(b =>
   {
         b.MapODataServiceRoute("odata", "odata", GetEdmModel(), new DefaultODataPathHandler(),
                ODataRoutingConventions.CreateDefault(),
                new DefaultODataBatchHandler()
    );
  });
  1. Send a post request to http://.../odata/$batch
{
    "requests": [
       {
         "id": "0d24a607-3fb0-4a83-a8fb-ef33f8c7e4ba",
         "method": "GET",
         "url": "http://localhost:5000/odata/Customers",
         "headers": {
             "accept": "application/json;odata.metadata=full"
          }
        }
    ]
}
  1. Return (Postman)
--batchresponse_3d454477-2888-4f47-839a-7d8965885460
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 200 OK
Content-Type: application/json; odata.metadata=full; odata.streaming=true; charset=utf-8
OData-Version: 4.0

{"@odata.context":"http://localhost:5000/odata/$metadata#Customers","value":[{"@odata.type":"#BatchTest.Models.Customer","@odata.id":"http://localhost:5000/odata/Customers(1)","@odata.editLink":"http://localhost:5000/odata/Customers(1)","Id":1,"Country":"\u0420\u043e\u0441\u0441\u0438\u044f"}]}
--batchresponse_3d454477-2888-4f47-839a-7d8965885460--
@maximpashuk

This comment has been minimized.

Contributor

maximpashuk commented Aug 29, 2018

Just few words more about string escaping.

Well-known Newtonsoft.Json serializer has a special enum StringEscapeHandling to control escaping behavior during json serialization.

JsonLight serializer in OData has only 'EscapeNonAscii' behavior implemented in terms of newtonsoft.json (see JsonValueUtils)

Probably, compromise solution is to change behavior of JsonValueUtils to escape only control characters ('Default' behavior in terms of newtonsoft.json)

I think this will satisfy most of people.

@xuzhg

This comment has been minimized.

Member

xuzhg commented Aug 29, 2018

Update:

if you set Accept=application/json in the request header, you will also get the result as:

{"responses":[{"id":"0d24a607-3fb0-4a83-a8fb-ef33f8c7e4ba","status":200,"headers":{"content-type":"application/json; odata.metadata=full; odata.streaming=true; charset=utf-8","odata-version":"4.0"}, "body" :{"@odata.context":"http://localhost:5000/odata/$metadata#Customers","value":[{"@odata.type":"#BatchTest.Models.Customer","@odata.id":"http://localhost:5000/odata/Customers(1)","@odata.editLink":"http://localhost:5000/odata/Customers(1)","Id":1,"Country":"\u0420\u043e\u0441\u0441\u0438\u044f"}]}}]}

However, if we use the Postman to show the response, Postman will help to render the escaped string as correct character:

{
    "responses": [
        {
            "id": "0d24a607-3fb0-4a83-a8fb-ef33f8c7e4ba",
            "status": 200,
            "headers": {
                "content-type": "application/json; odata.metadata=full; odata.streaming=true; charset=utf-8",
                "odata-version": "4.0"
            },
            "body": {
                "@odata.context": "http://localhost:5000/odata/$metadata#Customers",
                "value": [
                    {
                        "@odata.type": "#BatchTest.Models.Customer",
                        "@odata.id": "http://localhost:5000/odata/Customers(1)",
                        "@odata.editLink": "http://localhost:5000/odata/Customers(1)",
                        "Id": 1,
                        "Country": "Россия"
                    }
                ]
            }
        }
    ]
}

xuzhg added a commit to xuzhg/odata.net that referenced this issue Aug 31, 2018

@xuzhg

This comment has been minimized.

Member

xuzhg commented Sep 4, 2018

@maximpashuk @igor-moskvitin Do you have any chance to take a look about my Pull request about this escape string? #1242. Please let me know any problem.

@xuzhg

This comment has been minimized.

Member

xuzhg commented Sep 12, 2018

Merged. Thanks.

@xuzhg xuzhg closed this Sep 12, 2018

OData project automation moved this from To do to Done Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment