Error when inserting certain strings with brackets

[jghauser]

I've come across a bug when running the following code:

local tbl = require("sqlite.tbl") --- for constructing sql tables
local db = require("sqlite.db") --- for constructing sql databases

-- the data table
local tbl_data = tbl("data", {
	id = true,
	test = { "text", required = true, unique = true },
})

local tbls = {
	uri = "~/test.sqlite3",
	data = tbl_data,
}

local data = db(tbls)

data.data:remove() -- to ensure tbl is empty
data.data:insert({ test = "tes(t)" })

Gives:

E5113: Error while calling lua chunk: ...im/site/pack/packer/start/sqlite.lua/lua/sqlite/stmt.lua:35
: sqlite.lua: sql statement parse, , stmt: `insert into data (test) values(tes(t))`, err: `(`no such
 column: t`)`

It only happens when

• both opening and closing brackets are in the string
• the closing bracket is the last char of the string

I'm guessing something isn't being escaped properly.

[kkharji]

This happens because some sql functions are passed as is. I guess the parser function should have had a list of known sqlite function and escape, quote unrecognized ones.

I doubt I will fix this soon, but feel free to look into how statements are parsed

[jghauser]

Ok, thanks! I totally understand, I will probably just implement some escaping in my plugin. I'll have a look at the parser if I get to it and maybe submit a PR if I can think of anything.

[steliyan]

Pretty much hit the same: https://github.com/nvim-telescope/telescope-smart-history.nvim/pull/7

Hacky workaround... but it works. :)

[jghauser]

Thanks a lot @steliyan, this will be very useful for my plugin as I haven't implemented a fix yet :sweat_smile:

[steliyan]

@kkharji I looked into it and fixed it by providing an additional argument to treat everything as "raw" strings - meaning, whatever string you pass, it's written as-is in SQLite, nothing gets treated as a function.

The good thing is - it's backwards compatible, but I don't really like the usage. I've added insertRaw method, also added options to the existing insert, both are weird. Maybe an option for the connection/table is better, but this would couple the parser logic. :|

I was thinking a better approach would be to have something like:

sqlite.fn = {
  count = function(...) end,
  date = function(...) end,
 -- the rest of the funcs
}

Use like:

local db = sqlite.new('my.sqlite')
local table = db:tbl("history", {
  id = true,
  content = "text",
  picker = "text",
  cwd = "text",
  date = "date"
})

table:insert { content = escape(line), picker = title, cwd = cwd, date = sqlite.fn.now() }

This would simplify the parser, the upside (and downside maybe?) is the complexity would be pushed down to the consumer, but if the consumers are APIs and doesn't really rely on executing random statement, this would be an overall win.

Any thoughts?

[kkharji]

@steliyan Thanks for looking into it. Yes, insertRaw would be out of place. How about we just relay on field type definition and just auto escape anything of text or string type?

I feel this is way cleaner then introducing an escape function to end users. Most cases like 99% would like the text to be skipped. Maybe for those 1% how still want to use the high-level apis with inline sqlite function can have a function e.g. sqlite.unescape that would append some key to the string to let the parser know to unescape it.

What do you think?

[steliyan]

Sorry for misleading you, escape(line) is just copy/🍝 from https://github.com/nvim-telescope/telescope-smart-history.nvim/pull/7, should be just line, like so:

table:insert { content = line, picker = title, cwd = cwd, date = sqlite.fn.now() }

If line = "datetime('now')", then a raw string datetime('now') would be inserted for the line column.

I would expect this exact behavior, however, this would be a breaking change, not sure if you are OK with that.

Current state

From what I got from the codebase, I see 2 entry points:

table:insert {}

I think we should introduce a function, so the binding of params is simplified. This would be the escape hatch, which would give the rest 1%.

NOTE: I just saw, there are a couple of functions already, it's even in a screenshot in the README, I don't know how I've missed that. I am talking about: lua/sqlite/strfun.lua, for example.

I think returning a function, not a string, as the it's currently implemented, would be better. It kinda matches between Lua/SQLite, also this would simplify the parser/statement binder (not sure for the correct name).

db:eval

Here it's tricky, because I think the binding is confusing, at least for me.

Generally, having just a string, no binding params - send it down to SQLite, don't do anything. However, if I include a function-like for a value binding, then I don't get the raw value, but a function.

For example:

local select = stmt:parse(bind_db, "select :count from todos")
select:bind({count= 'count(*)'})

For todos table, having 3 rows, it returns: 3, but I would expect to get: { "count(*)", "count(*)", "count(*)" }.

I heavily use MySQL lately, as well as some JavaScript ORMs. They use :value and :identifier: syntax (check objectionjs raw). So I think, if the value is a non-function, we should treat as a value, otherwise as a builtin function.

A bit of a long post, but happy to discuss with you on what you think it's the best direction. :)

[kkharji]

Sorry for misleading you, escape(line) is just copy/🍝 from https://github.com/nvim-telescope/telescope-smart-history.nvim/pull/7, should be just line, like so:

Oh so this would auto-escape.

I would expect this exact behavior, however, this would be a breaking change, not sure if you are OK with that.

🤔 Yes, but this would break some programs. Put it is what we all expect 99% of the time and having to worry about what we insert is unnecessary.

However, given that we do provide sqlite.fn., we need to have those function give a string with for example prefix _DONT_ESCAPE_ME_, which will be checked and made as is.

I think returning a function, not a string, as the it's currently implemented, would be better. It kinda matches between Lua/SQLite, also this would simplify the parser/statement binder (not sure for the correct name).

Could you an give an example?. Not sure if this a suggestion or affirmation 😄

db:eval

Hmmm here, db:eval is considered low level api and nothing should be done to it. Whatever sqlite accept it should too. No extra handling or magic should be introduced here.

I heavily use MySQL lately, as well as some JavaScript ORMs. They use :value and :identifier: syntax (check objectionjs raw). So I think, if the value is a non-function, we should treat as a value, otherwise as a builtin function.

🤔 idk how I feel about that. Maybe seperate module called query_Builder used as q? but this should be in a separate issue, and would be considered as big enhancement.

@steliyan you can go ahead implement what we discuss, it easier to review code. Good luck

[steliyan]

Here is a sample implementation, a couple of open questions, but do you agree with the general direction?