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

fix(api) treat bigInt as string all across the system #20888

Merged
merged 6 commits into from
Jan 4, 2024

Conversation

mahendraHegde
Copy link
Contributor

@mahendraHegde mahendraHegde commented Dec 27, 2023

Scope

What's changed:

  • remove explicit handling of bigInt on api side, treat it as string and let DB do the casting

Potential Risks / Drawbacks

  • tested with PG and sqlite but not sure if any DB expect the casting as part of query binding itself
  • knex doesnt parse bigInt, so the queries are sent as xxxxn to DB, hence we dont have an option but to use string

Review Notes / Questions

  • found Document SQLite's lack of BigInt primary key support #20889 while working on this, as it requires some discussion i created a separate issue
  • UI inputs are not handling bigInt as well, its reading the saved data well, but pasting a bigint will result in loosing the precision because of implicit conversion to number(caused by input type=number)

Fixes #20291

Screenshots

create

Screenshot 2023-12-26 at 9 23 42 PM

filter

Screenshot 2023-12-26 at 10 36 07 PM

invalid

Screenshot 2023-12-26 at 10 36 50 PM

Copy link

changeset-bot bot commented Dec 27, 2023

🦋 Changeset detected

Latest commit: 8ad7afe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Minor
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mahendraHegde mahendraHegde marked this pull request as draft December 27, 2023 01:31
@mahendraHegde mahendraHegde marked this pull request as ready for review December 27, 2023 01:57
@mahendraHegde
Copy link
Contributor Author

@br41nslug ready to be reviewed

@br41nslug br41nslug self-requested a review December 27, 2023 10:15
@br41nslug
Copy link
Member

UI inputs are not handling bigInt as well, its reading the saved data well, but pasting a bigint will result in loosing the precision because of implicit conversion to number(caused by input type=number)

This is being addressed in #20287

Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great PR! Works like a charm 😄

Would only love to see that tiny constant thing addressed so we dont need a big block of comments explaining why these are hardcoded.

api/src/services/graphql/types/bigint.ts Outdated Show resolved Hide resolved
br41nslug and others added 2 commits January 3, 2024 20:12
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
@br41nslug br41nslug merged commit 40b25bc into directus:main Jan 4, 2024
4 checks passed
@github-actions github-actions bot added this to the Next Release milestone Jan 4, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

GraphqQL BigInt handling
3 participants