From 20006829f60be5fbb31b77005ee0170d86c24543 Mon Sep 17 00:00:00 2001 From: James McMahon Date: Thu, 26 Sep 2024 16:33:15 +0100 Subject: [PATCH 1/7] tidy doc --- R/get_resource_sql.R | 3 ++- man/get_resource_sql.Rd | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/R/get_resource_sql.R b/R/get_resource_sql.R index 0df6ba0..01cf693 100644 --- a/R/get_resource_sql.R +++ b/R/get_resource_sql.R @@ -25,7 +25,8 @@ #' #' ```sql = "SELECT * FROM \"\" WHERE \"Age\" = '34'"```. #' -#' @seealso [get_resource()] for downloading a resource without using a SQL query. +#' @seealso [get_resource()] for downloading a resource without using a +#' SQL query. #' #' @return a [tibble][tibble::tibble-package] with the query results. #' Only 32,000 rows can be returned from a single SQL query. diff --git a/man/get_resource_sql.Rd b/man/get_resource_sql.Rd index 4f2d4cd..a0d16ee 100644 --- a/man/get_resource_sql.Rd +++ b/man/get_resource_sql.Rd @@ -59,5 +59,6 @@ df2 <- get_resource( ) } \seealso{ -\code{\link[=get_resource]{get_resource()}} for downloading a resource without using a SQL query. +\code{\link[=get_resource]{get_resource()}} for downloading a resource without using a +SQL query. } From d28601a21623f982d30dd02615025f361cb997c9 Mon Sep 17 00:00:00 2001 From: James McMahon Date: Thu, 26 Sep 2024 17:37:47 +0100 Subject: [PATCH 2/7] Improve error checking and messages --- R/get_resource_sql.R | 20 +++++++++----------- tests/testthat/test-get_resource_sql.R | 6 +++--- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/R/get_resource_sql.R b/R/get_resource_sql.R index 01cf693..5fb3fb3 100644 --- a/R/get_resource_sql.R +++ b/R/get_resource_sql.R @@ -56,19 +56,17 @@ #' row_filters = row_filter #' ) get_resource_sql <- function(sql) { - if (length(sql) > 1) { + if (length(sql) != 1) { cli::cli_abort(c( - "SQL validation error.", - i = "{.var sql} must be of length 1", - x = "You entered an object of length {length(sql)}." + x = "SQL validation error.", + i = "{.var sql} must be length 1 not {length(sql)}." )) } - if (!("character" %in% class(sql))) { + if (!inherits(sql, "character")) { cli::cli_abort(c( - "SQL validation error.", - i = "{.var sql} must be of class {.cls character}", - x = "You entered an object of class {.cls {class(sql)[1]}}." + x = "SQL validation error.", + i = "{.var sql} must be of class {.cls character} not {.cls {class(sql)}}." )) } @@ -77,10 +75,10 @@ get_resource_sql <- function(sql) { sql <- gsub("\n", "", sql) # check query is a SELECT statement - if (substr(sql, 1, 6) != "SELECT") { + if (!grepl("^\\s*?SELECT", sql)) { cli::cli_abort(c( - "SQL validation error.", - i = "{.var sql} must start with SELECT" + x = "SQL validation error.", + i = "{.var sql} must start with {.val SELECT}" )) } diff --git a/tests/testthat/test-get_resource_sql.R b/tests/testthat/test-get_resource_sql.R index 734ab60..bb161e4 100644 --- a/tests/testthat/test-get_resource_sql.R +++ b/tests/testthat/test-get_resource_sql.R @@ -4,19 +4,19 @@ test_that("throws errors on invalid sql argument", { # wrong class expect_error( get_resource_sql(9000), - regexp = "You entered an object of class " + regexp = "must be of class" ) # wrong length expect_error( get_resource_sql(letters), - regexp = "You entered an object of length 26." + regexp = "must be length 1 not 26\\." ) # wrong start expect_error( get_resource_sql("this is wrong"), - regexp = "`sql` must start with SELECT" + regexp = "`sql` must start with" ) }) From 54399a05bb9c9fa94837d55b050257082c0b5c5b Mon Sep 17 00:00:00 2001 From: James McMahon Date: Thu, 26 Sep 2024 17:39:33 +0100 Subject: [PATCH 3/7] Don't remove spaces from SQL strings This is no longer needed thanks to #25 --- R/get_resource_sql.R | 4 ---- 1 file changed, 4 deletions(-) diff --git a/R/get_resource_sql.R b/R/get_resource_sql.R index 5fb3fb3..41d445d 100644 --- a/R/get_resource_sql.R +++ b/R/get_resource_sql.R @@ -70,10 +70,6 @@ get_resource_sql <- function(sql) { )) } - # remove spaces - sql <- gsub(" ", "", sql) - sql <- gsub("\n", "", sql) - # check query is a SELECT statement if (!grepl("^\\s*?SELECT", sql)) { cli::cli_abort(c( From 750ef0b5f3f4c92b66ef5164c8bce2c938453c6f Mon Sep 17 00:00:00 2001 From: James McMahon Date: Thu, 26 Sep 2024 17:42:59 +0100 Subject: [PATCH 4/7] Improve comment --- R/get_resource_sql.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/get_resource_sql.R b/R/get_resource_sql.R index 41d445d..7c10d92 100644 --- a/R/get_resource_sql.R +++ b/R/get_resource_sql.R @@ -78,7 +78,7 @@ get_resource_sql <- function(sql) { )) } - # add query field prefix + # Add the SQL statement to the query query <- list("sql" = sql) # attempt get request From 63c7e785607ffa5d00a066c9f1ed31c696c15b11 Mon Sep 17 00:00:00 2001 From: James McMahon Date: Thu, 26 Sep 2024 17:43:42 +0100 Subject: [PATCH 5/7] Add a check and warning for large queries --- R/get_resource_sql.R | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/R/get_resource_sql.R b/R/get_resource_sql.R index 7c10d92..79c76bf 100644 --- a/R/get_resource_sql.R +++ b/R/get_resource_sql.R @@ -84,6 +84,13 @@ get_resource_sql <- function(sql) { # attempt get request content <- phs_GET("datastore_search_sql", query) + if (!is.null(content[["result"]][["records_truncated"]])) { + cli::cli_warn( + "The data was truncated because your query matched more than the + maximum number of rows." + ) + } + # get correct order of columns order <- purrr::map_chr( content$result$fields, From bf8da7b86dcc595ca1327f5c351d0a4da539af6b Mon Sep 17 00:00:00 2001 From: James McMahon Date: Thu, 26 Sep 2024 17:53:18 +0100 Subject: [PATCH 6/7] Add test for joined query --- tests/testthat/test-get_resource_sql.R | 30 ++++++++++++++++++++------ 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/tests/testthat/test-get_resource_sql.R b/tests/testthat/test-get_resource_sql.R index bb161e4..82152d8 100644 --- a/tests/testthat/test-get_resource_sql.R +++ b/tests/testthat/test-get_resource_sql.R @@ -20,8 +20,9 @@ test_that("throws errors on invalid sql argument", { ) }) -test_that("gets expected data", { - sql <- " +test_that("gets expected data for a simple SQL query", { + data <- get_resource_sql( + sql = " SELECT \"TotalCancelled\",\"TotalOperations\",\"Hospital\",\"Month\" FROM @@ -29,12 +30,27 @@ test_that("gets expected data", { WHERE \"Hospital\" = 'D102H' " - df <- get_resource_sql(sql) + ) + + expect_s3_class(data, "tbl") + expect_equal(unique(data$Hospital), "D102H") + expect_named(data, c("TotalCancelled", "TotalOperations", "Hospital", "Month")) +}) + +test_that("gets expected data for a joined SQL query", { + data <- get_resource_sql( + sql = paste( + "SELECT pops.\"Year\", pops.\"HB\", lookup.\"HBName\", pops.\"AllAges\"", + "FROM \"27a72cc8-d6d8-430c-8b4f-3109a9ceadb1\" AS pops", + "JOIN \"652ff726-e676-4a20-abda-435b98dd7bdc\" AS lookup", + "ON pops.\"HB\" = lookup.\"HB\"", + "WHERE pops.\"Sex\" = 'All' AND pops.\"Year\" > 2006" + ) + ) - expect_equal(unique(df$Hospital), "D102H") - expect_equal( - c("TotalCancelled", "TotalOperations", "Hospital", "Month"), - names(df) + expect_s3_class(data, "tbl") + expect_gt(min(as.integer(data$Year)), 2006L) + expect_named(data,c("Year", "HB", "HBName", "AllAges") ) }) From a1369c64c4b188df5fca0c73460d24ca47d9c573 Mon Sep 17 00:00:00 2001 From: Moohan Date: Thu, 26 Sep 2024 16:54:46 +0000 Subject: [PATCH 7/7] Style code (GHA) --- tests/testthat/test-get_resource_sql.R | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/testthat/test-get_resource_sql.R b/tests/testthat/test-get_resource_sql.R index 82152d8..8c02002 100644 --- a/tests/testthat/test-get_resource_sql.R +++ b/tests/testthat/test-get_resource_sql.R @@ -50,8 +50,7 @@ test_that("gets expected data for a joined SQL query", { expect_s3_class(data, "tbl") expect_gt(min(as.integer(data$Year)), 2006L) - expect_named(data,c("Year", "HB", "HBName", "AllAges") - ) + expect_named(data, c("Year", "HB", "HBName", "AllAges")) }) test_that("SQL errors", {