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.

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

Don't encodeString() by default in dbQuoteString() #121

Closed
gaborcsardi opened this issue Jul 8, 2016 · 6 comments
Closed

Don't encodeString() by default in dbQuoteString() #121

gaborcsardi opened this issue Jul 8, 2016 · 6 comments

Comments

@gaborcsardi
Copy link

I use sqlInterpolate whenever I can, and it encodes the strings via dbQuoteString and encodeString by default, which is fine, but I don't think that there is an operation to decode a string that I am getting out of a database.

Maybe there is a standard way to decode such a string, but I cannot find it. I cannot even use capture.output with cat, because capture_output encodes the string again.

@krlmlr
Copy link
Member

krlmlr commented Jul 9, 2016

Could you please show an example?

@gaborcsardi
Copy link
Author

Good point, sorry. Here is one:

library(RSQLite)
library(DBI)

db <- dbConnect(RSQLite::SQLite(), "my.db")
dbSendQuery(db, "CREATE TABLE expect (name TEXT PRIMARY KEY, chat TEXT)")

dbSendQuery(db,
  sqlInterpolate(ANSI(),
    "INSERT INTO expect VALUES (?name, ?chat)",
    name = "foo",
    chat = "Special \n, \\n, \\\\r"
  )
)

res <- dbGetQuery(db, "SELECT * FROM expect")
res$chat

## [1] "Special \\n, \\\\n, \\\\\\\\r"

devtools::session_info()
## Session info -------------------------------------------------------------------
##  setting  value
##  version  R version 3.3.1 (2016-06-21)
##  system   x86_64, darwin13.4.0
##  ui       X11
##  language (EN)
##  collate  en_US.UTF-8
##  tz       Europe/London
##  date     2016-07-09
## 
## Packages -----------------------------------------------------------------------
##  package    * version  date       source
##  clisymbols   1.0.0    2015-06-08 CRAN (R 3.2.0)
##  crayon       1.3.2    2016-06-28 local
##  DBI        * 0.4-1    2016-05-26 Github (rstats-db/DBI@a79dcc6)
##  devtools     1.12.0   2016-06-24 CRAN (R 3.2.5)
##  digest       0.6.9    2016-01-08 CRAN (R 3.2.3)
##  memoise      1.0.0    2016-01-29 CRAN (R 3.2.3)
##  memuse       2.5      2015-07-02 CRAN (R 3.2.0)
##  parr         3.3.0    2016-04-16 Github (gaborcsardi/parr@3a2564e)
##  prompt       1.0.0    2016-04-16 local (gaborcsardi/prompt@53e0550)
##  Rcpp         0.12.5   2016-05-14 CRAN (R 3.2.5)
##  RSQLite    * 1.0.9002 2016-05-26 Github (rstats-db/RSQLite@3098b59)
##  withr        1.0.2    2016-06-20 CRAN (R 3.2.5)

After this I updated DBI and RSQLite, and got the same with

devtools::session_info()
## Session info -------------------------------------------------------------------
##  setting  value
##  version  R version 3.3.1 (2016-06-21)
##  system   x86_64, darwin13.4.0
##  ui       X11
##  language (EN)
##  collate  en_US.UTF-8
##  tz       Europe/London
##  date     2016-07-09
## 
## Packages -----------------------------------------------------------------------
##  package    * version  date       source
##  clisymbols   1.0.0    2015-06-08 CRAN (R 3.2.0)
##  crayon       1.3.2    2016-06-28 local
##  DBI        * 0.4-2    2016-07-09 Github (rstats-db/DBI@a1367f7)
##  devtools     1.12.0   2016-06-24 CRAN (R 3.2.5)
##  digest       0.6.9    2016-01-08 CRAN (R 3.2.3)
##  memoise      1.0.0    2016-01-29 CRAN (R 3.2.3)
##  memuse       2.5      2015-07-02 CRAN (R 3.2.0)
##  parr         3.3.0    2016-04-16 Github (gaborcsardi/parr@3a2564e)
##  prompt       1.0.0    2016-04-16 local (gaborcsardi/prompt@53e0550)
##  Rcpp         0.12.5   2016-05-14 CRAN (R 3.2.5)
##  RSQLite    * 1.0.9004 2016-07-09 Github (rstats-db/RSQLite@e9d3c0f)
##  withr        1.0.2    2016-06-20 CRAN (R 3.2.5)

@krlmlr
Copy link
Member

krlmlr commented Jul 9, 2016

Thanks. No conversion should be necessary, sqlInterpolate() should just add the strings so that a 1:1 copy is returned when calling select.

SQLite doesn't seem to use \ as escaping character:

sqlite> SELECT length('\n');
2
sqlite> SELECT length('n');
1
sqlite> SELECT length('\');
1

You should call sqlInterpolate(db, ...), not sqlInterpolate(ANSI(), ...), but the behavior is the same when I do the former. Postgres has the same behavior, only MySQL knows \n (but also accepts an inline line feed character).

I'll change the default in DBI, and add a test for the roundtrip.

@krlmlr krlmlr changed the title Decode a string encoded by encodeString Don't encodeString() by default in dbQuoteIdentifier() Jul 9, 2016
@krlmlr krlmlr changed the title Don't encodeString() by default in dbQuoteIdentifier() Don't encodeString() by default in dbQuoteString() Jul 9, 2016
@krlmlr krlmlr closed this as completed in 310346c Jul 9, 2016
@gaborcsardi
Copy link
Author

Thanks!

So even if encodeString is not called, the names and fields will be still properly escaped, right?

@krlmlr
Copy link
Member

krlmlr commented Jul 9, 2016

Everything should "just work" ;-) Could you please rephrase the question? I don't understand "names and fields".

@gaborcsardi
Copy link
Author

Forget the names and fields, it was a stupid question. Thanks again!

krlmlr pushed a commit that referenced this issue Jul 31, 2016
- The default implementation of `dbQuoteString()` doesn't call `encodeString()` anymore: Neither SQLite nor Postgres understand e.g. `\n` in a string literal, and all of SQLite, Postgres, and MySQL accept an embedded newline (#121).
- New `dbWithTransaction()` that calls `dbBegin()` and `dbCommit()`, and `dbRollback()` on failure (#110, @bborgesr).
krlmlr added a commit that referenced this issue Aug 11, 2016
- Interface changes
    - `dbDataType()` maps `character` values to `"TEXT"` by default (#102).
    - The default implementation of `dbQuoteString()` doesn't call `encodeString()` anymore: Neither SQLite nor Postgres understand e.g. `\n` in a string literal, and all of SQLite, Postgres, and MySQL accept an embedded newline (#121).

- Interface enhancements
    - New `dbSendStatement()` generic, forwards to `dbSendQuery()` by default (#20, #132).
    - New `dbExecute()`, calls `dbSendStatement()` by default (#109, @bborgesr).
    - New `dbWithTransaction()` that calls `dbBegin()` and `dbCommit()`, and `dbRollback()` on failure (#110, @bborgesr).
    - New `dbBreak()` function which allows aborting from within `dbWithTransaction()` (#115, #133).
    - Export `dbFetch()` and `dbQuoteString()` methods.

- Documentation improvements:
    - One example per function (except functions scheduled for deprecation) (#67).
    - Consistent layout and identifier naming.
    - Better documentation of generics by adding links to the class and related generics in the "See also" section under "Other DBI... generics" (#130). S4 documentation is directed to a hidden page to unclutter documentation index (#59).
    - Fix two minor vignette typos (#124, @mdsumner).
    - Add package documentation.
    - Remove misleading parts in `dbConnect()` documentation (#118).
    - Remove misleading link in `dbDataType()` documentation.
    - Remove full stop from documentation titles.
    - New help topic "DBIspec" that contains the full DBI specification (currently work in progress) (#129).
    - HTML documentation generated by `staticdocs` is now uploaded to http://rstats-db.github.io/DBI for each build of the "production" branch (#131).
    - Further minor changes and fixes.

- Internal
    - Use `contains` argument instead of `representation()` to denote base classes (#93).
    - Remove redundant declaration of transaction methods (#110, @bborgesr).
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants