dmpe / urlshorteneR

R package for 2 URL shortening service :loudspeaker: Compatible with Bitly API v4
http://cran.r-project.org/package=urlshorteneR
Apache License 2.0
22 stars 7 forks source link

bitly returns "Status 200" even if the "longUrl" argument is missing #9

Closed patperu closed 8 years ago

patperu commented 8 years ago

Hello,

first of all - I'm not an expert with APIs.... :smirk: I struggled a little bit trying to figure out, how to catch the response of bitly_LinksShorten if I call the function with an empty longUrl argument

bitly_LinksShorten("")
Dataframe mit 0 Spalten und 1 Zeile

To catch this, I have to do something like is(ncol == 0), or to check the input before of course. But it seems that the 'problem' is bitly itself. Here I'm using the GET-request from your code and set longUrl to "".

query <- list(access_token = bitly_token$credentials$access_token, longUrl = "")
return_request <- httr::GET("https://api-ssl.bitly.com/v3/shorten", query = query, httr::config(token = bitly_token))

> return_request
Response [https://api-ssl.bitly.com/v3/shorten?access_token=207b9e68ff2d50a113b21bc5af78280f4550cc41&longUrl=]
  Date: 2016-02-02 16:46
  Status: 200
  Content-Type: application/json; charset=utf-8
  Size: 68 B

I would not expect a "Status 200" here and if I call the content object I get a Status 500 with a proper text message.

> httr::content(return_request)
$data
list()

$status_code
[1] 500

$status_txt
[1] "MISSING_ARG_URI"

Maybe you could add a check somewhere around here and return NA if the status is not 200. https://github.com/dmpe/urlshorteneR/blob/master/R/ApiKey.R#L92

And yes, if you want, I could try to write a pull request.

Thank you for the package! Ciao Patrick

dmpe commented 8 years ago

Hi, Do it ! :+1: I will definitelly take a look. Please, take into consideration this http://blog.rstudio.org/2016/02/02/httr-1-1-0/

So, meaning update httr if needed too.

patperu commented 8 years ago

Uff... ok, will do :smirk:

dmpe commented 8 years ago

yeee, great! just as said: take into consideration nothing more nothing less. I usually bump packages (almost no matter what)

dmpe commented 8 years ago

http_error() replaces url_ok() and url_successful(). http_error() more clearly conveys intent and works with urls, responses and status codes.

seems particularly interesting

patperu commented 8 years ago

Great, thanks! I'll take a look.

dmpe commented 8 years ago

BTW: I am already working on it BUT

I would not expect a "Status 200" here

makes 100% sense and actually I believe that it is their problem which - I - have to workaround somehow, now. It wouldn't be my problem if their design would return 500 automatically.

dmpe commented 8 years ago

Ok, oone more question:

I struggled a little bit trying to figure out, how to catch the response of bitly_LinksShorten if I call the function with an empty longUrl argument

But why would you do that? Do you a use case for it ?

dmpe commented 8 years ago

Hi @patperu,

Please, can you test the current package version from github: devtools::install_github("dmpe/urlshorteneR)

Thanks. In anycase, please reopen this issue if you will still see the problems with your case As I plan to submit it soon to CRAN :)

patperu commented 8 years ago

Hi John, sorry for the silence!! I will test the new version today! Ciao Patrick

dmpe commented 8 years ago

:)

patperu commented 8 years ago

Hello John,

thank you very much for looking into this issue. My use case is simple: I'm scaping webpages (e.g. http://albo.studiok.it/bussoleno/albo/ ) and most of the time there is link field, but not always. So, the data.frame contains missing values in the link column. Of course, it would be easy to shorten only the rows with an entry.... but I just want to throw the column in a function and get back a vector of the same length with NA for the missing entries. When I call bitly_LinksShorten (current verison) with an URL I'm getting back a nice data.frame.

(res <- bitly_LinksShorten(longUrl = "http://www.opencoesione.gov.it/progetti/1si366/"))
#                                         long_url                   url    hash global_hash new_hash
# 1 http://www.opencoesione.gov.it/progetti/1si366/ http://bit.ly/1PlgCM5 1PlgCM5     1PlgCM6        0

Calling the function in a (stupid way!) without an argument it's a little bit tricky, because

bitly_LinksShorten(longUrl = "")
MISSING_ARG_URI 
Error in doRequest("GET", links_shorten_url, "bitly", query, showURL = showRequestURL) : 
  Internal Server Error (HTTP 500).

now it 'breaks' and my solution to catch this is way too complicated... catch_empty_longUrl.txt

Hmm, I'm not sure how to deal with this. I think I like the old version more, because it was easier to catch the error. But the problem is the empty call, not the error... :smirk: So simply don't call the function with a missing argument. What do you think?

dmpe commented 8 years ago

now it 'breaks' and my solution to catch this is way too complicated...

well, it should break - this is what i expect from empty argument...

So simply don't call the function with a missing argument.

......yes.....................but now i also understand what you want to do and yes that would be great feature indeed.

yeeh, will need to think about that more... PS:Pull Requests are welcome for it :)

df <- data.frame(pubDate = rep("2016-02-10", 4),
                 link = c(NA,
                          "http://www.opencoesione.gov.it/progetti/1misepac01_000443/",
                          "http://www.opencoesione.gov.it/progetti/1misepac01_000031/",
                          ""),
                 stringsAsFactors = FALSE)
df

err_df <- function(x) {
  data.frame(long_url = x,
             url = NA,
             hash = NA,
             global_hash = NA,
             new_hash = NA)}

shorten_bitly <- function(x) {
  res <- lapply(as.list(x), function(x) {
    tryCatch(bitly_LinksShorten(longUrl = x),
             error = function(e) err_df(x))
  })
  res <- do.call("rbind", res)
}

fin <- shorten_bitly(df$link)

fin
patperu commented 8 years ago

Ha, I like the new label. And I'll think about a solution and a pull request! Ah, maybe @sckott has an idea... :smirk:

sckott commented 8 years ago

i can try - what exactly is the problem?

patperu commented 8 years ago

Great! :smile:

Ok,... The function bitly_LinksShorten returns a dataframe like this

(res <- bitly_LinksShorten(longUrl = "http://www.opencoesione.gov.it/progetti/1si366/"))
#                                         long_url                   url    hash global_hash new_hash
# 1 http://www.opencoesione.gov.it/progetti/1si366/ http://bit.ly/1PlgCM5 1PlgCM5     1PlgCM6        0

Now are thinking about a 'solution' what the return(?) could be when calling the function with a missing argument.

patperu commented 8 years ago

BTW bitly returns "Status: 200" in this case (see first post).

sckott commented 8 years ago

So I'd change the fail behavior slightly, here https://github.com/dmpe/urlshorteneR/blob/master/R/ApiKey.R#L96-L97 you have

cat(json_response$status_txt, "\n")
stop_for_status(json_response$status_code)

I'd change that to

stop(sprintf("(%s) - %s", json_response$status_code, json_response$status_txt), call. = FALSE)

so that gives

bitly_LinksShorten(longUrl = "")
#> Error: (500) - MISSING_ARG_URI 
dmpe commented 8 years ago

Hi, @patperu, I have already commited sckotts suggestion. Please test it out.

What you are talking about is achievable but will tak long time until i get it.

dmpe commented 8 years ago

One option would have something like

  if (is.null(longUrl) || is.na(longUrl) || nzchar(longUrl) == FALSE) {
    stop("you should a have proper URL, not NULL, NA and empty string. First get rid of them")
  }

Another one of course, that it would skip those rows which do not have a proper URL

> for (p in 1:length(df$link)) {
+  fin[[p]] <- bitly_LinksShorten(longUrl = df$link[p])
+ }
Code: 500 - INVALID_URI
Code: 500 - MISSING_ARG_URI

I have also replaced stop with a message, now this will not stop the code flow.

    if (is.null(json_response$status_code) == FALSE && json_response$status_code >= 400) {
      message(sprintf("Code: %s - %s", json_response$status_code, json_response$status_txt))
    }
patperu commented 8 years ago

Hi John, you are releasing fast :) I'm now using dmpe/urlshorteneR@01c602a. I like it, that the code flow will not stop. This is what I get:

for (p in 1:length(df$link)) {
  fin[[p]] <- bitly_LinksShorten(longUrl = df$link[p])
}
Code: 500 - INVALID_URI
Code: 500 - MISSING_ARG_URI

fin
[[1]]
Dataframe mit 0 Spalten und 1 Zeile

[[2]]
long_url                   url    hash global_hash new_hash
1 http://www.opencoesione.gov.it/progetti/1misepac01_000443/ http://bit.ly/1T7D0Ld 1T7D0Ld     1T7D0Le        0

[[3]]
long_url                   url    hash global_hash new_hash
1 http://www.opencoesione.gov.it/progetti/1misepac01_000031/ http://bit.ly/1T7D2mq 1T7D2mq     1T7D2mr        0

[[4]]
Dataframe mit 0 Spalten und 1 Zeile

That's ok. Maybe it would be helpful to describe in the helpfile what you get back in case of an error (a message and a data.frame with 0 col and 1 row). And please don't take my wish too 'serious'. I could easily 'get rid of' the missing entries and match the result later. :)

dmpe commented 8 years ago

Fine. I will add some more comments to the vignette (where it is at best) and then later send it to CRAN. Thanks @sckott as well

patperu commented 8 years ago

Ok! Thanks for your hard work!

sckott commented 8 years ago

sure thing

patperu commented 8 years ago

Ups, sorry @sckott !! Of course a big Thank you! too!