Skip to content

Conversation

@zacblazic
Copy link
Contributor

It's currently possible for the parsed response from fetch_next_page to be a string instead of a hash.

In one scenario, when the server returns a 504 gateway timeout, it will return an HTML string instead of a hash and attempt to call has_key? on the string:

Traceback (most recent call last):
	12: from recreate_bug.rb:15:in `<main>'
	11: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:49:in `each'
	10: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:49:in `each'
	 9: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:49:in `each'
	 8: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:49:in `each'
	 7: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:49:in `each'
	 6: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:36:in `each'
	 5: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:36:in `each'
	 4: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:38:in `block in each'
	 3: from recreate_bug.rb:17:in `block in <main>'
	 2: from recreate_bug.rb:17:in `to_a'
	 1: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:47:in `each'
/Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:69:in `fetch_next_page': undefined method `has_key?' for #<String:0x00007fa4baa1dfe0> (NoMethodError)

My changes aim to handle this edge case and raise an exception with the underlying HTTP response message and code:

Traceback (most recent call last):
	8: from recreate_bug.rb:15:in `<main>'
	7: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:49:in `each'
	6: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:36:in `each'
	5: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:36:in `each'
	4: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:38:in `block in each'
	3: from recreate_bug.rb:17:in `block in <main>'
	2: from recreate_bug.rb:17:in `to_a'
	1: from /Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:47:in `each'
/Users/zacblazic/workspace/personal/onelogin-ruby-sdk/lib/onelogin/api/cursor.rb:70:in `fetch_next_page': Gateway Time-out (OneLogin::Api::ApiException)

It's currently possible for the parsed response to be a string
instead of a hash in some cases.

One scenario is if the server returns a 504 gateway
timeout, in which case we are returned a string of HTML
instead of JSON response.
@pitbulk
Copy link
Contributor

pitbulk commented Jul 16, 2020

Hi @zacblazic,

sorry for the delayed reply. I believe that rather than taking care of the response with a 504 happens, we should instead detect the 504 status and raise the error then, in fact that was my goal in that ticket that I never implemented: #19.

Not sure if this patch could side effect other endpoints returning a string

@zacblazic
Copy link
Contributor Author

No worries @pitbulk!

I agree that we should ideally be checking the response codes, definitely first prize in this scenario, but I opted for a quick win here instead. 😅

Not sure if this patch could side effect other endpoints returning a string

I thought about this and ensured that it would be safe as the code is right now since the code explicitly expects the result to be a hash currently by performing a check for specific keys which would fail otherwise i.e. !json.has_key?(@container). Another example is we always access the contents of the json result as a hash here.

Anyways, not saying this is a perfect fix but it should work.

@zacblazic zacblazic closed this Oct 10, 2023
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.

2 participants