Expected configuration format for SqlService is ambiguous

[TheE]

Sponge's SqlService provides managed JDBC DataSources to clients. A DataSource is requested by providing the configuration as String. The format of this String is defined in the API specification as follows:

jdbc:<engine>://[<username>[:<password>]@]<host>/<schema>

This is problematic: schema may be omitted because not every supported DBMS requires one (they are optional for H2, and not supported by SQLite). In these cases host may still include slashes as the the host is a path to file on the system. As a result, Sponge is unable to know whether a database is given or not.

Example

For example, look at this:

jdbc:h2:username:password@./path/to/dabase/file

The user expects Sponge to connect to a H2 database file under ./path/to/dabase/file that does not use any schema. Instead, Sponge should (from the API specification) connect to a database file under ./path/to/dabase/ with the schema file.

Even worse for SQLite:

jdbc:sqlite:./path/to/dabase/file

Sponge should try to parse file as a schema even if SQLite does not support schemas.

Fix

The simple fix would be to change the separator between host and schema. Instead of / a character (or multiple characters) should be used, that cannot be used within an URL or path - e.g. //. Obviously, this would break existing configurations but this is inevitable.

[Xfel]

The issue here is that sponge expects the URLs to be formatted in a specific way, where it really should just allow any valid JDBC url. Changing the syntax here would definetely do more harm then good, since for now it is at least consistent with the majority of JDBC drivers. Instead, any further requirements should be lifted.

[ryantheleach]

To future readers wanting context of the implementation: https://github.com/SpongePowered/SpongeCommon/blob/bleeding/src/main/java/org/spongepowered/common/service/sql/SqlServiceImpl.java#L242

[TheE]

The issue here is that sponge expects the URLs to be formatted in a specific way, where it really should just allow any valid JDBC url.

While I agree, the problem is that HikariCP, the library Sponge uses for connection pooling, requires username and password to be given separately from the JDBC url (see here). Without any specification, it would be impossible to extract these information from the JDBC url as different databases require different formats.

It might however be technically possible to change SqlService to something like the following (although handling of aliases would be awkward):

getDataSource(String jdbcUrl, @Nullable String username, @Nullable password);
getDataSource(String jdbcUrl, @Nullable Properties connectionProperties);

Changing the syntax here would definetely do more harm then good, since for now it is at least consistent with the majority of JDBC drivers. Instead, any further requirements should be lifted.

While this format is indeed supported by major DBMS like MySQL or PostgreSQL, it neither supports SQLite nor H2 (it does not support schema less databases)

From my own experience as developer and server admin I am inclined to say that most users are probably using these flat-file databases. (Even if they are not, I doubt that any noticeable number of users opts for something different than MySQL or MariaDB).

[ryantheleach]

@TheE If I can get confirmation from one the SpongeDevs that there isn't some alternate method that flat file databases can use, then I completely agree that this is something that needs fixing.

I'm honestly a little surprised it hasn't come up by now, but maybe people just chalked it down as not working, and accessed databases directly rather then using the aliases. It certainly would make sense for some of the plugin configurations I've seen.

[XakepSDK]

1. Add way to provide plugin directory to jdbc url(placeholder)
2. Add way to provide connection properties(Properties class)
3. For aliases - make complex config node, like this:

   "aliasName": {
   "url":"jdbc:...",
   "properties": {
   "username": "root",
   "password": "myPassword",
   "dataSource.XXXX": "propVal"
   }
   }

   (persistence.xml! just do it! =))

[ryantheleach]

I've labelled this as 1.13 as I'd prefer to keep a potential change to the configuration nodes / alias inputs to a major version change, even if it doesn't break the API compatibility directly, considering how long this has been without change.

[ryantheleach]

@onyn If we can get a response from @SpongePowered/developers would you be interested in championing this issue / PR?

[ghost]

I would gladly address special parsing for file-based database setups in addition to my current PR, but I would really prefer to work on it atop my current changes than create two parallel PRs in a “choose one” fashion.

I originally posted this on the wrong thing...

[ryantheleach]

@brjcarbone It wouldn't be choose one.

I would not want this faster then API 8. Where as I could see the other PR (Potentially, dev team approving) introduced in a minor version bump of API 7.

[onyn]

I did not catch what the problem is. Sponge do not parse all the url and do nothing with any url parts beyond login:password pair. I made some tests on urls, that was provided by @TheE and there is no problems:

    @Test
    public void testSchemaMissH2() throws SQLException {
        final String jdbcUrl = "jdbc:h2:username:password@./path/to/dabase/file";
        final SqlServiceImpl.ConnectionInfo subject = SqlServiceImpl.ConnectionInfo.fromUrl(null, jdbcUrl);

        assertEquals("username", subject.getUser());
        assertEquals("password", subject.getPassword());
        assertEquals("jdbc:h2:./path/to/dabase/file", subject.getAuthlessUrl());
        assertEquals("org.h2.Driver", subject.getDriverClassName());
    }

    @Test
    public void testSchemaMissSqlite() throws SQLException {
        final String jdbcUrl = "jdbc:sqlite:./path/to/dabase/file";
        final SqlServiceImpl.ConnectionInfo subject = SqlServiceImpl.ConnectionInfo.fromUrl(null, jdbcUrl);

        assertNull(subject.getUser());
        assertNull(subject.getPassword());
        assertEquals("jdbc:sqlite:./path/to/dabase/file", subject.getAuthlessUrl());
        assertEquals("org.sqlite.JDBC", subject.getDriverClassName());
    }

The problem is with poor documentation, not with the code. More examples and clarifications in the javadoc of SqlService#getDataSource methods may be sufficient to resolve this issue.

While I agree, the problem is that HikariCP, the library Sponge uses for connection pooling, requires username and password to be given separately from the JDBC url (see here). Without any specification, it would be impossible to extract these information from the JDBC url as different databases require different formats.

This is not true. HikariCP do not mandate how credentials must be set. They may be passed via setUsername/setPassword methods, but setting them in the jdbc url works equally fine.

[ryantheleach]

@onyn After reading your reply, I believe you are correct. I mistook the original issue to be saying that we were somehow exposing schema's, and (changing the jdbc url spec further then just the username/password stuff we already do) when it is just all passed through as you say.

[TheE]

The problem is with poor documentation, not with the code. More examples and clarifications in the javadoc of SqlService#getDataSource methods may be sufficient to resolve this issue.

You are right. Thank you for making theses tests! The problem is that the following documentation of SqlService.getDataSource(String) is wrong:

A jdbc connection url is expected to be of the form: jdbc:<engine>://[<username>[:<password>]@]<host >/<database> or an alias (available aliases are known only by the service provider)

Instead, the correct form is this one: jdbc:<engine>://[<username>[:<password>]@]<driver-specific-jdbc-url>. Perhaps, this could be expanded with some examples demonstrating what exactly 'driver-specific' means, e.g. examples for H2, SQLite and MySQL.

I can submit a PR for this change if needed.

This is not true. HikariCP do not mandate how credentials must be set. They may be passed via setUsername/setPassword methods, but setting them in the jdbc url works equally fine.

May I ask if you tested this? Reading the HikariCP documentation I am not entirely sure as they recommend setting username and password within the connection properties passed to HikariCP - of course this does not imply that setting them in the connection URL does not work.

However, if it really works, why was this configuration format for SqlService.getDataSource(String) introduced when it could, following @Xfel's suggestion above, simply accept any JDBC URL?

[onyn]

However, if it really works, why was this configuration format for SqlService.getDataSource(String) introduced when it could, following @Xfel's suggestion above, simply accept any JDBC URL?

I don't know. Better to ask original commiter of this code - @Minecrell

May I ask if you tested this?

Yes. You can verify it by yourself. I wrote this tests just for you:

click to expand

```java public void testMysqlUrlCredentials() throws SQLException { HikariConfig hc = new HikariConfig(); // you need to manually create user before running this test hc.setJdbcUrl("jdbc:mysql://localhost/?user=abc&password=def"); try ( HikariDataSource ds = new HikariDataSource(hc); Connection conn = ds.getConnection(); Statement stmt = conn.createStatement(); ResultSet rs = stmt.executeQuery("SELECT VERSION()") ) { if (rs.next()) { System.out.println("MySQL version: " + rs.getString(1)); } } } public void testH2UrlCredentials() throws SQLException, IOException { try { String initUrl = "jdbc:h2:./test.h2"; String checkUrl = "jdbc:h2:./test.h2;USER=abc;PASSWORD=def"; HikariConfig hc = new HikariConfig(); hc.setJdbcUrl(initUrl); try ( HikariDataSource ds = new HikariDataSource(hc); Connection conn = ds.getConnection(); Statement stmt = conn.createStatement(); ) { stmt.executeUpdate("CREATE USER abc PASSWORD 'def' ADMIN"); } hc = new HikariConfig(); hc.setJdbcUrl(checkUrl); try ( HikariDataSource ds = new HikariDataSource(hc); Connection conn = ds.getConnection(); Statement stmt = conn.createStatement(); ResultSet rs = stmt.executeQuery("SHOW SCHEMAS") ) { System.out.println("H2 default schemas:"); while (rs.next()) { System.out.println(rs.getString(1)); } } } finally { String[] rmList = new String[] { "./test.h2.mv.db", "./test.h2.trace.db" }; for (String file : rmList) { Path path = Paths.get(file); if (Files.exists(path)) { Files.delete(path); } } } } ``` ``` [17:25:33] [main/INFO]: HikariPool-1 - Starting... [17:25:33] [main/INFO]: HikariPool-1 - Start completed. MySQL version: 5.7.24 [17:25:33] [main/INFO]: HikariPool-2 - Starting... [17:25:33] [main/INFO]: HikariPool-2 - Start completed. [17:25:33] [main/INFO]: HikariPool-2 - Shutdown initiated... [17:25:33] [main/INFO]: HikariPool-2 - Shutdown completed. [17:25:33] [main/INFO]: HikariPool-3 - Starting... [17:25:33] [main/INFO]: HikariPool-3 - Start completed. H2 default schemas: INFORMATION_SCHEMA PUBLIC [17:25:33] [main/INFO]: HikariPool-3 - Shutdown initiated... [17:25:33] [main/INFO]: HikariPool-3 - Shutdown completed. ```

[ghost]

There's a fundamental misunderstanding here. The regex parser does not require a username/password because that group is optional and can identify paths, e.g. jdbc:sqlite:./path/to/dabase/file, perfectly fine. Test it yourself. Changing the input method will not change the efficacy of the SqlService.

[onyn]

There is no need in duplicating functionality. Less code - less bugs and less maintenance burden.

[ghost]

There would never be two options which produce the same result concurrently, but the point is that the regex system is perfect. It cannot be replaced with something "better."