vivekrajenderan / simplesamlphp

Automatically exported from code.google.com/p/simplesamlphp
Other
0 stars 0 forks source link

support for SQlite in consent module #549

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Initial attempt at implementing SQlite support for the consent module.

There are two things that needed fixing:

1. make username/password optional
2. don't use NOW() in the SQL queries as SQlite does not support that

I fixed these issues in attached patch, but I still have to test it with all 
databases (MySQL, PostgreSQL and SQlite). I will update the patch when I have 
more time to work on this, this is just a way to keep track of the patches (I 
do love git...)

Original issue reported on code.google.com by mooknarf@gmail.com on 27 Apr 2013 at 7:11

Attachments:

GoogleCodeExporter commented 9 years ago
Also a thing to check: How does this work wrt. timezones? You insert a date in 
the local time of the webserver, but there is no guarantee that the database 
server is using the same timezone.

Original comment by olavmrk@gmail.com on 29 Apr 2013 at 6:01

GoogleCodeExporter commented 9 years ago
Hmm... would it be an option then to use different queries for different 
databases? There are functions for SQlite to accomplish this NOW() behavior, 
but would requiring making a SQL query based on the specific database, which is 
something I would like to avoid... 

But maybe using the web server time instead of the database time would be fix 
for a bug? Retrieving the date from the database would give the web server 
"wrong" (or at least unexpected) results...

Original comment by mooknarf@gmail.com on 29 Apr 2013 at 8:08

GoogleCodeExporter commented 9 years ago
If using different queries for different databases is the only way to get the 
database to handle this, I think it is the best way to go.

Both PostgreSQL and MySQL save times in UTC internally. Then you can select the 
timezone you actually use per connection. E.g. with something like «SET 
time_zone = 'Europe/Oslo'». However, the specific option to set varies with 
the different databases.

I therefore think it is best to leave all date/time handling to the database 
itself if possible.

Original comment by olavmrk@gmail.com on 29 Apr 2013 at 12:49

GoogleCodeExporter commented 9 years ago
I updated the patch to use different SQL queries depending on the database. It 
seems that SQLite by default uses the time in UTC and not "localtime":

framek:~ fkooman$ sqlite3
SQLite version 3.7.12 2012-04-03 19:43:07
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> select datetime();
2013-04-30 10:32:55
sqlite> select datetime("now");
2013-04-30 10:33:00
sqlite> select datetime("now","localtime");
2013-04-30 12:33:03
sqlite>

For now I used "localtime" as well in the SQLite query... what do you think? 
Should I leave that out?

I now also tested the patch with SQLite, MySQL and PostgreSQL.

Original comment by mooknarf@gmail.com on 30 Apr 2013 at 12:19

Attachments:

GoogleCodeExporter commented 9 years ago
If the default DATETIME in sqlite is in UTC, then I think that is the one that 
should be used. Other than that, I think the patch looks good.

Original comment by olavmrk@gmail.com on 2 May 2013 at 10:54

GoogleCodeExporter commented 9 years ago
Fixed that, i.e.: now just using DATETIME("NOW") for SQLite.

Original comment by mooknarf@gmail.com on 2 May 2013 at 7:21

Attachments:

GoogleCodeExporter commented 9 years ago
Thanks! Committed in r3238.

Original comment by olavmrk@gmail.com on 3 May 2013 at 11:36