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

Double the backslash in mysql in case we're in 'LIKE' #10157

Merged
merged 1 commit into from Aug 29, 2016

Conversation

Projects
None yet
7 participants
@phate89
Copy link
Contributor

commented Jul 23, 2016

@DaveTBlake spotted a bug in our current mysqldataset. The bug is more a wrong implementation because of a specific case.
Currently db values are passed through PrepareSQL to get the parameters formatted well (for example strings will have backslash doubled and special chars formatted).
Sqlite has a specific function for it but mysql doesn't so in 2010 (!!!) firnsy (not sure if it was a member) ported the sqlite function to mysql (xbmc/xbmc-antiquated@a2ae89c https://sourceforge.net/p/xbmc/mailman/message/25665578/ )
The main problem is that mysql has a "unique feature" where in like statements ( for eample SELECT * FROM movie_view WHERE strPath LIKE 'C:\\Film\\%') doubling backslash isn't enough because it parse the string twice (explained in the note here http://dev.mysql.com/doc/refman/5.7/en/string-comparison-functions.html#operator_like ). SQlite doesn't work like this so this isn't handled. for example
this query works in both mysql and sqlite:

`SELECT * FROM movie_view WHERE strPath='C:\\Film\\Batman\\'

but this didn't work in mysql (work of course in sqlite)

`SELECT * FROM movie_view WHERE strPath LIKE 'C:\\Film\\Batman\\%'

because you need

`SELECT * FROM movie_view WHERE strPath LIKE 'C:\\\\Film\\\\Batman\\\\%'

This of course breaks every playlist with windows local paths because they all have backslash..
So we need to double backslash but when we have a like we need to quadruple it.
I tried to insert in this current code a check to detect if we're parsing a like parameter.. I'm not sure it's okay to do it like this but I can't think to another way.. Every suggestion is welcome..
@MilhouseVH not sure if it's worth it to add it to your builds.. I tested it quickly and it seems to work but it might break a lot of other things..

@wsnipex

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2016

it works and we use it to escape the string (it's used after I doubled the backslash) but in like is a different story. that function doesn't know if we're in a like or not so it just performs a regular escape (Adding backslash before every character needed). Like needs a regular escape PLUS doubled backslash...

@wsnipex

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

SELECT * FROM movie_view WHERE strPath LIKE QUOTE('C:\Film\Batman%')?

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2016

I tried and it doesn't work.. Also if it worked it could create problems with sqlite.. anyway thanks fo trying to find a simpler solution.. this should be our last resort..

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

Thanks for doing this @phate89 . I know it does not feel like an elegant solution, but I think it is a necessary one. At the moment on MySQL any queries using LIKE with backslash in the parameter just don't work, which includes any query on path in a Windows system.

+1

@Tolriq

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2016

@phate89 not sure how parsing is done in Mysql but I faced a more or like same problem in another project.

Things to tests :

  • What happens if there's "like" string in any parameter / field name like a query with xx = like
  • Check if there's case where the parameter is passed through a function like xxx like UPPERCASE(xxx%)
@tamland

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

What I'm wondering about is why do you have windows paths in you db in the first place.. Shouldn't the vfs handle this?

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

Not sure how you expect the vfs to handle this @tamland ?
In music database the path table has rows for every folder and sub folder that contains music files, the physical location of the music. This is used by library update to check what files have been changed and need rescanning etc., to deduce the location of NFO files and thumbnails for albums and artists, and as a filter parameter for smart playlists e.g. list only songs in this path. The library needs to know where the music files are.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2016

@tamland no if you simply add a local path on Windows you get the windows regular path..no vfs special path involved..

@tamland

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

Correct me if im wrong (since i dont use window), but afaik the vfs can already translate the platform independent urls to the real windows paths. Not that it in any way helps with the immediate problem but maybe it's time to fix the root cause: leaking platform details into the database. It really doesn't need to know about the path separator. And that would take care of a lot of different issues as well..

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2016

There's something like that if I remember well..but everything right now is stored with paths (including Linux paths, sources and arts) so changing it it's a huge change and I'm not sure we can change everything safely. Plus it's incorrect it's a path awareness problem.. it's a mysql like problem with \ symbol. It happens because we use like on paths but if we used for something else that required like and could contain a backslash you would have the same problem

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

Yeap replacing path would be a huge change, and it really is a MySQL problem with backslashes generically that this PR is trying to address. I would also worry about the speed impact on the library update checking too.

@phate89 phate89 changed the title [WIP] Double the backslash in mysql in case we're in 'LIKE' Double the backslash in mysql in case we're in 'LIKE' Aug 8, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2016

@DaveTBlake @MilhouseVH @tamland suggestion on how to go further? Just throw it in? wait for v18?

@wsnipex

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2016

@wsnipex I tried both.. The first doesn't work because it disables escapes in regular strings but you still need to escape them in like (so you still have to locate if the string is in a like). the second is a specific like option. it needs to be added after like and if there's no like the query fails so you still need to parse the query to determine if it's a like (and sqlite doesn't have it so we can't just use it)..

@wsnipex

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

Ok, seems there is no better solution then this PR for now. Time for a DB abstraction layer..

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

I would say merge it soonest. We don't have a better fix, and can't find anything wrong with this, but get it into the nightlies for real world testing.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 13, 2016

jenkins build and merge

@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta1 milestone Aug 13, 2016

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 13, 2016

jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 14, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2016

jenkins build this please

@phate89 phate89 force-pushed the phate89:double_backslash_mysql branch from f7e59fa to 0b9a62a Aug 29, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2016

jenkins build and merge

@jenkins4kodi jenkins4kodi merged commit 32fb8c5 into xbmc:master Aug 29, 2016

1 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
default I've found some spare time so building this now
Details
jenkins.kodi.tv Yeah yeah I'll get to it when i have some time
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@phate89 phate89 deleted the phate89:double_backslash_mysql branch Nov 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.