Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent application of inline markup within rethrown messages #870

Merged
merged 4 commits into from
Jan 21, 2025
Merged

prevent application of inline markup within rethrown messages #870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e3256a
prevent application of inline markup when rethrown messages fail to p…
simonpcouch Dec 9, 2024
e19d437
handle case when `parse_database_error()` succeeds
simonpcouch Dec 9, 2024
81946e5
merge upstream
simonpcouch Jan 21, 2025
f04be00
add issue ref
simonpcouch Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# odbc (development version)

* Addressed issue where error messages rethrown from some drivers would be
garbled when the raw error message contained curly brackets
(#859 by @simonpcouch).

* snowflake: Runtime driver configuration checks on `MacOS` (#857).

Expand Down
17 changes: 16 additions & 1 deletion R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ random_name <- function(prefix = "") {
rethrow_database_error <- function(msg, call = trace_back()$call[[1]]) {
tryCatch(
res <- parse_database_error(msg),
error = function(e) cli::cli_abort(msg, call = call)
error = function(e) cli::cli_abort("{msg}", call = call)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial guess re: the OP was that this had been the source of the failure (i.e. parse_database_error() failed and then the error handling mishandled the message). Turns out it wasn't, but I brought this in line with cli's documentation + added a test in the process that felt worth keeping around.

)

cli::cli_abort(
Expand Down Expand Up @@ -232,6 +232,8 @@ set_database_error_names <- function(cnd_body) {
return(cnd_body)
}

cnd_body <- escape_curly_brackets(cnd_body)

if (is.null(names(cnd_body))) {
return(
set_names(cnd_body, nm = c("x", rep("*", length(cnd_body) - 1)))
Expand All @@ -247,6 +249,19 @@ set_database_error_names <- function(cnd_body) {
)
}

# Escape curly brackets before formatting with cli (#859). Do so only to
# unnamed elements so that formatting is preserved for context
# added with `contextualize_database_error()`.
escape_curly_brackets <- function(cnd_body) {
if (is.null(names(cnd_body))) {
return(gsub("\\{", "{{", gsub("\\}", "}}", cnd_body)))
}

unnamed <- names(cnd_body) == ""
cnd_body[unnamed] <- gsub("\\{", "{{", gsub("\\}", "}}", cnd_body[unnamed]))
cnd_body
}

# check helpers for common odbc arguments --------------------------------------
check_row.names <- function(row.names, call = caller_env()) {
if (is.null(row.names) || is_scalar_logical(row.names) || is_string(row.names)) {
Expand Down
19 changes: 19 additions & 0 deletions tests/testthat/_snaps/utils.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,25 @@
* A network-related or instance-specific error has occurred while establishing a connection to 127.0.0.1. Server is not found or not accessible. Check if instance name is correct and if SQL Server is configured to allow remote connections. For more information see SQL Server Books Online.
i From 'nanodbc/nanodbc.cpp:1147'.

---

Code
rethrow_database_error(msg, call = NULL)
Condition
Error:
! ODBC failed with error S1000 from [SAP AG][LIBODBCHDB DLL][HDBODBC][SAP AG][LIBODBCHDB SO][HDBODBC][89013].
x General error;403 internal error: Error opening the cursor for the remote database <***.***> Connection not open;-10807 Connection down: Socket closed by peer {***.**.*.**:***** -> ***.**.***.**:***** TenantName:(none) SiteVolumeID:1:3 SiteType:PRIMARY ConnectionID:****** SessionID:************}
* <SQL> 'SELECT DISTINCT "po_id", ***CENSORED***'
i From 'nanodbc/nanodbc.cpp:1722'.

---

Code
rethrow_database_error(msg, call = NULL)
Condition
Error:
! `parse_database_error()` will not {be able to parse this}, but it should still be successfully rethrown as-is.

# check_row.names()

Code
Expand Down
9 changes: 9 additions & 0 deletions tests/testthat/test-utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ test_that("parse_database_error() works with messages from the wild", {
[Microsoft][ODBC Driver 18 for SQL Server]TCP Provider: Error code 0x2726
[Microsoft][ODBC Driver 18 for SQL Server]A network-related or instance-specific error has occurred while establishing a connection to 127.0.0.1. Server is not found or not accessible. Check if instance name is correct and if SQL Server is configured to allow remote connections. For more information see SQL Server Books Online. "
expect_snapshot(error = TRUE, rethrow_database_error(msg, call = NULL))

# message contains brackets that may be interpreted as inline markup (#859)
msg <- "nanodbc/nanodbc.cpp:1722: S1000
[SAP AG][LIBODBCHDB DLL][HDBODBC] General error;403 internal error: Error opening the cursor for the remote database <***.***> [SAP AG][LIBODBCHDB SO][HDBODBC] Connection not open;-10807 Connection down: [89013] Socket closed by peer {***.**.*.**:***** -> ***.**.***.**:***** TenantName:(none) SiteVolumeID:1:3 SiteType:PRIMARY ConnectionID:****** SessionID:************}
<SQL> 'SELECT DISTINCT \"po_id\", ***CENSORED***'"
expect_snapshot(error = TRUE, rethrow_database_error(msg, call = NULL))

msg <- "`parse_database_error()` will not {be able to parse this}, but it should still be successfully rethrown as-is."
expect_snapshot(error = TRUE, rethrow_database_error(msg, call = NULL))
})

test_that("set_database_error_names() works (#840)", {
Expand Down
Loading