From 294e83e3d69e3da8dbced2059323d8680349f548 Mon Sep 17 00:00:00 2001 From: Fuad Muhic Date: Wed, 31 Jan 2018 14:36:57 +0100 Subject: [PATCH] Reworkder HttpRequest class to use RETRY functionality Arvados-DCO-1.1-Signed-off-by: Fuad Muhic --- sdk/R/R/HttpRequest.R | 63 +++---------- sdk/R/R/RESTService.R | 49 +++++----- sdk/R/tests/testthat/fakes/FakeHttpRequest.R | 97 +++++++------------- sdk/R/tests/testthat/test-HttpRequest.R | 7 ++ 4 files changed, 75 insertions(+), 141 deletions(-) diff --git a/sdk/R/R/HttpRequest.R b/sdk/R/R/HttpRequest.R index ec364e5420..53c8e224a8 100644 --- a/sdk/R/R/HttpRequest.R +++ b/sdk/R/R/HttpRequest.R @@ -7,67 +7,26 @@ HttpRequest <- R6::R6Class( public = list( validContentTypes = NULL, + validVerbs = NULL, initialize = function() { self$validContentTypes <- c("text", "raw") + self$validVerbs <- c("GET", "POST", "PUT", "DELETE", "PROPFIND", "MOVE") }, - GET = function(url, headers = NULL, queryFilters = NULL, limit = NULL, offset = NULL) + execute = function(verb, url, headers = NULL, body = NULL, query = NULL, + limit = NULL, offset = NULL, retryTimes = 3) { - headers <- httr::add_headers(unlist(headers)) - query <- self$createQuery(queryFilters, limit, offset) - url <- paste0(url, query) + if(!(verb %in% self$validVerbs)) + stop("Http verb is not valid.") - serverResponse <- httr::GET(url = url, config = headers) - }, - - PUT = function(url, headers = NULL, body = NULL, - queryFilters = NULL, limit = NULL, offset = NULL) - { - headers <- httr::add_headers(unlist(headers)) - query <- self$createQuery(queryFilters, limit, offset) - url <- paste0(url, query) - - serverResponse <- httr::PUT(url = url, config = headers, body = body) - }, - - POST = function(url, headers = NULL, body = NULL, - queryFilters = NULL, limit = NULL, offset = NULL) - { - headers <- httr::add_headers(unlist(headers)) - query <- self$createQuery(queryFilters, limit, offset) - url <- paste0(url, query) - - serverResponse <- httr::POST(url = url, config = headers, body = body) - }, - - DELETE = function(url, headers = NULL, body = NULL, - queryFilters = NULL, limit = NULL, offset = NULL) - { - headers <- httr::add_headers(unlist(headers)) - query <- self$createQuery(queryFilters, limit, offset) - url <- paste0(url, query) - - serverResponse <- httr::DELETE(url = url, config = headers) - }, - - PROPFIND = function(url, headers = NULL) - { - h <- curl::new_handle() - curl::handle_setopt(h, customrequest = "PROPFIND") - curl::handle_setheaders(h, .list = headers) - - propfindResponse <- curl::curl_fetch_memory(url, h) - }, - - MOVE = function(url, headers = NULL) - { - h <- curl::new_handle() - curl::handle_setopt(h, customrequest = "MOVE") - curl::handle_setheaders(h, .list = headers) + headers <- httr::add_headers(unlist(headers)) + urlQuery <- self$createQuery(query, limit, offset) + url <- paste0(url, urlQuery) - propfindResponse <- curl::curl_fetch_memory(url, h) + response <- httr::RETRY(verb, url = url, body = body, + config = headers, times = retryTimes) }, createQuery = function(filters, limit, offset) diff --git a/sdk/R/R/RESTService.R b/sdk/R/R/RESTService.R index 62356b7638..74031b00c3 100644 --- a/sdk/R/R/RESTService.R +++ b/sdk/R/R/RESTService.R @@ -4,10 +4,10 @@ RESTService <- R6::R6Class( public = list( - hostName = NULL, - token = NULL, - http = NULL, - httpParser = NULL, + hostName = NULL, + token = NULL, + http = NULL, + httpParser = NULL, initialize = function(token, hostName, webDavHostName = NULL, http, httpParser) { @@ -32,7 +32,7 @@ RESTService <- R6::R6Class( headers <- list(Authorization = paste("OAuth2", self$token)) - serverResponse <- self$http$GET(discoveryDocumentURL, headers) + serverResponse <- self$http$execute("GET", discoveryDocumentURL, headers) discoveryDocument <- self$httpParser$parseJSONResponse(serverResponse) private$webDavHostName <- discoveryDocument$keepWebServiceUrl @@ -49,7 +49,7 @@ RESTService <- R6::R6Class( resourceURL <- paste0(self$hostName, resource, "/", uuid) headers <- list(Authorization = paste("OAuth2", self$token)) - serverResponse <- self$http$GET(resourceURL, headers) + serverResponse <- self$http$execute("GET", resourceURL, headers) resource <- self$httpParser$parseJSONResponse(serverResponse) @@ -63,9 +63,10 @@ RESTService <- R6::R6Class( { resourceURL <- paste0(self$hostName, resource) headers <- list(Authorization = paste("OAuth2", self$token)) + body <- NULL - serverResponse <- self$http$GET(resourceURL, headers, filters, - limit, offset) + serverResponse <- self$http$execute("GET", resourceURL, headers, body, + filters, limit, offset) resources <- self$httpParser$parseJSONResponse(serverResponse) @@ -84,11 +85,13 @@ RESTService <- R6::R6Class( items <- c() while(length(items) < itemsAvailable) { - serverResponse <- self$http$GET(url = resourceURL, - headers = headers, - queryFilters = filters, - limit = NULL, - offset = offset) + serverResponse <- self$http$execute(verb = "GET", + url = resourceURL, + headers = headers, + body = NULL, + query = filters, + limit = NULL, + offset = offset) parsedResponse <- self$httpParser$parseJSONResponse(serverResponse) @@ -109,7 +112,7 @@ RESTService <- R6::R6Class( headers <- list("Authorization" = paste("OAuth2", self$token), "Content-Type" = "application/json") - serverResponse <- self$http$DELETE(collectionURL, headers) + serverResponse <- self$http$execute("DELETE", collectionURL, headers) removedResource <- self$httpParser$parseJSONResponse(serverResponse) @@ -127,7 +130,7 @@ RESTService <- R6::R6Class( newContent <- jsonlite::toJSON(newContent, auto_unbox = T) - serverResponse <- self$http$PUT(resourceURL, headers, newContent) + serverResponse <- self$http$execute("PUT", resourceURL, headers, newContent) updatedResource <- self$httpParser$parseJSONResponse(serverResponse) @@ -145,7 +148,7 @@ RESTService <- R6::R6Class( content <- jsonlite::toJSON(content, auto_unbox = T) - serverResponse <- self$http$POST(resourceURL, headers, content) + serverResponse <- self$http$execute("POST", resourceURL, headers, content) newResource <- self$httpParser$parseJSONResponse(serverResponse) @@ -169,7 +172,7 @@ RESTService <- R6::R6Class( uuid, "/", relativePath); headers <- list(Authorization = paste("OAuth2", self$token)) - serverResponse <- self$http$DELETE(fileURL, headers) + serverResponse <- self$http$execute("DELETE", fileURL, headers) if(serverResponse$status_code < 200 || serverResponse$status_code >= 300) stop(paste("Server code:", serverResponse$status_code)) @@ -186,7 +189,7 @@ RESTService <- R6::R6Class( headers <- list("Authorization" = paste("OAuth2", self$token), "Destination" = toURL) - serverResponse <- self$http$MOVE(fromURL, headers) + serverResponse <- self$http$execute("MOVE", fromURL, headers) if(serverResponse$status_code < 200 || serverResponse$status_code >= 300) stop(paste("Server code:", serverResponse$status_code)) @@ -201,7 +204,7 @@ RESTService <- R6::R6Class( headers <- list("Authorization" = paste("OAuth2", self$token)) - response <- self$http$PROPFIND(collectionURL, headers) + response <- self$http$execute("PROPFIND", collectionURL, headers) if(all(response == "")) stop("Response is empty, request may be misconfigured") @@ -221,7 +224,7 @@ RESTService <- R6::R6Class( headers <- list("Authorization" = paste("OAuth2", self$token)) - response <- self$http$PROPFIND(subcollectionURL, headers) + response <- self$http$execute("PROPFIND", subcollectionURL, headers) if(all(response == "")) stop("Response is empty, request may be misconfigured") @@ -257,7 +260,7 @@ RESTService <- R6::R6Class( if(!(contentType %in% self$httpParser$validContentTypes)) stop("Invalid contentType. Please use text or raw.") - serverResponse <- self$http$GET(fileURL, headers) + serverResponse <- self$http$execute("GET", fileURL, headers) if(serverResponse$status_code < 200 || serverResponse$status_code >= 300) stop(paste("Server code:", serverResponse$status_code)) @@ -273,7 +276,7 @@ RESTService <- R6::R6Class( "Content-Type" = contentType) body <- content - serverResponse <- self$http$PUT(fileURL, headers, body) + serverResponse <- self$http$execute("PUT", fileURL, headers, body) if(serverResponse$status_code < 200 || serverResponse$status_code >= 300) stop(paste("Server code:", serverResponse$status_code)) @@ -309,7 +312,7 @@ RESTService <- R6::R6Class( "Content-Type" = contentType) body <- NULL - serverResponse <- self$http$PUT(fileURL, headers, body) + serverResponse <- self$http$execute("PUT", fileURL, headers, body) if(serverResponse$status_code < 200 || serverResponse$status_code >= 300) stop(paste("Server code:", serverResponse$status_code)) diff --git a/sdk/R/tests/testthat/fakes/FakeHttpRequest.R b/sdk/R/tests/testthat/fakes/FakeHttpRequest.R index 34dde62e78..c46e03bae9 100644 --- a/sdk/R/tests/testthat/fakes/FakeHttpRequest.R +++ b/sdk/R/tests/testthat/fakes/FakeHttpRequest.R @@ -36,85 +36,53 @@ FakeHttpRequest <- R6::R6Class( else self$content <- serverResponse - self$expectedURL <- expectedURL - self$URLIsProperlyConfigured <- FALSE - self$expectedQueryFilters <- expectedFilters - self$queryFiltersAreCorrect <- FALSE + self$expectedURL <- expectedURL + self$URLIsProperlyConfigured <- FALSE + self$expectedQueryFilters <- expectedFilters + self$queryFiltersAreCorrect <- FALSE self$requestHeaderContainsAuthorizationField <- FALSE - self$requestHeaderContainsDestinationField <- FALSE - self$requestHeaderContainsRangeField <- FALSE - self$requestHeaderContainsContentTypeField <- FALSE - self$JSONEncodedBodyIsProvided <- FALSE - self$requestBodyIsProvided <- FALSE + self$requestHeaderContainsDestinationField <- FALSE + self$requestHeaderContainsRangeField <- FALSE + self$requestHeaderContainsContentTypeField <- FALSE + self$JSONEncodedBodyIsProvided <- FALSE + self$requestBodyIsProvided <- FALSE - self$numberOfGETRequests <- 0 + self$numberOfGETRequests <- 0 self$numberOfDELETERequests <- 0 - self$numberOfPUTRequests <- 0 - self$numberOfPOSTRequests <- 0 - self$numberOfMOVERequests <- 0 + self$numberOfPUTRequests <- 0 + self$numberOfPOSTRequests <- 0 + self$numberOfMOVERequests <- 0 self$serverMaxElementsPerRequest <- 5 }, - GET = function(url, headers = NULL, queryFilters = NULL, limit = NULL, offset = NULL) + execute = function(verb, url, headers = NULL, body = NULL, query = NULL, + limit = NULL, offset = NULL, retryTimes = 3) { private$validateURL(url) private$validateHeaders(headers) private$validateFilters(queryFilters) - self$numberOfGETRequests <- self$numberOfGETRequests + 1 + private$validateBody(body) - if(!is.null(self$content$items_available)) + if(verb == "GET") + self$numberOfGETRequests <- self$numberOfGETRequests + 1 + else if(verb == "POST") + self$numberOfPOSTRequests <- self$numberOfPOSTRequests + 1 + else if(verb == "PUT") + self$numberOfPUTRequests <- self$numberOfPUTRequests + 1 + else if(verb == "DELETE") + self$numberOfDELETERequests <- self$numberOfDELETERequests + 1 + else if(verb == "MOVE") + self$numberOfMOVERequests <- self$numberOfMOVERequests + 1 + else if(verb == "PROPFIND") { - return(private$getElements(offset, limit)) + return(self$content) } + + if(!is.null(self$content$items_available)) + return(private$getElements(offset, limit)) else return(self$content) - }, - - PUT = function(url, headers = NULL, body = NULL, - queryFilters = NULL, limit = NULL, offset = NULL) - { - private$validateURL(url) - private$validateHeaders(headers) - private$validateBody(body) - self$numberOfPUTRequests <- self$numberOfPUTRequests + 1 - - self$content - }, - - POST = function(url, headers = NULL, body = NULL, - queryFilters = NULL, limit = NULL, offset = NULL) - { - private$validateURL(url) - private$validateHeaders(headers) - private$validateBody(body) - self$numberOfPOSTRequests <- self$numberOfPOSTRequests + 1 - - self$content - }, - - DELETE = function(url, headers = NULL, body = NULL, - queryFilters = NULL, limit = NULL, offset = NULL) - { - private$validateURL(url) - private$validateHeaders(headers) - self$numberOfDELETERequests <- self$numberOfDELETERequests + 1 - self$content - }, - - PROPFIND = function(url, headers = NULL) - { - private$validateURL(url) - private$validateHeaders(headers) - self$content - }, - - MOVE = function(url, headers = NULL) - { - private$validateURL(url) - private$validateHeaders(headers) - self$numberOfMOVERequests <- self$numberOfMOVERequests + 1 - self$content } ), @@ -143,9 +111,6 @@ FakeHttpRequest <- R6::R6Class( validateBody = function(body) { - if(!is.null(body) && class(body) == "json") - self$JSONEncodedBodyIsProvided <- TRUE - if(!is.null(body)) { self$requestBodyIsProvided <- TRUE diff --git a/sdk/R/tests/testthat/test-HttpRequest.R b/sdk/R/tests/testthat/test-HttpRequest.R index e85037465b..66ab9af196 100644 --- a/sdk/R/tests/testthat/test-HttpRequest.R +++ b/sdk/R/tests/testthat/test-HttpRequest.R @@ -1,6 +1,13 @@ context("Http Request") +test_that("execyte raises exception if http verb is not valid", { + + http <- HttpRequest$new() + expect_that(http$execute("FAKE VERB", "url"), + throws_error("Http verb is not valid.")) +}) + test_that(paste("createQuery generates and encodes query portion of http", "request based on filters, limit and offset parameters"), { -- 2.39.5