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

Decide on the size of numbers we accept #172

Closed
amorton opened this issue Feb 23, 2023 · 13 comments · Fixed by #432
Closed

Decide on the size of numbers we accept #172

amorton opened this issue Feb 23, 2023 · 13 comments · Fixed by #432
Assignees

Comments

@amorton
Copy link
Contributor

amorton commented Feb 23, 2023

Split from #135

For the Document Value Limits for a number we ended with:

TODO: define max num that is well handled by BigDecimal

Need to work out what limits we may have, because JSON and BigDecimal both support unlimited size but we need a limit to protect the server from bad workloads and know what we need to test against.

With reference to the BigDecimal we could do something like say the integer component can be no bigger than 128 bites . By reading unscaledValue and then getting the number of bits from the BigInteger

@tatu-at-datastax did you have some opinions ?

@tatu-at-datastax
Copy link
Contributor

This gets bit tricky. Jackson 2.15 will actually have support for configurable maximum limits for numbers, with default of 1000 digits. But that is not yet out; I hope to get that released in March. So one option would be to wait until that version and off-load part of validation there. It also has maximum length for String tokens (values).

If we want to validate these earlier things are bit more complicated if we want to catch problems as early as possible.
It is doable of course, just... quite a bit of effort for sort of edge case.

So I would probably add this is one of later things to address so that we could just use Jackson's limit support.
And I can keep this in mind wrt Jackson release too, one of first use cases.

@amorton amorton added this to the March milestone Feb 24, 2023
@amorton
Copy link
Contributor Author

amorton commented Feb 24, 2023

. So one option would be to wait until that version and off-load part of validation there. It also has maximum length for String tokens (values).

@tatu-at-datastax delaying until jackson has the features will be fine, any chance it could support some of the other documents limits we have in the spec e.g. max array size ? this would seem like the best place to put them.

I have created a new milestone to push this work out , will update this ticket.

@tatu-at-datastax
Copy link
Contributor

@amorton Yes, absolutely, I can work on some of the validation aspects and get the validation hooked in. And then we can decide parts that may be postponed.

@tatu-at-datastax
Copy link
Contributor

Other document constraints are now validated as per #172 (via #278). Leaving this open for numbers.

@vkarpov15
Copy link
Collaborator

As far as I know, Mongoose doesn't have any particular restrictions on number sizes, just whatever MongoDB and JavaScript support.

One thing worth mentioning in this vein though: we're planning on adding native support for JavaScript BigInts to Mongoose soon, see Automattic/mongoose#13081. BigInt support isn't a blocker, but it may be worthwhile to think about how to support BigInts. Here's a writeup I did on JavaScript BigInts a few years ago.

@tatu-at-datastax
Copy link
Contributor

I think our plan for now is to only use a single number type (BigDecimal) for internal processing.
I don't know if we can really support true typing mongo style, due to many reasons. But BigDecimal itself has unlimited magnitude, precision, so it does not limit value sizes. But the whole are of type-processing will be a major challenge for us.

@tatu-at-datastax
Copy link
Contributor

Planning to support this via Jackson 2.15 once released (during April 2023). May at first rely on library defaults which are reasonably loose, until we can ensure that 2.15 is used by all deployments (CNDB needs to upgrade).

@sync-by-unito
Copy link

sync-by-unito bot commented Apr 26, 2023

➤ John Messavussu commented:

Tatu Saloranta - Is this still blocked?

@sync-by-unito
Copy link

sync-by-unito bot commented Apr 26, 2023

➤ Tatu Saloranta commented:

John Messavussu Yes, sort of. Jackson 2.15.0 is now released, but we need to make sure CNDB upgrades their Jackson version to 2.14.2 first. Need CNDB Team’s help with:

https://github.com/riptano/cndb/pull/6739 ( https://github.com/riptano/cndb/pull/6739|smart-link )

@tatu-at-datastax
Copy link
Contributor

Jackson 2.15.1 released so will work on this: realized CNDB is non-issue as we only need upgrade to JSON API (or APIs in general).

My current thinking is to limit maximum textual length to 50 digits (which translates to about 160 bits of precision I think) -- it's more than what we need, but from DoS perspective should be low enough to prevent meaningful issues. From performance measurements I saw wrt Java/Jackson, lengths of 750 characters or so was where performance started to significantly degrade (change to limits is via FasterXML/jackson-core#827 but I don't seem to have link to perf numbers; will add if I can find a link)

@tatu-at-datastax
Copy link
Contributor

tatu-at-datastax commented May 18, 2023

Filed:

FasterXML/jackson-databind#3938

but this looks quite specific; somehow related to handling of polymorphic values, buffering. Was finally able to reproduce, interested in seeing how to actually fix. :)

EDIT: was able to reproduce, and find a fix. But fix will only be in 2.15.2, to be released, so need to wait until that occurs.

@sync-by-unito
Copy link

sync-by-unito bot commented May 18, 2023

➤ Tatu Saloranta commented:

Moving back to “blocked” until Jackson 2.15.2 released.

@tatu-at-datastax
Copy link
Contributor

Back to working on this now that Jackson 2.15.2 is available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants