Schema support #220
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,16 +61,22 @@ setMethod("show", "SQL", function(object) { | |
| cat(paste0("<SQL> ", [email protected], collapse = "\n"), "\n", sep = "") | ||
| }) | ||
|
|
||
| #' @export | ||
| `[.SQL` <- function(x, ...) SQL(NextMethod()) | ||
|
|
||
| #' @export | ||
| `[[.SQL` <- function(x, ...) SQL(NextMethod()) | ||
|
|
||
| #' Quote identifiers | ||
| #' | ||
| #' Call this method to generate a string that is suitable for | ||
| #' use in a query as a column or table name, to make sure that you | ||
| #' generate valid SQL and avoid SQL injection. The inverse operation is | ||
|
|
||
| #' [dbUnquoteIdentifier()]. | ||
| #' | ||
| #' @param conn A subclass of [DBIConnection-class], representing | ||
| #' an active connection to an DBMS. | ||
| #' @param x A character vector, [SQL] or [Table] object to quote as identifier. | ||
| #' @param ... Other arguments passed on to methods. | ||
| #' | ||
| #' @template methods | ||
|
|
@@ -100,6 +106,9 @@ setGeneric("dbQuoteIdentifier", | |
|
|
||
| quote_identifier <- | ||
| function(conn, x, ...) { | ||
| if (is.list(x)) { | ||
|
There was a problem hiding this comment. I think auto-vectorisation over lists is best avoided because it violates type-stability (and we generally avoid it elsewhere) |
||
| return(vapply(x, function(xx) list(dbQuoteIdentifier(conn, xx)), list(1))) | ||
| } | ||
| if (is(x, "SQL")) return(x) | ||
| if (is(x, "Table")) { | ||
| return(SQL(paste0(dbQuoteIdentifier(conn, x@name), collapse = "."))) | ||
|
|
@@ -121,17 +130,87 @@ quote_identifier <- | |
|
|
||
| #' @rdname hidden_aliases | ||
| #' @export | ||
| setMethod("dbQuoteIdentifier", signature("DBIConnection"), quote_identifier) | ||
|
|
||
| # Need to keep other method declarations around for now, because clients might | ||
| # use getMethod(), see e.g. https://github.com/r-dbi/odbc/pull/149 | ||
| #' @rdname hidden_aliases | ||
| #' @export | ||
| setMethod("dbQuoteIdentifier", signature("DBIConnection", "character"), quote_identifier) | ||
|
|
||
| #' @rdname hidden_aliases | ||
| #' @export | ||
| setMethod("dbQuoteIdentifier", signature("DBIConnection", "SQL"), quote_identifier) | ||
|
|
||
| #' Unquote identifiers | ||
| #' | ||
| #' Call this method to convert a [SQL] object created by [dbQuoteIdentifier()] | ||
|
There was a problem hiding this comment. After reading this documentation I don't know what this is going to return. It sounds like it's going to return table objects which doesn't seem right? There was a problem hiding this comment. Return values are part of the specs in DBItest which I haven't adapted yet. The method is returning a There was a problem hiding this comment. Root of my confusion here is that |
||
| #' back to a list of [Table] objects. | ||
| #' | ||
| #' @param conn A subclass of [DBIConnection-class], representing | ||
| #' an active connection to an DBMS. | ||
| #' @param x An [SQL] or [Table] object or character vector, or a list of such | ||
| #' objects, to unquote. | ||
| #' @param ... Other arguments passed on to methods. | ||
| #' | ||
| #' @template methods | ||
| #' @templateVar method_name dbUnquoteIdentifier | ||
| #' | ||
| #' @inherit DBItest::spec_sql_unquote_identifier return | ||
| #' @inheritSection DBItest::spec_sql_unquote_identifier Specification | ||
| #' | ||
| #' @family DBIResult generics | ||
| #' @export | ||
| #' @examples | ||
| #' # Unquoting allows to understand the structure of a possibly complex quoted | ||
| #' # identifier | ||
| #' | ||
| #' dbUnquoteIdentifier( | ||
| #' ANSI(), | ||
| #' SQL(c("Schema"."Table", "UnqualifiedTable")) | ||
|
There was a problem hiding this comment. Unless I am missing something this does not parse. I think you are missing a set of quotes. SQL(c("Schema"."Table", "UnqualifiedTable"))
#> Error: unexpected symbol in "SQL(c("Schema"." |
||
| #' ) | ||
| #' | ||
| #' # Character vectors are wrapped in a list | ||
| #' dbQuoteIdentifier( | ||
| #' ANSI(), | ||
| #' c(schema = "Schema", table = "Table") | ||
| #' ) | ||
| #' | ||
| #' # Lists of character vectors are returned unchanged | ||
| #' dbQuoteIdentifier( | ||
| #' ANSI(), | ||
| #' list(c(schema = "Schema", table = "Table"), "UnqualifiedTable") | ||
| #' ) | ||
| setGeneric("dbUnquoteIdentifier", | ||
| def = function(conn, x, ...) standardGeneric("dbUnquoteIdentifier") | ||
| ) | ||
|
|
||
| #' @rdname hidden_aliases | ||
| #' @export | ||
| setMethod("dbUnquoteIdentifier", signature("DBIConnection"), function(conn, x, ...) { | ||
| if (is.list(x)) { | ||
| return(vapply(x, dbUnquoteIdentifier, conn = conn, list(1))) | ||
| } | ||
| if (is(x, "SQL")) { | ||
| . <- strsplit(as.character(x), '^"|"$|"[.]"') | ||
| . <- lapply(., `[`, -1L) | ||
| split <- . | ||
|
There was a problem hiding this comment. I realize you likely converted this from a magrittr pipeline, but I think this would be better as There was a problem hiding this comment. And actually I think it is more explicit to use There was a problem hiding this comment. I don't like nested expressions, and the dot looks like the lesser of all evils here to me, because I don't need to invent names for intermediates I don't care about. There was a problem hiding this comment. To be honest, I really don't like the use of |
||
| tables <- lapply(split, Table) | ||
| quoted <- lapply(tables, dbQuoteIdentifier, conn = conn) | ||
| bad <- quoted != x | ||
| if (any(bad)) { | ||
| stop("Can't unquote ", x[bad][[1L]], call. = FALSE) | ||
| } | ||
| return(tables) | ||
| } | ||
| if (is(x, "Table")) { | ||
| return(list(x)) | ||
| } | ||
| if (is.character(x)) { | ||
| return(list(do.call(Table, as.list(x)))) | ||
| } | ||
| stop("x must be character, SQL or Table, or a list of such objects", call. = FALSE) | ||
| }) | ||
|
|
||
| #' Quote literal strings | ||
| #' | ||
|
|
@@ -194,15 +273,15 @@ quote_string <- | |
| # use getMethod(), see e.g. https://github.com/r-dbi/odbc/pull/149 | ||
| #' @rdname hidden_aliases | ||
| #' @export | ||
| setMethod("dbQuoteString", signature("DBIConnection"), quote_string) | ||
|
|
||
| #' @rdname hidden_aliases | ||
| #' @export | ||
| setMethod("dbQuoteString", signature("DBIConnection", "character"), quote_string) | ||
|
|
||
| #' @rdname hidden_aliases | ||
| #' @export | ||
| setMethod("dbQuoteString", signature("DBIConnection", "SQL"), quote_string) | ||
|
|
||
| #' Quote literal values | ||
| #' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,21 +3,34 @@ setClass("Table", slots = list(name = "character")) | |
|
|
||
| #' Refer to a table nested in a hierarchy (e.g. within a schema) | ||
| #' | ||
| #' Objects of class `Table` have a single slot `name`, which is a named | ||
|
There was a problem hiding this comment. Calling this a table seems slightly confusing since it's the name of a table, not a table itself. Maybe TableRef? There was a problem hiding this comment. And doesn't every other object start with There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment. This search doesn't find packages that export
There was a problem hiding this comment. Sorry, it needs to be There was a problem hiding this comment. Why doesn't it find "nest"? https://github.com/search?utf8=%E2%9C%93&q=user%3Acran+filename%3ANAMESPACE+%22export+nest%22&type= There was a problem hiding this comment. I am not sure, but it seems like GitHub limits the length of time a query can run. Anyway I ran the search on my Local CRAN mirror, there are no CRAN packages that export You can call it There was a problem hiding this comment. Confirmed (from a scraping of my local CRAN mirror) that no package exports an |
||
| #' character vector. | ||
| #' | ||
| #' @param ... Components of the hierarchy, e.g. `schema`, `table`, | ||
| #' or `cluster`, `catalog`, `schema`, `table`. | ||
| #' For more on these concepts, see | ||
| #' \url{http://stackoverflow.com/questions/7022755/} | ||
| #' @export | ||
| Table <- function(...) { | ||
| components <- c(...) | ||
| if (is.null(names(components)) || any(names(components) == "")) { | ||
|
There was a problem hiding this comment. I am not sure we want to force naming, particularly for the most common case (schema & table). There was a problem hiding this comment. Which comes first -- schema or table? If we force naming, that confusion won't arise. There was a problem hiding this comment. Yes, but it makes it much more cumbersome to use the function. If it is cumbersome to use no one is going to use it. Having if (length(components) == 2) && (is.null(names(components)) || all(names(components) == ""))) {
names(components) <- c("schema", "table")
}There was a problem hiding this comment. Backends are free to implement their own wrappers with named arguments, and probably should too. For instance, in BigQuery, "schema" means something entirely different. I'd rather keep things strict for the first implementation, we can always relax later but it's more difficult to impose restrictions on a function that has been exported. |
||
| stop("All arguments to Table() must be named.", call. = FALSE) | ||
| } | ||
| new("Table", name = components) | ||
| } | ||
|
|
||
| #' @rdname hidden_aliases | ||
| #' @param object Table object to print | ||
| #' @export | ||
| setMethod("show", signature("Table"), function(object) { | ||
| cat(toString(object), "\n", sep = "") | ||
| }) | ||
|
|
||
| #' @export | ||
| toString.Table <- function(x, ...) { | ||
| paste0("<Table> ", paste(x@name, collapse = ".")) | ||
| } | ||
|
|
||
| #' @rdname hidden_aliases | ||
| #' @export | ||
| setMethod("dbQuoteIdentifier", signature("DBIConnection", "Table"), quote_identifier) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tables -> objects ?