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

Handle empty array as a response in .first #70

Closed
wants to merge 2 commits into from
Closed

Handle empty array as a response in .first #70

wants to merge 2 commits into from

Conversation

Kooshaba
Copy link

I ran into this issue when querying the API for a Contact that did not exist using .first. The data in the body was returning an empty array rather than a hash in this case.

@@ -62,7 +62,9 @@ def all(conditions = {})

# Retrieve a single resource. See usage comment on .all
def first(conditions = {})
new(conditions.merge(Connection.get(create_route(:get, conditions)).body['data']))
data = Connection.get(create_route(:get, conditions)).body['data']
data = {} if data.empty?
Copy link
Collaborator

@apurvis apurvis May 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get a comment

@apurvis
Copy link
Collaborator

apurvis commented May 21, 2016

Honestly I don't think we should call new({}) in the case of a missing resource - we should raise an exception.

in my experience SG usually returns a bad status when you request a resource that doesn't exist... you are saying that in the case of Contact.first that they return valid status and just an empty body?

I believe you because I have never actually used the Contact resource and SG is not super consistent, but could you post the debug output?

@apurvis
Copy link
Collaborator

apurvis commented May 21, 2016

cc @jarthod

@jarthod
Copy link
Collaborator

jarthod commented May 21, 2016

Indeed to would be better to raise here, it's weird if sgizmo returns a 200 for that. I'm interested in the debug output too.

@Kooshaba
Copy link
Author

SurveyGizmo::API::Contact.first(survey_id: 2204849, campaign_id: 3312991, id: 100664998)
2016-05-24 16:49:41 INFO get https://restapi.surveygizmo.com/v4/survey/2204849/surveycampaign/3312991/contact/100664998?api_token=<SG_API_KEY>&api_token_secret=<SG_API_SECRET>
2016-05-24 16:49:41 DEBUG User-Agent: "Faraday v0.9.2"
2016-05-24 16:49:42 INFO 200
2016-05-24 16:49:42 DEBUG date: "Tue, 24 May 2016 23:48:49 GMT"
server: "Apache/2.2.15 (CentOS)"
set-cookie: "PHPSESSID=550ijmnc08mang2rv2dd7djls6; path=/; domain=restapi.surveygizmo.com; secure"
expires: "Thu, 19 Nov 1981 08:52:00 GMT"
cache-control: "no-store, no-cache, must-revalidate, post-check=0, pre-check=0"
pragma: "no-cache"
access-control-allow-origin: "http://www.surveygizmo.com"
content-length: "94"
connection: "close"
content-type: "application/json; charset=UTF-8"
2016-05-24 16:49:42 DEBUG {"result_ok":true,"total_count":null,"page":1,"total_pages":0,"results_per_page":50,"data":[]}
TypeError: no implicit conversion of Array into Hash
from /Users/andy.cernera/.rvm/gems/ruby-2.3.1@reporting/gems/survey-gizmo-ruby-6.1.1/lib/survey_gizmo/resource.rb:65:in `merge'

Here is the debug output.

The contact in this situation belongs to the specified survey, but it is the wrong campaign. I can resubmit with a raise if you prefer, let me know what you think!

@apurvis
Copy link
Collaborator

apurvis commented May 25, 2016

ugh yeah that sucks.

  • i would write to surveygizmo support and complain, because that just seems flat out wrong
  • and yeah for the purposes of this gem, we should raise an exception. seems like it might be safer to use the total_pages: 0 and total_count:null than to try to situationally figure out when it would or wouldn't make sense for data to be []

@jarthod
Copy link
Collaborator

jarthod commented May 25, 2016

Ok they suck, as always. But with your fix in this case what would be returned by .first, an empty Contact instance isn't it ? This is not really what you expect, it should either raise or return nil IMO.
WDYT @apurvis ?

@apurvis
Copy link
Collaborator

apurvis commented May 25, 2016

we should raise an exception

@apurvis
Copy link
Collaborator

apurvis commented May 25, 2016

i would probably put the exception in the response parser - basically i don't think total_pages should ever be null.

but also make sure you add a spec for this behavior.

@Kooshaba
Copy link
Author

Ok they suck, as always.

Glad we agree haha.

I'll resubmit with an exception in a bit. Thanks!

@apurvis
Copy link
Collaborator

apurvis commented Jul 19, 2016

Closing this PR because this isn't getting merged in this form, but opened an issue about it if someone wants to fix it in a more universal way #81

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.

3 participants