From 0da63581f1ca89e05ac52566b82f9344708b2b2f Mon Sep 17 00:00:00 2001 From: Ian Lyttle Date: Sat, 3 Feb 2024 13:53:42 -0700 Subject: [PATCH] refactor: update helper for box id (#264) * build helper and test * bump version and add to NEWS * switch to as_box_id() * remove box_id(), {bit64} dependency * add to exported functions * add to more exported functions * add to remaining exported functions * add trailing newline * fix missed conflict --- DESCRIPTION | 4 ++-- NEWS.md | 2 ++ R/boxr__internal_get.R | 2 +- R/boxr__internal_misc.R | 30 ++++++++++++++++++++++++------ R/boxr__internal_update_upload.R | 13 ++++++++++--- R/boxr_add_description.R | 2 ++ R/boxr_collab.R | 15 ++++++++++++++- R/boxr_comment.R | 3 +++ R/boxr_delete_restore.R | 18 ++++++++++++++++++ R/boxr_dir_verbs.R | 3 +++ R/boxr_file_versions.R | 8 +++++++- R/boxr_misc.R | 25 ++++++++++++++++++------- R/boxr_read.R | 6 ++++++ R/boxr_save_load.R | 2 ++ R/boxr_search.R | 6 ++++-- R/boxr_upload_download.R | 3 ++- R/boxr_write.R | 4 ++++ tests/testthat/test_00_internal.R | 14 +++++++++++++- 18 files changed, 135 insertions(+), 25 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 6718671f..042ddb47 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Package: boxr Type: Package Title: Interface for the 'Box.com API' -Version: 0.3.6.9012 +Version: 0.3.6.9013 Authors@R: c( person("Brendan", "Rocks", email = "foss@brendanrocks.com", role = c("aut")), @@ -34,7 +34,6 @@ Description: An R interface for the remote file hosting service 'Box' License: MIT + file LICENSE Imports: assertthat, - bit64, dplyr, digest, fs, @@ -52,6 +51,7 @@ Imports: lifecycle, jsonlite, jose, + cli, withr Suggests: clipr (>= 0.3.0), diff --git a/NEWS.md b/NEWS.md index 95632699..c6dd7ce0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -6,6 +6,8 @@ ## Internal +* update with internal helper for Box ids. (#90) + * update minimum version of rio package to reflect newer treatment of JSON files. (#261) * remove unused internal function, removing dependency on httpuv package. (#259) diff --git a/R/boxr__internal_get.R b/R/boxr__internal_get.R index c6327d81..6d08f446 100644 --- a/R/boxr__internal_get.R +++ b/R/boxr__internal_get.R @@ -14,7 +14,7 @@ handle_file_id <- function(obj) { file_id <- obj } # Return -- if valid - return(box_id(file_id)) + return(as_box_id(file_id)) } diff --git a/R/boxr__internal_misc.R b/R/boxr__internal_misc.R index 19d563e4..9e507563 100644 --- a/R/boxr__internal_misc.R +++ b/R/boxr__internal_misc.R @@ -19,15 +19,33 @@ box_filename <- function(x) { x } +# ref: https://rlang.r-lib.org/reference/topic-error-call.html +as_box_id <- function(x, arg = rlang::caller_arg(x), + call = rlang::caller_env()) { + + # a box_id is a string that contains only digits + + # some defaults are NULL; we just pass these through + if (is.null(x)) { + return(NULL) + } -# Validate ids supplied -box_id <- function(x) { - if (!is.null(x) && any(is.na(bit64::as.integer64(x)))) - stop("box.com API ids must be (coercible to) 64-bit integers") - if (!is.null(x)) - return(as.character(bit64::as.integer64(x))) + id <- as.character(x) + + has_only_digits <- stringr::str_detect(id, "^\\d+$") + + if (!all(has_only_digits)) { + cli::cli_abort( + message = "{.arg {arg}} must contain only digits", + class = "boxr_id", + call = call + ) + } + + id } + # helper to identify void values is_void <- function(x) { is.null(x) || diff --git a/R/boxr__internal_update_upload.R b/R/boxr__internal_update_upload.R index ec393cde..59cb1b6b 100644 --- a/R/boxr__internal_update_upload.R +++ b/R/boxr__internal_update_upload.R @@ -18,6 +18,9 @@ #' @keywords internal #' box_upload_new <- function(dir_id, file, pb = FALSE) { + + dir_id <- as_box_id(dir_id) + httr::RETRY( "POST", "https://upload.box.com/api/2.0/files/content", @@ -29,7 +32,7 @@ box_upload_new <- function(dir_id, file, pb = FALSE) { list( attributes = paste0( - '{"name": "', basename(file), '", "parent": {"id":"', box_id(dir_id) + '{"name": "', basename(file), '", "parent": {"id":"', dir_id ,'"}}' ), file = httr::upload_file(file) @@ -41,9 +44,13 @@ box_upload_new <- function(dir_id, file, pb = FALSE) { #' @rdname box_upload_new #' @keywords internal box_update_file <- function(file_id, file, dir_id, pb = FALSE) { + + dir_id <- as_box_id(dir_id) + file_id <- as_box_id(file_id) + httr::RETRY( "POST", - paste0("https://upload.box.com/api/2.0/files/", box_id(file_id), + paste0("https://upload.box.com/api/2.0/files/", file_id, "/content"), get_token(), encode = "multipart", @@ -53,7 +60,7 @@ box_update_file <- function(file_id, file, dir_id, pb = FALSE) { list( attributes = paste0( - '{"name": "', basename(file), '", "parent": {"id":"', box_id(dir_id) + '{"name": "', basename(file), '", "parent": {"id":"', dir_id ,'"}}' ), file = httr::upload_file(file) diff --git a/R/boxr_add_description.R b/R/boxr_add_description.R index fa9ed855..a57c414e 100644 --- a/R/boxr_add_description.R +++ b/R/boxr_add_description.R @@ -12,6 +12,8 @@ #' #' @export box_add_description <- function(file_id, description) { + + file_id <- as_box_id(file_id) file_id <- handle_file_id(file_id) req <- httr::RETRY( diff --git a/R/boxr_collab.R b/R/boxr_collab.R index ee6a8801..1ba0ff3d 100644 --- a/R/boxr_collab.R +++ b/R/boxr_collab.R @@ -66,6 +66,11 @@ box_collab_create <- function(dir_id = NULL, user_id = NULL, file_id = NULL, group_id = NULL, login = NULL, role = "editor", can_view_path = FALSE) { + dir_id <- as_box_id(dir_id) + user_id <- as_box_id(user_id) + file_id <- as_box_id(file_id) + group_id <- as_box_id(group_id) + # if login is provided, ignore user_id if (!is_void(login)) { user_id <- NULL @@ -177,6 +182,9 @@ box_dir_invite <- function(dir_id, user_id, login = NULL, role = "viewer", can_view_path = FALSE) { .Deprecated("box_collab_create") + dir_id <- as_box_id(dir_id) + user_id <- as_box_id(user_id) + # if login is provided, ignore user_id if (!is_void(login)) { user_id <- NULL @@ -218,7 +226,10 @@ box_dir_invite <- function(dir_id, user_id, login = NULL, role = "viewer", #' @export #' box_collab_get <- function(dir_id = NULL, file_id = NULL) { - + + dir_id <- as_box_id(dir_id) + file_id <- as_box_id(file_id) + # detect item type for API call item_id <- dir_id %|0|% file_id @@ -275,6 +286,8 @@ box_collab_get <- function(dir_id = NULL, file_id = NULL) { #' box_collab_delete <- function(collab_id) { + collab_id <- as_box_id(collab_id) + url <- glue::glue("https://api.box.com/2.0/collaborations/{collab_id}") resp <- httr::RETRY( diff --git a/R/boxr_comment.R b/R/boxr_comment.R index fecf358c..158cbd4a 100644 --- a/R/boxr_comment.R +++ b/R/boxr_comment.R @@ -41,6 +41,7 @@ box_comment_create <- function(file_id = NULL, message, comment_id = NULL) { # https://developer.box.com/reference/post-comments/ checkAuth() + file_id <- as_box_id(file_id) item <- comment_item_helper(file_id, comment_id) @@ -76,6 +77,8 @@ box_comment_create <- function(file_id = NULL, message, comment_id = NULL) { #' box_comment_get <- function(file_id) { + file_id <- as_box_id(file_id) + resp <- httr::RETRY( "GET", glue::glue("https://api.box.com/2.0/files/{file_id}/comments"), diff --git a/R/boxr_delete_restore.R b/R/boxr_delete_restore.R index 4c0fa00c..e0c2850e 100644 --- a/R/boxr_delete_restore.R +++ b/R/boxr_delete_restore.R @@ -24,6 +24,9 @@ #' #' @export box_delete_file <- function(file_id) { + + file_id <- as_box_id(file_id) + boxDeleteFile(file_id) invisible(NULL) } @@ -31,6 +34,9 @@ box_delete_file <- function(file_id) { #' @rdname box_delete_file #' @export box_restore_file <- function(file_id) { + + file_id <- as_box_id(file_id) + req <- httr::RETRY( "POST", paste0( @@ -54,6 +60,9 @@ box_restore_file <- function(file_id) { #' @rdname box_delete_file #' @export box_delete_folder <- function(dir_id) { + + dir_id <- as_box_id(dir_id) + boxDeleteFolder(dir_id) invisible(NULL) } @@ -62,6 +71,9 @@ box_delete_folder <- function(dir_id) { #' @rdname box_delete_file #' @export box_restore_folder <- function(dir_id) { + + dir_id <- as_box_id(dir_id) + req <- httr::RETRY( "POST", paste0( @@ -91,6 +103,9 @@ box_restore_folder <- function(dir_id) { #' @keywords internal boxDeleteFile <- function(file_id) { + + file_id <- as_box_id(file_id) + req <- httr::RETRY( "DELETE", paste0( @@ -113,6 +128,9 @@ boxDeleteFile <- function(file_id) { #' @keywords internal boxDeleteFolder <- function(dir_id) { + + dir_id <- as_box_id(dir_id) + req <- httr::RETRY( "DELETE", paste0( diff --git a/R/boxr_dir_verbs.R b/R/boxr_dir_verbs.R index 79622e68..8fa35d3b 100644 --- a/R/boxr_dir_verbs.R +++ b/R/boxr_dir_verbs.R @@ -63,7 +63,9 @@ #' @export box_fetch <- function(dir_id = box_getwd(), local_dir = getwd(), recursive = TRUE, overwrite = FALSE, delete = FALSE) { + checkAuth() + dir_id <- as_box_id(dir_id) t1 <- Sys.time() @@ -164,6 +166,7 @@ box_push <- function(dir_id = box_getwd(), local_dir = getwd(), ignore_dots = TRUE, overwrite = FALSE, delete = FALSE) { checkAuth() + dir_id <- as_box_id(dir_id) t1 <- Sys.time() diff --git a/R/boxr_file_versions.R b/R/boxr_file_versions.R index 8a85dce0..0bcdfb63 100644 --- a/R/boxr_file_versions.R +++ b/R/boxr_file_versions.R @@ -33,7 +33,9 @@ #' @export #' box_version_history <- function(file_id) { - + + file_id <- as_box_id(file_id) + content <- box_version_api(file_id) if (is_void(content)) { @@ -61,6 +63,8 @@ box_version_history <- function(file_id) { #' box_previous_versions <- function(file_id) { + file_id <- as_box_id(file_id) + lifecycle::deprecate_soft( "3.6.0", what = "boxr::box_previous_versions()", @@ -72,6 +76,8 @@ box_previous_versions <- function(file_id) { # internal function to support superseding prev_versions <- function(file_id) { + file_id <- as_box_id(file_id) + entries <- box_version_api(file_id) # The box API isn't very helpful if there are no previous versions. If this diff --git a/R/boxr_misc.R b/R/boxr_misc.R index 57c3d708..d2779f76 100644 --- a/R/boxr_misc.R +++ b/R/boxr_misc.R @@ -18,6 +18,8 @@ #' @export box_ls <- function(dir_id = box_getwd(), limit = 100, max = Inf, fields = NULL) { + dir_id <- as_box_id(dir_id) + if (limit > 1000) { warning("The maximum limit is 1000; box_ls is using 1000.") limit <- 1000 @@ -28,7 +30,7 @@ box_ls <- function(dir_id = box_getwd(), limit = 100, max = Inf, fields = NULL) url_root <- "https://api.box.com/2.0" url <- httr::parse_url( - paste(url_root, "folders", box_id(dir_id), "items", sep = "/") + paste(url_root, "folders", dir_id, "items", sep = "/") ) fields_all <- @@ -127,14 +129,15 @@ box_pagination <- function(url, max){ #' @export #' box_setwd <- function(dir_id) { - - checkAuth() - + + checkAuth() + dir_id <- as_box_id(dir_id) + req <- httr::RETRY( "GET", paste0( "https://api.box.com/2.0/folders/", - box_id(dir_id) + dir_id ), get_token(), terminate_on = box_terminal_http_codes() @@ -258,14 +261,18 @@ boxr_options <- function() { box_dir_create <- function(dir_name, parent_dir_id = box_getwd()) { checkAuth() + parent_dir_id <- as_box_id(parent_dir_id) add_folder_ref_class(httr::content( - boxDirCreate(dir_name, box_id(parent_dir_id)) + boxDirCreate(dir_name, parent_dir_id) )) } #' @keywords internal boxDirCreate <- function(dir_name, parent_dir_id = box_getwd()) { + + parent_dir_id <- as_box_id(parent_dir_id) + httr::RETRY( "POST", "https://api.box.com/2.0/folders/", @@ -273,7 +280,7 @@ boxDirCreate <- function(dir_name, parent_dir_id = box_getwd()) { encode = "multipart", body = paste0( - '{"name":"', dir_name, '", "parent": {"id": "', box_id(parent_dir_id), + '{"name":"', dir_name, '", "parent": {"id": "', parent_dir_id, '"}}' ), terminate_on = box_terminal_http_codes() @@ -299,6 +306,10 @@ boxDirCreate <- function(dir_name, parent_dir_id = box_getwd()) { #' @export #' box_browse <- function(dir_id = NULL, file_id = NULL) { + + dir_id <- as_box_id(dir_id) + file_id <- as_box_id(file_id) + item <- collab_item_helper(dir_id, file_id) utils::browseURL(glue::glue("https://app.box.com/{item$type}/{item$id}")) } diff --git a/R/boxr_read.R b/R/boxr_read.R index b104d2a3..84786ee9 100644 --- a/R/boxr_read.R +++ b/R/boxr_read.R @@ -53,6 +53,7 @@ box_read <- function(file_id, type = NULL, version_id = NULL, version_no = NULL, read_fun = rio::import, ...) { checkAuth() + file_id <- as_box_id(file_id) temp_file <- withr::local_tempfile() @@ -110,6 +111,7 @@ box_read <- function(file_id, type = NULL, version_id = NULL, #' @rdname box_read #' @export box_read_csv <- function(file_id, ...) { + file_id <- as_box_id(file_id) box_read(file_id, format = "csv", ...) } @@ -117,6 +119,7 @@ box_read_csv <- function(file_id, ...) { #' @rdname box_read #' @export box_read_tsv <- function(file_id, ...) { + file_id <- as_box_id(file_id) box_read(file_id, format = "tsv", ...) } @@ -124,6 +127,7 @@ box_read_tsv <- function(file_id, ...) { #' @rdname box_read #' @export box_read_json <- function(file_id, ...) { + file_id <- as_box_id(file_id) box_read(file_id, read_fun = jsonlite::fromJSON, ...) } @@ -131,12 +135,14 @@ box_read_json <- function(file_id, ...) { #' @rdname box_read #' @export box_read_excel <- function(file_id, ...) { + file_id <- as_box_id(file_id) box_read(file_id, format = "excel", ...) } #' @rdname box_read #' @export box_read_rds <- function(file_id, ...) { + file_id <- as_box_id(file_id) box_read(file_id, read_fun = readRDS, ...) } diff --git a/R/boxr_save_load.R b/R/boxr_save_load.R index 114004eb..580f24a2 100644 --- a/R/boxr_save_load.R +++ b/R/boxr_save_load.R @@ -45,6 +45,8 @@ box_save <- function(..., dir_id = box_getwd(), file_name = ".RData", box_save_image <- function(dir_id = box_getwd(), file_name = ".RData", description = NULL, filename) { + dir_id <- as_box_id(dir_id) + # TODO: in future version, remove argument if (!missing(filename)) { diff --git a/R/boxr_search.R b/R/boxr_search.R index 6c37ae5e..705b5f20 100644 --- a/R/boxr_search.R +++ b/R/boxr_search.R @@ -62,6 +62,8 @@ box_search <- function( # Validation & Coercion --------------------------------------------------- checkAuth() + ancestor_folder_ids <- as_box_id(ancestor_folder_ids) + owner_user_ids <- as_box_id(owner_user_ids) # For converting dates to box's preferred format to_rfc3339 <- function(x) { as.character(x, format = "%Y-%m-%dT%H:%M:%SZ") @@ -112,8 +114,8 @@ box_search <- function( paste(valid_content_types, collapse = ", ")) # Validate ids - ancestor_folder_ids <- box_id(ancestor_folder_ids) - owner_user_ids <- box_id(owner_user_ids) + ancestor_folder_ids <- as_box_id(ancestor_folder_ids) + owner_user_ids <- as_box_id(owner_user_ids) # Validate trash if (!trash %in% c(TRUE, FALSE)) diff --git a/R/boxr_upload_download.R b/R/boxr_upload_download.R index 07a64a37..cd2f3dca 100644 --- a/R/boxr_upload_download.R +++ b/R/boxr_upload_download.R @@ -59,11 +59,11 @@ box_dl <- function(file_id, local_dir = getwd(), overwrite = FALSE, pb = options()$boxr.progress, filename) { checkAuth() + file_id <- as_box_id(file_id) assertthat::assert_that(assertthat::is.dir(local_dir)) assertthat::assert_that(!is.na(overwrite)) assertthat::assert_that(is.logical(overwrite)) - # TODO: in future version, remove argument if (!missing(filename)) { @@ -136,6 +136,7 @@ box_dl <- function(file_id, local_dir = getwd(), overwrite = FALSE, box_ul <- function(dir_id = box_getwd(), file, pb = options()$boxr.progress, description = NULL) { checkAuth() + dir_id <- as_box_id(dir_id) # Validate filename file <- box_filename(file) diff --git a/R/boxr_write.R b/R/boxr_write.R index b4d446f7..b28018c3 100644 --- a/R/boxr_write.R +++ b/R/boxr_write.R @@ -43,6 +43,8 @@ box_write <- function(object, file_name, dir_id = box_getwd(), description = NULL, write_fun = rio::export, x, filename, ...) { + dir_id <- as_box_id(dir_id) + # TODO: in future version, remove argument if (!missing(filename)) { @@ -80,6 +82,8 @@ box_write <- function(object, file_name, dir_id = box_getwd(), description = NUL box_save_rds <- function(object, dir_id = box_getwd(), file_name = ".RDS", description = NULL) { + dir_id <- as_box_id(dir_id) + temp_file <- fs::path_temp(file_name) on.exit(fs::file_delete(temp_file)) diff --git a/tests/testthat/test_00_internal.R b/tests/testthat/test_00_internal.R index 216c0bce..b7c5c4d1 100644 --- a/tests/testthat/test_00_internal.R +++ b/tests/testthat/test_00_internal.R @@ -38,4 +38,16 @@ test_that("stack_rows works", { expect_identical(stack_rows_tbl(list(list_a, list_b)), tibble_ab) expect_identical(stack_rows_df(list(list_a, list_b)), df_ab) -}) \ No newline at end of file +}) + + +test_that("as_box_id works", { + + expect_identical(as_box_id(c(1, 2, 3)), c("1", "2", "3")) + expect_identical(as_box_id(1407651371263), "1407651371263") + + # TODO: update to expect_snapshot_error() when we use testthat 3e + expect_error(as_box_id("foo"), class = "boxr_id") + expect_error(as_box_id(3.4), class = "boxr_id") + +})