minimul / qbo_api

Ruby JSON-only client for QuickBooks Online API v3. Built on top of the Faraday gem.
MIT License
85 stars 45 forks source link

raise_http_exception.rb#parse_json: undefined method `[]` for ni:NilClass (NoMethodError) #125

Open ybakos opened 1 year ago

ybakos commented 1 year ago

When my app has not o-authed with QBO, the qbo_api used to elegantly raise an exception, that we could rescue and do a redirect.

v3.0.1 seems to have a regression.

/Users/ybakos/.rvm/gems/ruby-3.1.1@rugged_thread/gems/qbo_api-3.0.1/lib/qbo_api/raise_http_exception.rb:72:in parse_json': undefined method[]' for nil:NilClass (NoMethodError)

  errors = fault['Error'] || fault['error']

To replicate:

Do not have a valid authentication token. Invoke qbo_api.all(:customer_type).count.

The behavior was different in 2.0.2.

ybakos commented 1 year ago

When qbo_api.all returns an Enumerator, and credentials are not valid, it used to raise a QboApi::Unauthorized error upon accessing the Enumerator.

Now, no such exception is raised, and as soon as one tries to use that Enumerator, the unhandled exception, noted above, arises.

I am digging more into finding out in which version this behavior has emerged. Likely due to the faraday change, etc.

ybakos commented 1 year ago

I am getting warmer. In 3.0.0, qbo_api would raise a QboApi::BadRequest exception.

In 3.0.1, the error is unhandled resulting in the NoMethodError in the top post here in this issue.

ybakos commented 1 year ago

I have isolated the behavior change. To be clear, there are two behavior changes.

  1. In 2.x, qbo_api would raise a QboApi::Unauthorized exception when the values passed to the QboApi constructor are invalid, eg:
qbo_api = QboApi.new(access_token: nil, realm_id: nil)
qbo_api.get(:customer_types).first # QboApi::Unauthorized

In 3.0.0, qbo_api raises a QboApi::BadRequest exception for the same example as above, eg:

qbo_api = QboApi.new(access_token: nil, realm_id: nil)
qbo_api.get(:customer_types).first # QboApi::BadRequest

This, perhaps, is an indicator of how the latest Faraday sets a 400 instead of 401 when the credentials are nil (?).

  1. Now, in 3.0.1, we see a new behavior change. No intentional exception is raised when we execute:
    qbo_api = QboApi.new(access_token: nil, realm_id: nil)
    qbo_api.get(:customer_types).first # 👈 uncaught NoMethodError exception

The NoMethodError, as reported in the top post of this issue, is raised.

When I revert https://github.com/minimul/qbo_api/commit/8ea982e5f5b8c51aa27d795f505cedfdde458343 , a QboApi::BadRequest is raised.

minimul commented 1 year ago

Hi @ybakos.

Thanks so much for this research and breakdown.

I'll take a closer look at this soon.

minimul commented 1 year ago

The gem is catching 400s => QboApi::BadRequest. 🤷‍♂️

ybakos commented 1 year ago

@minimul Hi, I am not sure what your last comment is indicating.

If you try:

qbo_api = QboApi.new(access_token: nil, realm_id: nil)
qbo_api.get(:customer_types).first # 👈 uncaught NoMethodError exception

You will see that qbo_api is not catching an exception, or it is not checking for a situation and raising a QboApi exception.

minimul commented 1 year ago

It was in reference to your comment

This, perhaps, is an indicator of how the latest Faraday sets a 400 instead of 401 when the credentials are nil (?).

So I guess that is not the problem since qbo_api is catching 400s.

I agree that an exception should be raised as that is the past behavior.

That said, I'm personally not on 3.x and won't be for a bit so I'm open to PRs but I won't be digging into this anytime soon.

mculp commented 1 week ago

hey @ybakos, for a second, I thought I had caused this issue. I recently submitted a PR to update how errors are handled, which I do think fixed a nil issue. Not sure if it fixed yours, but maybe you could try it out and let me know. If not, I’ll try to address your issue.

https://github.com/minimul/qbo_api/pull/137

mculp commented 1 week ago

yes, I do think my PR would fix the NoMethodError you are experiencing because fault is set to an empty hash by default

minimul commented 5 days ago

I'll take a look soon.