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

JSON Schema serialize BigInt to string #491

Closed
2 tasks done
tzelon-cypator opened this issue Jul 13, 2022 · 5 comments
Closed
2 tasks done

JSON Schema serialize BigInt to string #491

tzelon-cypator opened this issue Jul 13, 2022 · 5 comments
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor

Comments

@tzelon-cypator
Copy link

tzelon-cypator commented Jul 13, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the regression has not already been reported

Last working version

4.1.0

Stopped working in version

4.2.0

Node.js version

16

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

12.3.1

💥 Regression Report

Before version 4.2.0 response schema with property type string, format int64, and pattern ^[0-9]*$ would be able to serialize BigInt to string.
version 4.2.0 return an error "Do not know how to serialize a BigInt"

Removing the format solves the issue.

Steps to Reproduce

codesandbox demo https://codesandbox.io/s/bigint-serialize-jqn5uu

app.get(
    "/",
    {
      schema: {
        response: {
          200: {
            type: "object",
            properties: {
              hello: {
                type: "string",
                format: "int64",
                pattern: "^[0-9]*$"
              }
            }
          }
        }
      }
    },
    async (request, reply) => {
      return { hello: 123n };
    }
  );

Expected Behavior

I'm not sure if the problem is in v4.2.0 or previous versions.

@mcollina
Copy link
Member

I could not have guessed that would have worked at all. Our support to BigInt is limited to integers: https://github.com/fastify/fast-json-stringify/blob/4d990f660bca9c3b3eb98f086a04c80ade428277/test/bigint.test.js.

I think something must be added in https://github.com/fastify/fast-json-stringify/blob/967bacfa248dde27550ea8fd5f3cebeb4e45b171/serializer.js to bring that back.

Would you like to send a PR?

@mcollina mcollina transferred this issue from fastify/fastify Jul 13, 2022
@mcollina mcollina added semver-minor Issue or PR that should land as semver minor good first issue Good for newcomers labels Jul 13, 2022
@tzelon-cypator
Copy link
Author

Sure, I will try to work on it.

@Adibla
Copy link

Adibla commented Aug 31, 2022

The matter appears to be settled. I tried to reproduce it but I don't have any errors. Does anybody still have that problem?

@ivan-tymoshenko
Copy link
Member

The matter appears to be settled. I tried to reproduce it but I don't have any errors. Does anybody still have that problem?

I think it started working after #504. I'm not sure, you can check if you want.

@Eomm
Copy link
Member

Eomm commented Sep 4, 2022

The matter appears to be settled. I tried to reproduce it but I don't have any errors. Does anybody still have that problem?

I think a test for this case would be good!
We have them already but we are not using the setup described above

ivan-tymoshenko added a commit that referenced this issue Sep 4, 2022
Eomm pushed a commit that referenced this issue Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

No branches or pull requests

5 participants