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

Edge case - increase limits of monetary columns #838

Closed
Izayda opened this issue Apr 22, 2021 · 5 comments
Closed

Edge case - increase limits of monetary columns #838

Izayda opened this issue Apr 22, 2021 · 5 comments
Milestone

Comments

@Izayda
Copy link
Contributor

Izayda commented Apr 22, 2021

Is your feature request related to a problem? Please describe.
The limit for monetary fields (cart sum, order sum, product variant price) is now (2^31-1)/100 = 21 474 836,47
This limit can be not enough for countries with high denomination currencies.

Describe the solution you'd like
Limit appears from TypeORM data type Number. It is considered as int with corresponding limit in node.js and selected DB.

Quick research shows, that there are couple of approaches:

  1. Using bigint It's solid approach, but Typeorm consider bigint as string, so many modifications should be done at the Vendure side
  2. Using decimal(15,4) or even bigger Also solid approach, but it requires using accurate data type and math operations at the Vendure side.

Describe alternatives you've considered
Continue using int. But there will still exist problems with overflowing.

@mattgills
Copy link
Contributor

mattgills commented Sep 29, 2022

Throwing in support of this feature request. In addition to countries with high denomination currencies, B2B e-commerce often involves higher priced goods. We have experienced scenarios of needing to ask customers to split orders, so they do not exceed the ~$21M limit. There is some ability to manipulate the TypeORM metadata to utilize bigint in the database, but there are some side effects that I have not figured out how to solve without changes to Vendure. One example is that the GraphQL requires special handling of bigint data types and treats them as strings.

Edit:
Minor correction, we had products that couldn't be loaded not orders that couldn't be placed. A $21M order would be a good problem to have, maybe someday 😄.

@michaelbromley michaelbromley moved this to 🤔 Under consideration in Vendure OS Roadmap Sep 29, 2022
@michaelbromley
Copy link
Member

Thanks @mattgills for your input on this.
If we can settle on a robust solution for the DB data type, we could perhaps introduce a new Scalar type on the GraphQL side which is able to handle bigint data as a number rather than a string.

Then I guess we'd only be limited by JavaScript's Number.MAX_SAFE_INTEGER which is 9007199254740991, so that would allow us to go up to $9 quadrillion, which should be enough even for your customers ;)

@michaelbromley michaelbromley added this to the v2.0 milestone Sep 29, 2022
@mattgills
Copy link
Contributor

I would lean towards keeping the integer math, as that has been reliable, and that reliability has been important with respect to financial calculations. A few things that may be helpful with bigint being interpreted as a string:

  1. This GitHub issue thread using transformers to take the string from the DB and convert it to a number would allow for a single transformer to be defined and applied to all bigint columns. Could there be a special Vendure BigInt() decorator that handles this and ensures it's always included?
  2. Since bigint has larger boundaries that JavaScript's Number.MAX_SAFE_INTEGER and Number.MIN_SAFE_INTEGER, it would be nice to limit the value stored in the DB to JavaScript's more restrictive range. For some reason I haven't found a clear typeorm decorator or property to limit the max value of int/bigint column. Perhaps theres something that could be included in the transformer/decorator that ensures the integer is bound to JavaScript's range.

As for GraphQL, a custom Scalar seems like the right approach. I would think creating it from scratch in Vendure would be better than leveraging 3rd party packages, like apollo-type-bigint (not sure how active that is - last commit was 2019). Regardless, there is interesting things to take from that repo, such as the differentiation between safe and bigInt - safe would work with 53 bit integers, which is in the realm of my comment in point 2.

@michaelbromley michaelbromley moved this from 🤔 Under consideration to 🏗 In progress in Vendure OS Roadmap Feb 3, 2023
@badrange
Copy link

badrange commented Feb 8, 2023

There is also this scalar:

https://the-guild.dev/graphql/scalars/docs/scalars/big-int

@michaelbromley
Copy link
Member

@badrange thanks! yeah I've been experimenting with that - we already have the graphql-scalars package as a dependency. I'm currently leaning towards a custom Money scalar based on that that we can fully control from Vendure config, which would gives us more possibility to support unforeseen issues in future. Still in the research phase right now.

@michaelbromley michaelbromley moved this from 🏗 In progress to 🔖 Ready in Vendure OS Roadmap Feb 23, 2023
@michaelbromley michaelbromley moved this from 🔖 Ready to ✅ Done in Vendure OS Roadmap Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants