Skip to content
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

[System.Text.Json] Serializer loses precision on ulong #52393

Closed
queequac opened this issue Nov 26, 2023 · 13 comments
Closed

[System.Text.Json] Serializer loses precision on ulong #52393

queequac opened this issue Nov 26, 2023 · 13 comments
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. Needs: Repro Indicates that the team needs a repro project to continue the investigation on this issue
Milestone

Comments

@queequac
Copy link

Description

Serializing an ulong to JSON within asp.net core loses precision. Stumbled over this while upgrading an older project from traditional ASPnet to .net core 8.

Reproduction Steps

Using .net 8 and System.Text.Json 8.0.0.

        public class Test
        {
            public ulong Key { get; set; }
        }

Exposing data via minimal API

            app.MapGet("/api/test", async (HttpContext context) =>
            {
                return new Test() { Key = 2121241830635120343 };
            });

The result in the browser is the following:

{
    "Key": 2121241830635120400
}

Expected behavior

{
    "Key": 2121241830635120343 
}

Actual behavior

{
    "Key": 2121241830635120400
}

Regression?

No response

Known Workarounds

For the moment I have written a custom JsonConverter which calls ToString for the ulong myself.

Configuration

.net 8, running x64 (emulated on win dev kit arm64) on Windows 11.

System.Text.Json 8.0.0

Other information

No response

@ghost ghost added the untriaged label Nov 26, 2023
@ghost
Copy link

ghost commented Nov 26, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Serializing an ulong to JSON within asp.net core loses precision. Stumbled over this while upgrading an older project from traditional ASPnet to .net core 8.

Reproduction Steps

Using .net 8 and System.Text.Json 8.0.0.

        public class Test
        {
            public ulong Key { get; set; }
        }

Exposing data via minimal API

            app.MapGet("/api/test", async (HttpContext context) =>
            {
                return new Test() { Key = 2121241830635120343 };
            });

The result in the browser is the following:

{
    "Key": 2121241830635120400
}

Expected behavior

{
    "Key": 2121241830635120343 
}

Actual behavior

{
    "Key": 2121241830635120400
}

Regression?

No response

Known Workarounds

For the moment I have written a custom JsonConverter which calls ToString for the ulong myself.

Configuration

.net 8, running x64 (emulated on win dev kit arm64) on Windows 11.

System.Text.Json 8.0.0

Other information

No response

Author: queequac
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@huoyaoyuan
Copy link
Member

Javascript number is defined as double. See dotnet/runtime#60780 (comment) and related discussions.

@gregsdennis
Copy link

gregsdennis commented Nov 26, 2023

@huoyaoyuan JavaScript numbers, sure, but this is JSON, in which numbers are arbitrary-precision.

I would expect the full value (in the big-int case as well).

@elgonzo
Copy link

elgonzo commented Nov 26, 2023

EDIT: As it turned out (see below), the issue is not so much with ASP.NET but rather with imprecise json number presentation in the MS Edge browser. Uff...

It appears to be ASP.NET Core's doing, employing a custom/specific STJ configuration serialization. STJ itself has no problem serializing ulong's correctly (https://dotnetfiddle.net/l248U1). I therefore would not see it as a problem of STJ, but an issue related to ASP.NET Core's serialization configuration.

@eiriktsarpalis
Copy link
Member

It seems to be aspnetcore specific, I'm not sure why though. Transferring to the relevant repo for triage.

@eiriktsarpalis eiriktsarpalis transferred this issue from dotnet/runtime Nov 27, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Nov 27, 2023
@martincostello martincostello added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed untriaged needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Nov 27, 2023
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@captainsafia
Copy link
Member

Thanks for reporting this issue, @queequac. I appreciate your patience as I work through the backlog of bug reports here.

I, unfortunately, wasn't able to repro this locally with the following sample using either both RDG (dynamic code-gen) or RDF (static code-gen).

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

ulong number = 2121241830635120343;

app.MapGet("/primitive", () => number);
app.MapGet("/complex", () => new Test { Key = number });

app.Run();

class Test
{
    public ulong Key { get; set; }
}

You mentioned that you discovered this problem while migrating a previous codebase. Is the repro missing a detail that only shows up in the code you are migrating?

@captainsafia captainsafia added Needs: Repro Indicates that the team needs a repro project to continue the investigation on this issue area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels May 8, 2024
@dotnet-policy-service dotnet-policy-service bot added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label May 8, 2024
@captainsafia captainsafia added this to the Backlog milestone May 8, 2024
@martincostello
Copy link
Member

I wonder if this is the same issue as #55571?

@queequac
Copy link
Author

queequac commented May 15, 2024

Hi @captainsafia ,

thanks for reaching out. Actually I can easily reproduce the issue with an empty net8.0 web api project from scratch:

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.

var app = builder.Build();

// Configure the HTTP request pipeline.

app.MapGet("/test", () =>
{
    ulong x = ulong.MaxValue - 3; // 18446744073709551612
    return x; // results in 18446744073709552000
});

app.Run();

Same with your sample above, it does not return 2121241830635120343 but 2121241830635120400 for /primitive
(I used the project template without native-aot)

@dotnet-policy-service dotnet-policy-service bot added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. Status: No Recent Activity labels May 15, 2024
@queequac
Copy link
Author

queequac commented Jun 1, 2024

@martincostello Oh wow, thanks for the hint.
@captainsafia Indeed ASP.NET is not doing anything wrong here. While I am NOT using Swagger, surprisingly Microsoft Edge is showing the resulting data with Javascript precision. The developer tools' network view shows the correct number on the wire.🤯 Google Chrome does also show the ulong in the document as-is, only Edge seems (since recently?) to ‘translate’ internally into standard-compliant JavaScript before rendering. Actually not sure if that is a good thing Edge is doing here... maybe raise an issue there, @gregsdennis , if precision in JSON is not equivalent to precision in Javascript.

@queequac queequac closed this as completed Jun 1, 2024
@gregsdennis
Copy link

gregsdennis commented Jun 1, 2024

maybe raise an issue there, @gregsdennis , if precision in JSON is not equivalent to precision in Javascript. - @queequac

Not sure where you're suggesting I raise an issue, or for what.

@queequac
Copy link
Author

queequac commented Jun 1, 2024

maybe raise an issue there, @gregsdennis , if precision in JSON is not equivalent to precision in Javascript. - @queequac

Not sure where you're suggesting I raise an issue, or for what.

You mentioned above

JavaScript numbers, sure, but this is JSON, in which numbers are arbitrary-precision.

In this case handling json-documents in edge's rendering should maybe also not lose precision. Anyway, personally no hard feelings about this.🙃

@gregsdennis
Copy link

Okay, but JSON and JS number handling are inherently defined differently.

While JSON numbers are theoretically unlimited in scale and precision, JS does define those limitations, so if you're interpreting JSON through JS, it's totally understandable that you'll encounter those limits.

There is no issue that I can file in any repo that's going to help this.

My point in that comment was that if this were an STJ problem, then it should serialize the ulong fully. But it turned out to not be an STJ issue.

The behavior of Asp.Net could do what it feels is right given its domain is web services.

@queequac
Copy link
Author

queequac commented Jun 1, 2024

There is no issue that I can file in any repo that's going to help this.

You have completely misunderstood me. :)
It was not my intention to change JS or JSON. It was more about potentially opening an issue for Ms Edge, since it handles numbers in JSON documents like numbers in JS.

Anyway, nothing else to discuss here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. Needs: Repro Indicates that the team needs a repro project to continue the investigation on this issue
Projects
None yet
Development

No branches or pull requests

8 participants