openactive / modelling-opportunity-data

OpenActive Modelling Opportunity Data specification
https://www.openactive.io/modelling-opportunity-data/
Other
6 stars 6 forks source link

Prefer strings for decimal values #245

Open nickevansuk opened 4 years ago

nickevansuk commented 4 years ago

Proposer

On behalf of Bookwhen and TeamUp

Use Case

There are issues in both Ruby and Python rendering decimals as JSON numbers using standard libraries. Although we had previously discussed the benefits of using the number format, implementation experience has shown that in practice a string format is more widely accepted and implemented.

References

Approaches taken by other APIs include:

Proposal

Although the integer approach might have merits, it is not compatible with the existing implementation, as we could not distinguish e.g. 100 (£100 in the current implementation).

Hence, this proposal suggests that we adopt the same approach as schema.org and use strings as the preferred representation of decimal values within the Modelling Opportunity specification, and also within the Open Booking API.

Open Booking API implementations that are in progress would conform to this new approach, and other feeds would be updated over time.

Example

{
  "@type": "Offer",
  "url": "https://goteamup.com/p/1534221-north-star-yoga/e/27468710-online-class/",
  "price": "0.00",
  "priceCurrency": "GBP"
}
nickevansuk commented 4 years ago

More research of various JSON libraries in various languages is required here to confirm that above. Additionally, any meta-analysis that can be found to support the proposed direction here would be helpful.

nathansalter commented 4 years ago

In PHP it's pretty arbitrary whether you use floats or strings containing floats. If you do numeric operators on any string containing a numeric value it's silently converted behind the scenes:

php > var_dump("1.24" == 1.24);
bool(true)

For that reason I'd suggest switching to the string representation, even though internally in our systems we use integer for prices (mirroring stripe and a common approach for payment providers) I understand how it wouldn't be possible to determine if 100 was £1 or £100, so a migration wouldn't really be possible without other parameters.

nickevansuk commented 4 years ago

Yes for sure, in hindsight it does indeed seem that integer would have been the best option. I wonder if schema.org are in the same boat...

thill-odi commented 4 years ago

Googling around, it seems schema.org's specification of Number or Text simply (and unsurprisingly) reflects the situation 'in the wild'. There are plenty of APIs that specify a datatype of double for the numeric aspect of price, as well as a number that use string (such as Google Pay). Fear of floating-point imprecision sometimes leads to recommendation of strings.

I don't see how we have much choice but to follow schema.org's lead here - though perhaps with a recommendation of strings over numeric types.

nickevansuk commented 3 years ago

Just referencing https://github.com/openactive/models-php/issues/64 here. Seems that there might be issues in PHP too @nathansalter ...

nathansalter commented 3 years ago

Yes, PHP has fairly good currency handling with the bcmath library, and this uses strings to represent fixed point numbers rather than floats or ints. I think it's worth updating the spec to allow both floats and strings. This also aligns correctly with https://schema.org/price

nickevansuk commented 3 years ago

As discussed on the 2021-02-24 W3C call, further investigation has revealed that it is indeed possible to represent prices as a JSON Number in PHP, Ruby and Python (as shown by fixes made in both Bookwhen in Ruby and TeamUp in Python, and @nathansalter in PHP). There are no issues with .NET due to the Decimal type.

Therefore, although it appears that following schema.org conventions may have been more consistent in hindsight, there does not appear to be any actual issues using a JSON Number in modern languages and libraries.

We're desperately in need of a counter-example to the above to keep this issue open, as this appears to not be an issue after all?

nathansalter commented 3 years ago

Agreed, I'll try to get in contact with our client that was having an issue with this and get some code examples. It seems like the issue might be just not calling round($price, 2) on the prices before putting them in the OA Models.

nickevansuk commented 3 years ago

Just to update this issue, all implementers have confirmed that there is no issue using JSON Number for pricing. Hence this issue should be closed if no counter-examples are forthcoming.

nathansalter commented 3 years ago

I can confirm none of our partners use string pricing any more