avadev / AvaTax-REST-V2-Ruby-SDK

Sales Tax API SDK for Ruby and AvaTax REST
https://developer.avalara.com/sdk/
Apache License 2.0
36 stars 56 forks source link

Don't change default_options for Oj globally #22

Closed SophiaLWu closed 6 years ago

SophiaLWu commented 6 years ago

In connection.rb, the Oj global default_options are changed here: https://github.com/avadev/AvaTax-REST-V2-Ruby-SDK/blob/7de81a1d115774722ed687199916cb23127b796a/lib/avatax/connection.rb#L23

This is problematic because it globally changes the options. This can lead to unexpected consequences since it is affecting behavior of an app outside of just the avatax API scope. For instance, if an app makes a request to avatax API, it changes the default options, and thus any other future times the app uses Oj to serialize JSON, decimals are converted to big decimals.

Could this be changed to not add default_options to Oj globally? One workaround would be to pass in bigdecimal_load: :bigdecimal as an option instead for the load methods.

han8909227 commented 6 years ago

@SophiaLWu Thank you for bringing up this issue, I will discuss the best approach to resolve it with my team and implement a workaround.

Will keep you update, Han from Avalara

SophiaLWu commented 6 years ago

@han8909227 Awesome - thanks so much for the quick response!

han8909227 commented 6 years ago

@SophiaLWu We have added a config to disable the decimal => big_decimal conversion, and the default behavior is false (not converting). I hope this resolves your concern, and please close this issue if that's the case.

SophiaLWu commented 6 years ago

Hi @han8909227 thanks for being quick with this. Is it an issue for the calculations that your api performs for us if we disable this conversion? I'm assuming you guys converted to big_decimal for more precision.

han8909227 commented 6 years ago

@SophiaLWu Our API doesn't take any param to reduce or add precision to the calculated numbers. This conversion is only applicable to the response object, meaning it only parses decimal to big_decimal after the response has been fetched from the API.

SophiaLWu commented 6 years ago

@han8909227 Great - thanks for the fix!