Configure database connection via db.connection_string param #10037

pvannierop · 2023-02-19T16:34:22Z

This update is needed to allow connection parameters required to run MySQL8.

Problem

In order to run MySQL 8 additional parameters must be provided to the database connection. At present similar parameters (e.g., db.use_ssl) are provided via dedicated properties in portal.properties and, by extension, in java class DatabaseProperties.

Solution

In order to make it easier to provide custom database connection parameters, I propose in this PR to leverage the existing db.connection_url parameter (that is unused as far as I can see) and make this the required way of configuring the connection. The db.user , db.password and db.driver properties will still be used. The db.host, db.database, db.port and db.use_ssl properties become obsolete.

This PR will also update the python migration script to run migration using the db.connection_string property. The python importer scripts all rely on confguration at the level of the Java code, and do not require update.

Backward compatibility

This implementation is backwards compatible with configuring with the legacy db.host, db.database, db.port and db.use_ssl properties.

Deprecation warning

A deprecation warning was added for the legacy style database connection params:

1:41:25.713 [RMI TCP Connection(2)-127.0.0.1] INFO  o.m.c.portal.util.GlobalProperties - Successfully read properties file

----------------------------------------------------------------------------------------------------------------
-- Deprecation warning:
-- You are connecting to the database using the deprecated 'db.host', 'db.portal_db_name' and 'db.use_ssl' properties.
-- Please use the 'db.connection_string' instead (see https://docs.cbioportal.org/deployment/customization/portal.properties-reference/).
----------------------------------------------------------------------------------------------------------------

11:41:25.718 [RMI TCP Connection(2)-127.0.0.1] INFO  o.s.c.s.PostProcessorRegistrationDelegate$BeanPostProcessorChecker - Bean 'businessDataSource' of type [org.mskcc.cbio.portal.dao.JdbcDataSource] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying)
...
11:41:29.012 [RMI TCP Connection(2)-127.0.0.1] INFO  o.s.web.context.ContextLoader - Root WebApplicationContext initialized in 6066 ms

----------------------------------------------------------------------------------------------------------------
-- You are connecting to the OncoKB public instance which does not include any therapeutic information.
-- Please consider obtaining a license to support future OncoKB development by following https://docs.cbioportal.org/2.4-integration-with-other-webservices/oncokb-data-access.
-- Thank you.
----------------------------------------------------------------------------------------------------------------

<6>Initializing Spring DispatcherServlet 'api'
11:41:29.079 [RMI TCP Connection(2)-127.0.0.1] INFO  o.s.web.servlet.DispatcherServlet - Initializing Servlet 'api'

Verification

MSK tasks:

Java part
- (backward compatible) importers
- (backward compatible) backend spring app database connection
(backward compatible) local backend migration script
deployments (beta portal)
internal usage

The Hyve tasks:

docker and docker-compose
- backend database connection
- migration script is still working

sonarcloud · 2023-02-19T16:39:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

pvannierop · 2023-02-20T10:10:49Z

Allow cBioPortal to user MySQL

n1zea144 · 2023-02-21T16:43:37Z

core/src/main/java/org/mskcc/cbio/portal/dao/JdbcDataSource.java

+            Assert.hasText(database, errorMessage("database", "db.database") + 
+                " Or preferably, set the 'db.connection_string' and remove 'db.host', 'db.database' and 'db.use_ssl (best practice).");
+            connectionURL = String.format(
+                "jdbc:mysql://%s/%s&zeroDateTimeBehavior=convertToNull&useSSL=%s",


@pvannierop This may be because I updated my local JDBC driver to 8.0.31, but I also had to add allowLoadLocalInfile=true to allow the bulk loader to work properly.

Related, I also had to update a prepared statement in MySQLbulkLoader.flushAll to get this method to work:

stmt = con.prepareStatement("SELECT @@foreign_key_checks;", ResultSet.TYPE_SCROLL_SENSITIVE, ResultSet.CONCUR_UPDATABLE);

@n1zea144 The idea of this PR is that when you have the need for these params, you should provide the full connection string with the db.connection_string parameter, rather than specifying the 'legacy' db.host, db.use_ssl, etc. Remember that allowLoadLocalInfile=true is considered as a security risk. Activation as a default is not the way to go in my opinion.

Related, I also had to update a prepared statement in MySQLbulkLoader.flushAll to get this method to work:

stmt = con.prepareStatement("SELECT @@foreign_key_checks;", ResultSet.TYPE_SCROLL_SENSITIVE, ResultSet.CONCUR_UPDATABLE);

That is strange. I could load a study w/o making these changes. How could that be?

re allowLoadLocalInfile=true - I guess we would apply this just for the importer? Doesn't seem like much of a security risk in that setting

I guess we would apply this just for the importer
Correct

pvannierop · 2023-03-06T10:33:49Z

@inodb The integration test fails because there is a missing Python dependency. This test will automatically become green when the new docker image is pushed to Dockerhub that has this dependency.

dippindots

@pvannierop Thanks for making this pr. The JDBC part looks good to me, and I can see that's backward compatible, we can still use the old property file. But I don't know why don't we also make migrate_db.py backward compatible. Maybe I missed some important things, could you help me understand this?

core/src/main/scripts/importer/cbioportalImporter.py

core/src/main/scripts/migrate_db.py

inodb · 2023-04-05T15:28:28Z

I think this might need to be updated too: https://github.com/cBioPortal/cbioportal/blob/master/docker/web-and-data/docker-entrypoint.sh#L27-L33

sonarcloud · 2023-04-20T16:14:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

0.0% Coverage
0.0% Duplication

core/src/main/java/org/mskcc/cbio/portal/dao/JdbcDataSource.java

oplantalech · 2023-07-21T09:18:11Z

docker/web-and-data/docker-entrypoint.sh

+    then
+        echo "The portal.properties file defines both db.connection_string and (one of) db.host, db.portal_db_name and "
+        echo "db.use_ssl. Please configure with either db.connection_string (preferred), or db.host, db.portal_db_name and db.use_ssl."
+        exit 1


We're promising backwards-compatibility but that's not true: if staying with properties as they are right now, the error message in line 64 will show up (since we have both obsolete and db.connection_string properties defined).

Removing the obsolete properties and leaving db.connection_string will not work, since the value of this property needs to be changed too (from jdbc:mysql://localhost:3306/ to jdbc:mysql://localhost:3306/cbioportal?useSSL=false).

If the user removes the db.connection_string and keeps the obsolete properties as they are in their setup, then the promised backwards compatibility comes into play. But since we want to prioritise using the new setup (db.connection_string), and the user needs to modify portal.properties anyway, maybe it makes more sense to force to use db.connection_string with the correct format and clean up the usage of the obsolete properties?

@inodb What do you think?

@pvannierop How about we don't allow db.connection_string shows up with one of 'db.host', 'db.portal_db_name', and 'db.use_ssl' properties? If that's the case, we throw an error and point out that the users should use db.connection_string only instead.

I know this is probably not 100% backward compatible, but I guess it's fine.

core/src/main/scripts/importer/cbioportal_common.py

oplantalech

@dippindots For me it's ready to be merged 😃

dippindots · 2023-08-07T23:42:15Z

@oplantalech Thanks for help testing the docker part. I also have done my testing for the local build. And it looks good, this is currently pending @sheridancbio's testing on our internal importing code compatibility. I will work with @sheridancbio to finish the testing and then let you know.

@oplantalech Do you have any ideas about the failing tests? I think the Labeler test is fine. But We should figure out why Core tests and Integration tests are failing before we merge this.

Needed for flexible configuration of connection parameters required to run MySQL8.

dippindots

@pvannierop It looks all good to me, just the localdb tests are not working, but I guess that's because we haven't updated the cbioportal-docker-compose repository yet. I created a pull request to make the related changes, could you help to take a look? https://github.com/cBioPortal/cbioportal-docker-compose/pull/22/files

I will include this update in the next release. @inodb @pvannierop Just wondering maybe we should update software's minor version to 5.4.0 because based on our documentation, this change might require additional effort for a deployer of cBioPortal

docs/Migration-Guide.md

sonarcloud · 2023-08-25T09:07:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

dippindots · 2023-08-25T09:38:56Z

Merging this pull request. The localdb tests are failing because it's not using the latest URL format. Related pull request:
frontend: cBioPortal/cbioportal-frontend#4710
cbioportal-docker-compose: cBioPortal/cbioportal-docker-compose#22

pvannierop mentioned this pull request Mar 1, 2024

Use MySQL 8.0 cBioPortal/icebox#450

Open

pvannierop self-assigned this Feb 19, 2023

pvannierop requested review from inodb, dippindots, onursumer and BasLee February 19, 2023 17:07

n1zea144 reviewed Feb 21, 2023

View reviewed changes

inodb requested a review from haynescd February 27, 2023 16:00

pvannierop requested a review from n1zea144 March 6, 2023 09:45

dippindots reviewed Mar 7, 2023

View reviewed changes

core/src/main/scripts/importer/cbioportalImporter.py Show resolved Hide resolved

core/src/main/scripts/migrate_db.py Outdated Show resolved Hide resolved

pvannierop force-pushed the mysql8 branch from 21871a9 to 2d9bd82 Compare April 3, 2023 09:23

pvannierop requested a review from dippindots April 3, 2023 09:25

pvannierop force-pushed the mysql8 branch 2 times, most recently from 3edb0ad to 451188a Compare April 3, 2023 09:40

pvannierop force-pushed the mysql8 branch from 451188a to ad83f3b Compare April 18, 2023 12:50

pvannierop mentioned this pull request Jul 14, 2023

Migration script does not respect db.use_ssl property #10275

Closed

pvannierop force-pushed the mysql8 branch from a746e8d to f332e68 Compare July 20, 2023 08:23

oplantalech requested changes Jul 21, 2023

View reviewed changes

dippindots reviewed Jul 25, 2023

View reviewed changes

core/src/main/scripts/importer/cbioportal_common.py Show resolved Hide resolved

pvannierop force-pushed the mysql8 branch from 65edfbb to de7243a Compare July 27, 2023 14:22

dippindots force-pushed the mysql8 branch from de7243a to 283adf3 Compare August 3, 2023 21:31

oplantalech approved these changes Aug 4, 2023

View reviewed changes

averyniceday mentioned this pull request Aug 8, 2023

Test the Db Host/Connection String knowledgesystems/pipelines-scrum#1146

Closed

dippindots mentioned this pull request Aug 10, 2023

Fix disabling SSL for MySQL connection #10276

Closed

dippindots force-pushed the mysql8 branch from 283adf3 to 2105a7a Compare August 10, 2023 19:59

dippindots added the devops label Aug 10, 2023

pvannierop force-pushed the mysql8 branch 5 times, most recently from 4ecb380 to a433c26 Compare August 22, 2023 15:09

Configure database connection via db.connection_string param

bd4742c

Needed for flexible configuration of connection parameters required to run MySQL8.

dippindots force-pushed the mysql8 branch from a433c26 to bd4742c Compare August 22, 2023 16:18

dippindots approved these changes Aug 22, 2023

View reviewed changes

dippindots reviewed Aug 25, 2023

View reviewed changes

docs/Migration-Guide.md Outdated Show resolved Hide resolved

dippindots approved these changes Aug 25, 2023

View reviewed changes

Update version in Migration-Guide

e1a0a0a

dippindots merged commit e260eaa into cBioPortal:master Aug 25, 2023
12 of 14 checks passed

dippindots mentioned this pull request Mar 1, 2024

backend localdb not running after MySQL connection url update cBioPortal/icebox#571

Open

averyniceday mentioned this pull request Oct 9, 2023

Replace DB Prop Assertion with Warning #10424

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure database connection via db.connection_string param #10037

Configure database connection via db.connection_string param #10037

pvannierop commented Feb 19, 2023 •

edited by dippindots

sonarcloud bot commented Feb 19, 2023

pvannierop commented Feb 20, 2023

n1zea144 Feb 21, 2023 •

edited

pvannierop Feb 27, 2023 •

edited

inodb Mar 10, 2023

pvannierop Jul 25, 2023

pvannierop commented Mar 6, 2023

dippindots left a comment

inodb commented Apr 5, 2023

sonarcloud bot commented Apr 20, 2023

oplantalech Jul 21, 2023

dippindots Jul 25, 2023 •

edited

dippindots Jul 25, 2023

oplantalech left a comment

dippindots commented Aug 7, 2023

dippindots left a comment

sonarcloud bot commented Aug 25, 2023

dippindots commented Aug 25, 2023

Configure database connection via db.connection_string param #10037

Configure database connection via db.connection_string param #10037

Conversation

pvannierop commented Feb 19, 2023 • edited by dippindots

Problem

Solution

Backward compatibility

Deprecation warning

Verification

sonarcloud bot commented Feb 19, 2023

pvannierop commented Feb 20, 2023

n1zea144 Feb 21, 2023 • edited

Choose a reason for hiding this comment

pvannierop Feb 27, 2023 • edited

Choose a reason for hiding this comment

inodb Mar 10, 2023

Choose a reason for hiding this comment

pvannierop Jul 25, 2023

Choose a reason for hiding this comment

pvannierop commented Mar 6, 2023

dippindots left a comment

Choose a reason for hiding this comment

inodb commented Apr 5, 2023

sonarcloud bot commented Apr 20, 2023

oplantalech Jul 21, 2023

Choose a reason for hiding this comment

dippindots Jul 25, 2023 • edited

Choose a reason for hiding this comment

dippindots Jul 25, 2023

Choose a reason for hiding this comment

oplantalech left a comment

Choose a reason for hiding this comment

dippindots commented Aug 7, 2023

dippindots left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 25, 2023

dippindots commented Aug 25, 2023

pvannierop commented Feb 19, 2023 •

edited by dippindots

n1zea144 Feb 21, 2023 •

edited

pvannierop Feb 27, 2023 •

edited

dippindots Jul 25, 2023 •

edited