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

Invalidate any integers greater than 2^53 #439

Open
3 tasks
lukehesluke opened this issue Oct 3, 2023 · 2 comments
Open
3 tasks

Invalidate any integers greater than 2^53 #439

lukehesluke opened this issue Oct 3, 2023 · 2 comments

Comments

@lukehesluke
Copy link
Contributor

lukehesluke commented Oct 3, 2023

Context

JSON technically has no limits on size or precision of numerics. However, practically, the programming language / library used to interpret the JSON will make assumptions.

I think it would be reasonable to put at least a size limit of 2^63 - 1 on (positive) JSON integers so that they can easily be processed by languages with something like the C# or Java (signed) "long" type, the default PHP integer type on 64-bit systems, etc.

However, I would suggest going further and reducing this size limit to 2^53 - 1, which is the largest "safe" integer that can be represented in JavaScript's primitive number type. JavaScript represents a large part of the OpenActive community and also an absolutely necessary one due at least to web applications

Proposal

  • Impose a limit on all integers, so that they will be rejected by validator if they are not between the values -(2^53 - 1) ≤ x ≤ (2^53 + 1)
  • This would be imposed on schema data but also, importantly, the RPDE modified field, where imprecision could lead to the wrong update being considered the "latest"

Checklist

RE BigInt

JavaScript has a BigInt type, which can represent larger integers, but this is awkward to use with JavaScript's standard libraries and OpenActive's data as stands. MDN suggests using JSON.parse's reviver arg to parse large numbers into BigInts in JSON (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#use_within_json) but this can only work if the number was serialized into a string, and integers are not serialized into strings for OpenActive data.

The only other alternatives that allow a JavaScript user to therefore parse OpenActive data that has large integers is to either create a custom JSON parser (!) or use a library like https://github.com/sidorares/json-bigint https://github.com/josdejong/lossless-json, where there unfortunately don't seem to be many options

@lukehesluke
Copy link
Contributor Author

lukehesluke commented Oct 12, 2023

One thing I will add:

I am currently getting around this on a Node.JS app that consumes RPDE feeds by using BigInts to work with those feeds whose modifieds exceed 2^53.

It is quite a chore compared to how it would be if numbers had size limits imposed. This is because at every point at which object serialization or deserialization occurs (e.g. logs, database queries, queues, HTTP, etc) or where certain libraries are used, special handling has to be introduced, otherwise errors accumulate or there is a significant loss in accuracy (or there are sorting issues in cases where BigInts are automatically converted to strings)

@lukehesluke
Copy link
Contributor Author

lukehesluke commented Nov 6, 2023

Further difficulties with JavaScript: I've tried using https://github.com/sidorares/json-bigint in some of my own code to handle OpenActive data and can unfortunately say that it is not production-ready. There is a known issue in which it will attempt to convert floating point numbers to BigInts (if there are enough digits), which will cause an error. This means that the following OpenActive data will raise an error if processed with json-bigint:

{
  "@type": "GeoCoordinates",
  latitude: 51.52665709999999,
  longitude: -0.0882679
}

Luckily, there's another library, https://github.com/josdejong/lossless-json, which does a better job. You can replicate something like the expected behaviour of json-bigint with the following:

const { parse, isInteger } = require('lossless-json');

/**
 * @param {string} value
 * @returns {number | bigint}
 */
function convertNumberToBigIntIfLargeEnoughInt(value) {
  if (isInteger(value)) {
    const asInt = parseInt(value, 10);
    /* Note we consider equality to either of the bounds to be "too large" just
    to be extra cautious against the effects of precision loss */
    if (asInt >= Number.MAX_SAFE_INTEGER || asInt <= Number.MIN_SAFE_INTEGER) {
      return BigInt(value);
    }
    return asInt;
  }
  return parseFloat(value);
}

function jsonParseAllowingBigInts(text) {
  return parse(text, null, convertNumberToBigIntIfLargeEnoughInt);
}

> var s =
  '{"modified": 1689163234269127120371280937102987,"data": {"geo": {"@type": "GeoCoordinates","latitude": 51.52665709999999,"longitude": -0.0882679}, "remainingSpaces": 1}}';
> jsonParseAllowingBigInts(s)
{
  modified: 1689163234269127120371280937102987n,
  data: {
    geo: {
      '@type': 'GeoCoordinates',
      latitude: 51.52665709999999,
      longitude: -0.0882679
    },
    remainingSpaces: 1
  }
}

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

No branches or pull requests

1 participant