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

Fix mysqldb bugs #216

Merged
merged 1 commit into from Nov 23, 2014

Conversation

Projects
None yet
4 participants
@combro2k
Contributor

combro2k commented Sep 17, 2014

There were some bugs where the xlrstats won't log anymore it was due the ? in the queries.
This seems to be changed?
After I changed the file it worked again

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Sep 17, 2014

Member

This reintroduce the issue #201. I persoanlly have XLRStats working on all my servers and I do not have any error being shown in the log file. Also automated tests do not complain about this change.

Member

danielepantaleone commented Sep 17, 2014

This reintroduce the issue #201. I persoanlly have XLRStats working on all my servers and I do not have any error being shown in the log file. Also automated tests do not complain about this change.

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 17, 2014

Contributor

Which OS etc do you have?
I run it with Ubuntu 14.04 on docker

Contributor

combro2k commented Sep 17, 2014

Which OS etc do you have?
I run it with Ubuntu 14.04 on docker

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 17, 2014

Contributor

root@b3:~# python -V
Python 2.7.6

root@b3:~# pip list
argparse (1.2.1)
astroid (1.2.1)
chardet (2.0.1)
colorama (0.2.5)
coverage (3.7.1)
ecdsa (0.11)
html5lib (0.999)
logilab-common (0.62.1)
mock (1.0.1)
mockito (0.5.2)
MySQL-python (1.2.3)
nose (1.3.4)
nose-exclude (0.2.0)
nosexcover (1.0.10)
paramiko (1.14.1)
pip (1.5.4)
pycrypto (2.6.1)
pylint (1.3.1)
pysqlite (2.6.3)
requests (2.2.1)
setuptools (3.3)
six (1.5.2)
unittest2 (0.5.1)
urllib3 (1.7.1)
wsgiref (0.1.2)

Contributor

combro2k commented Sep 17, 2014

root@b3:~# python -V
Python 2.7.6

root@b3:~# pip list
argparse (1.2.1)
astroid (1.2.1)
chardet (2.0.1)
colorama (0.2.5)
coverage (3.7.1)
ecdsa (0.11)
html5lib (0.999)
logilab-common (0.62.1)
mock (1.0.1)
mockito (0.5.2)
MySQL-python (1.2.3)
nose (1.3.4)
nose-exclude (0.2.0)
nosexcover (1.0.10)
paramiko (1.14.1)
pip (1.5.4)
pycrypto (2.6.1)
pylint (1.3.1)
pysqlite (2.6.3)
requests (2.2.1)
setuptools (3.3)
six (1.5.2)
unittest2 (0.5.1)
urllib3 (1.7.1)
wsgiref (0.1.2)

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Sep 17, 2014

Member

I run it on Debian 7. Can you please post a log showing the XLRstats bugs you mentioned above?

Member

danielepantaleone commented Sep 17, 2014

I run it on Debian 7. Can you please post a log showing the XLRstats bugs you mentioned above?

@thomasleveil

This comment has been minimized.

Show comment
Hide comment
@thomasleveil

thomasleveil Sep 17, 2014

Member

could you share your Dockerfile, or maybe you published you docker image on the Docker hub?

Member

thomasleveil commented Sep 17, 2014

could you share your Dockerfile, or maybe you published you docker image on the Docker hub?

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone
Member

danielepantaleone commented Sep 17, 2014

I guess it's this one: https://github.com/combro2k/docker-b3

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 17, 2014

Contributor

Correct @fenixxx.
Combro2k/b3 @thomasleveil ;-) on the hub

Contributor

combro2k commented Sep 17, 2014

Correct @fenixxx.
Combro2k/b3 @thomasleveil ;-) on the hub

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 17, 2014

Contributor

On that repo I still used the original one ;-)

Contributor

combro2k commented Sep 17, 2014

On that repo I still used the original one ;-)

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 17, 2014

Contributor

It was btw a quick setup, still need to set it up correctly. The readme.md is a bit messed up ;-)

Contributor

combro2k commented Sep 17, 2014

It was btw a quick setup, still need to set it up correctly. The readme.md is a bit messed up ;-)

@thomasleveil

This comment has been minimized.

Show comment
Hide comment
@thomasleveil

thomasleveil Sep 17, 2014

Member

@combro2k could you provide a log file showing the errors you had?

reverting back the ? to %s in the SQL queries won't happen because the ? prevent security issues (sql injection). So the only way out is to figure out what broke in your case.

Member

thomasleveil commented Sep 17, 2014

@combro2k could you provide a log file showing the errors you had?

reverting back the ? to %s in the SQL queries won't happen because the ? prevent security issues (sql injection). So the only way out is to figure out what broke in your case.

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 18, 2014

Contributor

oh wait, it might be an issue that I use mariadb mysql?

Contributor

combro2k commented Sep 18, 2014

oh wait, it might be an issue that I use mariadb mysql?

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 18, 2014

Contributor

I can't find the logs..
But I can test it later, the issue I read about was on http://bytes.com/topic/python/answers/23824-mysqldb-typeerror-not-all-arguments-converted

Contributor

combro2k commented Sep 18, 2014

I can't find the logs..
But I can test it later, the issue I read about was on http://bytes.com/topic/python/answers/23824-mysqldb-typeerror-not-all-arguments-converted

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Sep 21, 2014

Member

I maanaged to get my hands on some log file showing errors: http://pastebin.com/Zki1PPGa
There is infact an error being shown in the log produced by this line of the XLRstats plugin: https://github.com/FenixXx/big-brother-bot/blob/release-1.10/b3/extplugins/xlrstats.py#L687

I personally don't have a clue why this is not working since in other places it is (and tests are passing too)

Member

danielepantaleone commented Sep 21, 2014

I maanaged to get my hands on some log file showing errors: http://pastebin.com/Zki1PPGa
There is infact an error being shown in the log produced by this line of the XLRstats plugin: https://github.com/FenixXx/big-brother-bot/blob/release-1.10/b3/extplugins/xlrstats.py#L687

I personally don't have a clue why this is not working since in other places it is (and tests are passing too)

@thomasleveil

This comment has been minimized.

Show comment
Hide comment
@thomasleveil

thomasleveil Sep 22, 2014

Member

tests are passing too

most tests are using sqlite (due to FakeConsole) so the tests for xlrstats are never run against MySQL.

Replacing ? with %%s seems to fix this issue while still preventing sql injections... but this syntax won't work with sqlite :/

Member

thomasleveil commented Sep 22, 2014

tests are passing too

most tests are using sqlite (due to FakeConsole) so the tests for xlrstats are never run against MySQL.

Replacing ? with %%s seems to fix this issue while still preventing sql injections... but this syntax won't work with sqlite :/

@thomasleveil

This comment has been minimized.

Show comment
Hide comment
@thomasleveil

thomasleveil Sep 22, 2014

Member

please give a try to my branch gh-216. In this branch, the python-mysqldb module is replaced with the pymysql module which you can easily install with pip install pymysql

Member

thomasleveil commented Sep 22, 2014

please give a try to my branch gh-216. In this branch, the python-mysqldb module is replaced with the pymysql module which you can easily install with pip install pymysql

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 22, 2014

Contributor

I am going to test it ;-)

Contributor

combro2k commented Sep 22, 2014

I am going to test it ;-)

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 22, 2014

Contributor

@fenixxx that was the error yes:
140921 18:51:44 ERROR 'XlrstatsPlugin: could not parse event 13: not all arguments converted during string formatting'

Contributor

combro2k commented Sep 22, 2014

@fenixxx that was the error yes:
140921 18:51:44 ERROR 'XlrstatsPlugin: could not parse event 13: not all arguments converted during string formatting'

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 22, 2014

Contributor

Still the same issue:

argparse (1.2.1)
astroid (1.2.1)
chardet (2.0.1)
colorama (0.2.5)
coverage (3.7.1)
ecdsa (0.11)
html5lib (0.999)
logilab-common (0.62.1)
mock (1.0.1)
mockito (0.5.2)
MySQL-python (1.2.3)
nose (1.3.4)
nose-exclude (0.2.0)
nosexcover (1.0.10)
paramiko (1.15.0)
pip (1.5.4)
pycrypto (2.6.1)
pylint (1.3.1)
PyMySQL (0.6.2)
pysqlite (2.6.3)
requests (2.2.1)
setuptools (3.3)
six (1.5.2)
unittest2 (0.5.1)
urllib3 (1.7.1)
wsgiref (0.1.2)

Contributor

combro2k commented Sep 22, 2014

Still the same issue:

argparse (1.2.1)
astroid (1.2.1)
chardet (2.0.1)
colorama (0.2.5)
coverage (3.7.1)
ecdsa (0.11)
html5lib (0.999)
logilab-common (0.62.1)
mock (1.0.1)
mockito (0.5.2)
MySQL-python (1.2.3)
nose (1.3.4)
nose-exclude (0.2.0)
nosexcover (1.0.10)
paramiko (1.15.0)
pip (1.5.4)
pycrypto (2.6.1)
pylint (1.3.1)
PyMySQL (0.6.2)
pysqlite (2.6.3)
requests (2.2.1)
setuptools (3.3)
six (1.5.2)
unittest2 (0.5.1)
urllib3 (1.7.1)
wsgiref (0.1.2)

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 22, 2014

Contributor

140922 09:17:56 ERROR 'XlrstatsPlugin: could not parse event 34: not all arguments converted during string formatting'

Contributor

combro2k commented Sep 22, 2014

140922 09:17:56 ERROR 'XlrstatsPlugin: could not parse event 34: not all arguments converted during string formatting'

@thomasleveil

This comment has been minimized.

Show comment
Hide comment
@thomasleveil

thomasleveil Sep 22, 2014

Member

This is weird, I had it working using the environment from https://github.com/thomasleveil/docker-b3/tree/fig

Member

thomasleveil commented Sep 22, 2014

This is weird, I had it working using the environment from https://github.com/thomasleveil/docker-b3/tree/fig

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Sep 22, 2014

Member

I have just experienced the error too. In this post (a bit old) some users are complaining about the very same error. I think the problem is the placeholder being used (but I can't explain myself why since MySQLdb.placeholders specifies also Question marks as well as Numbers, C sytle string substitution (%s) and the pythonic way (%(value)s)....I'm out of ideas

Member

danielepantaleone commented Sep 22, 2014

I have just experienced the error too. In this post (a bit old) some users are complaining about the very same error. I think the problem is the placeholder being used (but I can't explain myself why since MySQLdb.placeholders specifies also Question marks as well as Numbers, C sytle string substitution (%s) and the pythonic way (%(value)s)....I'm out of ideas

@thomasleveil

This comment has been minimized.

Show comment
Hide comment
@thomasleveil

thomasleveil Sep 22, 2014

Member

@fenixxx do you have this error even when replacing MySQLdb with pymysql?

Member

thomasleveil commented Sep 22, 2014

@fenixxx do you have this error even when replacing MySQLdb with pymysql?

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Sep 22, 2014

Member

Yes

140922 12:55:28 ERROR   '[SELECT * from xlr_playerstats WHERE client_id = ? LIMIT 1] (1,)'
140922 12:55:28 ERROR   'Could not start plugin xlrstats'
Traceback (most recent call last):
  File "/home/servers/resources/bigbrotherbot/b3/parser.py", line 888, in startPlugins
    start_plugin(plugin_name)
  File "/home/servers/resources/bigbrotherbot/b3/parser.py", line 871, in start_plugin
    p.onStartup()
  File "/home/servers/resources/bigbrotherbot/b3/plugin.py", line 332, in onStartup
    self.startup()  # support backwards compatibility
  File "/home/servers/resources/bigbrotherbot/b3/extplugins/xlrstats.py", line 253, in startup
    player = self.get_PlayerStats(sclient)
  File "/home/servers/resources/bigbrotherbot/b3/extplugins/xlrstats.py", line 687, in get_PlayerStats
    cursor = self.query(q, (client_id,))
  File "/home/servers/resources/bigbrotherbot/b3/storage/database.py", line 407, in query
    raise e
TypeError: not all arguments converted during string formatting

This is the version of pymsqyl I installed:

PyMySQL                   - Pure-Python MySQL Driver
  INSTALLED: 0.6.2
  LATEST:    0.6.2-rc1
Member

danielepantaleone commented Sep 22, 2014

Yes

140922 12:55:28 ERROR   '[SELECT * from xlr_playerstats WHERE client_id = ? LIMIT 1] (1,)'
140922 12:55:28 ERROR   'Could not start plugin xlrstats'
Traceback (most recent call last):
  File "/home/servers/resources/bigbrotherbot/b3/parser.py", line 888, in startPlugins
    start_plugin(plugin_name)
  File "/home/servers/resources/bigbrotherbot/b3/parser.py", line 871, in start_plugin
    p.onStartup()
  File "/home/servers/resources/bigbrotherbot/b3/plugin.py", line 332, in onStartup
    self.startup()  # support backwards compatibility
  File "/home/servers/resources/bigbrotherbot/b3/extplugins/xlrstats.py", line 253, in startup
    player = self.get_PlayerStats(sclient)
  File "/home/servers/resources/bigbrotherbot/b3/extplugins/xlrstats.py", line 687, in get_PlayerStats
    cursor = self.query(q, (client_id,))
  File "/home/servers/resources/bigbrotherbot/b3/storage/database.py", line 407, in query
    raise e
TypeError: not all arguments converted during string formatting

This is the version of pymsqyl I installed:

PyMySQL                   - Pure-Python MySQL Driver
  INSTALLED: 0.6.2
  LATEST:    0.6.2-rc1
@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 23, 2014

Contributor

%%s works for now, the ? doesn't work still ..

Maybe it is because I run MariaDB 10.x as a DB ?

Contributor

combro2k commented Sep 23, 2014

%%s works for now, the ? doesn't work still ..

Maybe it is because I run MariaDB 10.x as a DB ?

@thomasleveil

This comment has been minimized.

Show comment
Hide comment
@thomasleveil

thomasleveil Sep 23, 2014

Member

maybe because of MariaDB, but I doubt that.
Next time I have a some free time I will try to see what happens when both python-mysqldb and pymysql are installed on the system. I suspect that pymysql defaults to mysqldb if found

Member

thomasleveil commented Sep 23, 2014

maybe because of MariaDB, but I doubt that.
Next time I have a some free time I will try to see what happens when both python-mysqldb and pymysql are installed on the system. I suspect that pymysql defaults to mysqldb if found

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Sep 24, 2014

Contributor

I created a new build when I used your version of b3.
That wasn't the case aswel it seems...

Contributor

combro2k commented Sep 24, 2014

I created a new build when I used your version of b3.
That wasn't the case aswel it seems...

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Sep 24, 2014

Member

@thomasleveil I had a look at the pymysql implementation of the execute method and the string substitution is done there:

if args is not None:
    query = query % self._escape_args(args, conn)

That's why it doesn't accepts any placeholder different from standards ones (%s, %d, ...). As far as I have read, only the sqlite3 interface supports the ? placeholder, so I think we don't have any option if not to revert back the changes.

I also had a look at the official driver on the mysql website (http://dev.mysql.com/downloads/file.php?id=453458) but apparently %s markers are required

Member

danielepantaleone commented Sep 24, 2014

@thomasleveil I had a look at the pymysql implementation of the execute method and the string substitution is done there:

if args is not None:
    query = query % self._escape_args(args, conn)

That's why it doesn't accepts any placeholder different from standards ones (%s, %d, ...). As far as I have read, only the sqlite3 interface supports the ? placeholder, so I think we don't have any option if not to revert back the changes.

I also had a look at the official driver on the mysql website (http://dev.mysql.com/downloads/file.php?id=453458) but apparently %s markers are required

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Sep 24, 2014

Member

I did some researches and I found this module: peewee
It supports PostgreSQL, MySQL and SQLite. It can be used both with ORM approach (which would be better but less portable for plugins) or directly specifying SQL statements (as you can see here) using the execute_sql method. It supports question marks paramstyle. Maybe it's worth looking into it.

Member

danielepantaleone commented Sep 24, 2014

I did some researches and I found this module: peewee
It supports PostgreSQL, MySQL and SQLite. It can be used both with ORM approach (which would be better but less portable for plugins) or directly specifying SQL statements (as you can see here) using the execute_sql method. It supports question marks paramstyle. Maybe it's worth looking into it.

@combro2k

This comment has been minimized.

Show comment
Hide comment
@combro2k

combro2k Nov 3, 2014

Contributor

Did you already have a look at it @thomasleveil?

Contributor

combro2k commented Nov 3, 2014

Did you already have a look at it @thomasleveil?

@thomasleveil

This comment has been minimized.

Show comment
Hide comment
@thomasleveil

thomasleveil Nov 3, 2014

Member

I still have to find a few hours of free time to get my head around this.

If I summup what's left to do:

  • figure out why pymysql isn't working with ? for combro2k and FenixXx while it is working for me
  • give a try at peewee
Member

thomasleveil commented Nov 3, 2014

I still have to find a few hours of free time to get my head around this.

If I summup what's left to do:

  • figure out why pymysql isn't working with ? for combro2k and FenixXx while it is working for me
  • give a try at peewee
@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Nov 3, 2014

Member

@thomasleveil I already gave a try to peewee but while examples are showings queries working with ? placeholders, on my system it didn't work. Probably the reason is the same why pymysql doesn't accept them.

Member

danielepantaleone commented Nov 3, 2014

@thomasleveil I already gave a try to peewee but while examples are showings queries working with ? placeholders, on my system it didn't work. Probably the reason is the same why pymysql doesn't accept them.

@markweirath

This comment has been minimized.

Show comment
Hide comment
@markweirath

markweirath Nov 22, 2014

Member

For this moment it seems wise to merge this pull request, I've also tested it on my system and can't get it to work properly with a mysql db.

It is also referenced here:

Merging this pull request for now will make sure xlrstats will still work for mysql systems for this dev version.

Member

markweirath commented Nov 22, 2014

For this moment it seems wise to merge this pull request, I've also tested it on my system and can't get it to work properly with a mysql db.

It is also referenced here:

Merging this pull request for now will make sure xlrstats will still work for mysql systems for this dev version.

@danielepantaleone

This comment has been minimized.

Show comment
Hide comment
@danielepantaleone

danielepantaleone Nov 23, 2014

Member

I agree, also because it seems the only solution left

Member

danielepantaleone commented Nov 23, 2014

I agree, also because it seems the only solution left

thomasleveil added a commit that referenced this pull request Nov 23, 2014

@thomasleveil thomasleveil merged commit 32c44a7 into BigBrotherBot:release-1.10 Nov 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@combro2k combro2k deleted the combro2k:release-1.10 branch Nov 29, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment