search

DobyTang / LazyLibrarian

This project isn't finished yet. Goal is to create a SickBeard, CouchPotato, Headphones-like application for ebooks. Headphones is used as a base, so there are still a lot of references to it.
732 stars 71 forks source link

Removed escaping from placeholder parameters in SQL #1588

Closed knobunc closed 6 years ago

knobunc commented 6 years ago

The code was effectively double escaping ... in some places. It would escape " to "" and pass it as a placeholder. Then the DB code would further escape that. So 'E.E. "Doc" Smith' would not work corretly.

I looked at all DB commands across the code and fixed up any placeholder related problems that I saw. PRAGMA statements can not have placeholders so I left them as they were.

There was also a change to a match, we were doing the same replacement before we tried a match... but the response from the DB should not have been escaped, so I stripped that out too.

knobunc commented 6 years ago

Yeah, I just hit this... I'm not familiar with python. Patch forthcoming.

knobunc commented 6 years ago

Ok... how does that look?

philborman commented 6 years ago

Just a question of coding style, you probably have more experience than me. I had a quick google, as some of the tuples with multiple items have a trailing comma and some don't, and python doesn't care either way. Would be good if we had some consistency, should we standardise do you think? eg should we use (book, title,) or (book, title)

knobunc commented 6 years ago

I asked a co-worker with far more experience with python and he suggested that the convention was: (single,) (one, two)

BUT he also wondered why we were passing tuples there rather than: match(cmd, param1) match(cmd, param1, param2)

I'll give that a whirl and see if it works.

knobunc commented 6 years ago

This latest version just fixes the missing ,.

I'll have a version shortly that removes the tuples as soon as I have a chance to test with it.

philborman commented 6 years ago

Thanks, just running the patch now on my fork.

knobunc commented 6 years ago

Ok, it looks like database.py doesn't take variable arguments and needs the tuple. So for now, I think we stick with the patch as is.

philborman commented 6 years ago

I think the underlying python library needs a tuple. The description for sqlite cursor.execute says one or two parameters, cmd [,args] where args is a tuple. The tests here seem ok on the first library, will try a larger library and if all seems ok will merge the patch tomorrow. Thanks for your work.

philborman commented 6 years ago

No issues on the full library overnight, merged, thanks.