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

Avoid JSON serializing Float::INFINITY #2495

Merged
merged 2 commits into from
Jan 25, 2018

Conversation

jhawthorn
Copy link
Contributor

This works fine under the default JSON library (it ends up being NULL), but it errors under OJ.

Fixes #2486

@hhff
Copy link

hhff commented Jan 10, 2018

LGTM but i'd guess that in_stock? and potentially is_backorderable? also doesn't need to be serialized when we're not tracking inventory. Just a thought!

This renders small.json.jbuilder as a partial which already has
total_on_hand.
This works fine under the default JSON library (it ends up being NULL),
but it errors under OJ.
@jhawthorn jhawthorn force-pushed the avoid_json_infinity branch from 3b9ca9f to aaaef13 Compare January 25, 2018 23:21
@jhawthorn
Copy link
Contributor Author

jhawthorn commented Jan 25, 2018

LGTM but i'd guess that in_stock? and potentially is_backorderable? also doesn't need to be serialized when we're not tracking inventory. Just a thought!

I've spent some time thinking about this. You're right that it's confusing to have values that should basically be ignored when track_inventory is off. I think in stock should probably keep in_stock even if we were to hide backorderable, as we do in most places consider track_inventory=false to imply in_stock.

Related to this is #2522, where we are considering adding another attribute/scope to represent (in_stock || backorderable || !track_inventory).

For now, at least, I think we should keep all of these fields in for compatibility.

@jhawthorn jhawthorn merged commit 51253a8 into solidusio:master Jan 25, 2018
@joshhepworth
Copy link
Contributor

joshhepworth commented Feb 13, 2018

I just ran into this issue as well, and actually had to address this in the Products API as well due to this line: https://github.com/solidusio/solidus/blob/master/api/app/helpers/spree/api/api_helpers.rb#L54

The because the Spree::Product#any_variants_not_track_inventory? method is private, I ended up with this in _product.json.jbuilder, which is not pretty for a few reasons:

  json.(product, *@product_attributes - [:total_on_hand])

  json.total_on_hand(product.total_on_hand.to_f.infinite? == 1 ? nil : product.total_on_hand)

I'm not sure if this is acceptable, or if you'd rather have a solution that cleaned this up a little more extensively. I'm also not sure if the product_attributes helper ends up touching other endpoints as well.

kennyadsl added a commit to nebulab/solidus that referenced this pull request May 27, 2019
Float::Infinity serialization is broken with Oj gem, see
solidusio#2495 (comment)

This commit makes sure we are always nullifying total_on_hand before
passing trying to serialize it, if it is a Float::Infinite.
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 this pull request may close these issues.

5 participants