Fix a regression from the MariaDB change that broke course archival procedures if the DBD:mysql driver is used

[drgrice1]

Fix course archival procedures (or anything that calls _get_db_info in Std.pm. The code in sql_single.pm is modified in the same way for consistency (althought that wasn't broken). The modification is to always use the new parsing of the DSN string regardless of the DBD driver used.

[Alex-Jordan]

Is this for an issue that happened following the merge of #1160?

That was merged Tuesday morning. I put my production server on WeBWorK-2.16 Monday, and this morning I archived Fall courses and had not seen this. Did I get lucky that I did not have #1160 merged?

[pstaabp]

I can test, but not sure what to test.

[pstaabp]

It seems like from the title if DBD::MySql is installed and used. Does this mean with the old way of defining the $dbn field?

[drgrice1]

@taniwallach: I modified the parsing of $dsn to be more versatile and allow things like $database_dsn="DBI:$database_driver:database=$database_name;mariadb_socket=/var/run/mysqld/mysqld.sock";. The parsing code is a simplified version of what is in _OdbcParse that covers the cases we need.

@Alex-Jordan: Yes, it is a regression from #1160. It only affects archiving courses if you are using the DBD::mysql database driver, or if you are using the DBD:MariaDB driver and have to add something like the mariadb_socket option in my comment above.

@pstaabp: Select $database_driver="mysql";" in site.conf, and then try to archive a course. Without this pull request you will get a lot of errors stating Failed to dump table ...`. With this pull request that won't happen.

[drgrice1]

It would probably also be good to check that you can also archive a course using $database_driver="MariaDB"; as well. This changes the code used to parse the $dsn in that case also. You could also try with setting $database_driver="MariaDB"; and $database_dsn="DBI:$database_driver:database=$database_name;mariadb_socket=/var/run/mysqld/mysqld.sock";. You may need to adjust the socket path for your system.

[pstaabp]

In site.conf I switched

$database_driver="MariaDB";

to

$database_driver="mysql";

Trying to archive a course, I get the error:

no database specified in DSN! at /Users/pstaab/code/ww-docker/webwork2/lib/WeBWorK/DB/Schema/NewSQL/Std.pm line 297.

Note: the line number should be lower, because I dumped out the $dsn variable as:

"dbi:mysql:webwork"

[drgrice1]

That $dsn will not work. It needs to be $dsn="dbi:mysql:database=webwork";.

[drgrice1]

@pstaabp: What do you have in site.conf for $database_dsn?

[pstaabp]

Not sure where that came from. The new code builds up the $dsn variable and somehow I overrode it--again another case for not having generate scripts in the configuration files.

I was able to archive using both mysql and MariaDB. I didn't select the socket though. Should I?

[drgrice1]

@pstaabp: You probably don't need to mess with the socket. Those using webwork on Ubuntu 18.04 with the MariaDB driver probably will need that, but Ubuntu 20.04 does not.

[pstaabp]

This fixes the archive problem using mysql.

[taniwallach]

I think the changes are good, and also will support additional DSN settings in the name=value format, such as that for mariadb_socket when needed.

---

I have tried to trigger the bug inside a Docker setup which is running the WeBWorK-2.16 branch, and the code in site.conf building the value of $database_dsn from the individual settings. I simply am not managing to get the error to trigger.

I set WEBWORK_DB_DRIVER: mysql in docker-compose.yml, and added

warn "In lib/WeBWorK/DB/Schema/NewSQL/Std.pm see DSN = $dsn\n";

near the top of lib/WeBWorK/DB/Schema/NewSQL/Std.pm (after the variable was defined) but the archiving works for me and /var/log/apache2/error.log shows lines like:

[Mon Mar 29 21:37:34.487011 2021] [perl:warn] [pid 997] [client 192.168.64.1:45644] [/webwork2/admin/] In lib/WeBWorK/DB/Schema/NewSQL/Std.pm see DSN = DBI:mysql:database=webwork;host=db;port=3306, referer: http://localhost:8080/webwork2/admin/

when I make an archive, but the archive file was created, where for @drgrice1 it fails.

Maybe the problem only really occurs for localhost where

    $database_dsn="DBI:$database_driver:database=$database_name";

without a host and port setting.

[taniwallach]

This certainly fixes the bug where a match was used instead of a substitution, which seems to have triggered a bug in some settings.

---

I tested with the modified version of lib/WeBWorK/DB/Schema/NewSQL/Std.pm in Docker using both database drivers, and it certainly works for me and I can archive a course with both drivers. However, I could do so before also.

---

I think this can be merged.

[taniwallach]

@pstaab - Were you able to trigger the error before the patch?

In any case, this is certainly better than the old code, so I think it should be merged ASAP.

[pstaabp]

@pstaab - Were you able to trigger the error before the patch?

@taniwallach The only errors I had were self-inflected by changing things I shouldn't. I think this is good to go.

[drgrice1]

Yes, this only happens when you use the $dsn that does not have the host and port. Execute the following from the command line to see the difference:

perl -MDBD::mysql -e 'my %dsn; DBD::mysql->_OdbcParse("DBI:mysql:database=webwork", \%dsn, ["database", "host", "port"]); print join(", ", map { "$_ => $dsn{$_}"} keys %dsn). "\n"'

compared to

perl -MDBD::mysql -e 'my %dsn; DBD::mysql->_OdbcParse("DBI:mysql:database=webwork;host=db;port=3306", \%dsn, ["database", "host", "port"]); print join(", ", map { "$_ => $dsn{$_}"} keys %dsn). "\n"'

The first returns database => webwork, host => mysql and the second returns host => db, port => 3306, database => webwork.

This is because the while loop in _OdbcParse does the following:
loop 1 (both above cases): set $dsn{database} = 'dbi'
loop 2 (both above cases): set $dsn{host} = 'mysql'
loop 3 (both above cases): set $dsn{database} = 'webwork'
This is where the loop and the _OdbcParse method ends in the first case. Thus it ends with %dsn = { database => 'webwork', host => 'mysql' }.
In the second case the loop continues with:
loop 4: set $dsn{host} = 'db'
loop 5: set $dsn{port} = '3306'.
Thus the second case ends with %dsn = { database => 'webwork', host => 'db', port => '3306' }.

This is also why the method still works with $dsn = "DBI:mysql:database=webwork;localhost=127.0.0.1;port=3306 (the TCP connection version).

[drgrice1]

The point is that the _OdbcParse method does not expect "DBI::driver" at the beginning of the dsn string. The substitution removed that before. Of course the match does not.

[taniwallach]

@drgrice1 Thanks for the detailed explanation... I did realize at some point that only the shorter dsn was able to trigger the problem, but did not walk through the old code to figure out precisely why.

It is clear that the new code sets aside the leading DBI:driver portion and parses the name-value pairs for the rest, so is much better than what had been in the quick approach I had to parse in a fixed order for the MariaDB driver.

We no longer allow "aliases" (dbname or db instead of database; hostname instead of host) but since we build the dsn in a standard manner, this should not matter.

[drgrice1]

Yeah. It wouldn't be too hard to implement that if we still want that.

[taniwallach]

Yeah. It wouldn't be too hard to implement that if we still want that.

I think we are better off sticking to a single standard, and making that clear in the release notes for anyone who wants to use a custom dsn. Otherwise, we can keep adding alternates to the "standard" key-name, just to support specific sites which could just use the standard form.

However, it is possible that some sites have a need for a custom dsn with additional settings, ex. to handle ssl settings. I found one forum post with such as dsn:

• $database_dsn ="dbi:mysql:webwork:webwork-secure-do-user-3930413-0.db.ondigitalocean.com:25060;mysql_ssl=1;mysql_ssl_ca_file=/mnt/c/Users/Henry/Desktop/Perl/ca-certificate.crt"
  -in: https://webwork.maa.org/moodle/mod/forum/discuss.php?d=4699#p13946

and I'm not sure if that was tested in terms of archiving courses. I suspect that archiving did not work with that in the prior code, but we can see how to handle that in the future, if we have a cloud DB to test with. We can then extend the standard for the WW dsn to support addition settings needed for such use cases.

[drgrice1]

That dsn would have worked with the _OdbcParse method (and will also work with the code implemented here with minor modification). However, the additional arguments would not (and still will not with the code implemented here) be passed on to the mysqldump command for course archiving. It would not be hard to pass those arguments along also by modifying to code that creates the $my_cnf variable contents.

[taniwallach]

... However, the additional arguments would not (and still will not with the code implemented here) be passed on to the mysqldump command for course archiving. It would not be hard to pass those arguments along also by modifying to code that creates the $my_cnf variable contents.

This is just one sample of what sort of custom dsn may be needed for a specific setting,

If I was certain what was needed for common "cloud DB" usage, I would say we should try to fix this before WW 2.16 is finalized, but I don't really know what is needed.

I may be a bit wiser in a few weeks, as one of the Israeli institutions I am working with expressed interest in moving the database for their WW server to an AWS RDS database. However, I don't have any timeline on when they want to try that out.If and when I can test things out, I will look into what is needed.

If someone already has suitable experience and resources for looking into this sooner - all the better.

[drgrice1]

The point is that you could just pass along all of the arguments that are parsed out. For example, if the dsn is
$database_dsn ="dbi:mysql:database=webwork;host=webwork-secure-do-user-3930413-0.db.ondigitalocean.com;port=25060;mysql_ssl=1;mysql_ssl_ca_file=/mnt/c/Users/Henry/Desktop/Perl/ca-certificate.crt" (slightly modified to work with both the old and new code), then you would get (from both the old and the new code)

%dsn = {
    database => 'webwork',
    host => 'webwork-secure-do-user-3930413-0.db.ondigitalocean.com',
    port => '25060',
    mysql_ssl => '1',
    mysql_ssl_ca_file => '/mnt/c/Users/Henry/Desktop/Perl/ca-certificate.crt'
}

Then you pass all of those on to $my_cnf instead of just the host and port.