Support JDBC-style connection URLs in connect(String)#188
Conversation
Add JdbcConnectionUrl, a package-private parser for jdbc:surrealdb:... URLs, and wire it into Surreal.connect(String) so callers can specify host, port, namespace, database, and root credentials in a single connection string instead of separate connect/useNs/useDb/signin calls. Non-JDBC connect strings are unaffected.
Independent verification of the JDBC-style connection URL support (surrealdb#105) turned up two real bugs, confirmed against a live SurrealDB server: - The native `any` engine dispatch matches scheme tokens case-sensitively (e.g. only lowercase "ws"/"http"), so an upper/mixed-case scheme such as "jdbc:surrealdb:WS://..." was passed through verbatim and the connection was rejected with "Invalid URL: ..." even though this parser accepts the scheme token case-insensitively. Fixed by lowercasing the scheme before building the native connect string. - An empty namespace or database path segment (e.g. "host//db" or "host/ns//") was silently parsed as namespace/database = "" instead of null, which would go on to trigger a confusing useNs("")/useDb("") call. These now throw a clear IllegalArgumentException at parse time instead. Also improves the "missing host" error message to echo back the URL the caller actually passed instead of an internally-rewritten form (relevant when the implicit default-ws-scheme rule spliced in a "ws:" prefix), and adds regression tests for these cases plus IPv6 host literals and percent-encoded passwords containing reserved delimiter characters. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
emmanuel-keller
left a comment
emmanuel-keller
left a comment
There was a problem hiding this comment.
Review — request changes.
Nice addition overall — the parser is well structured and comes with strong coverage (29 parser unit tests plus integration tests for ws/memory/surrealkv and the uppercase-scheme regression). There's one blocking issue (inline, on the URLDecoder usage) plus a few smaller points.
Also: no CHANGELOG.md entry was added — recent PRs (#183/#184/#185) each add one referencing the PR number; please add one here too.
| return preferred != null ? preferred : fallback; | ||
| } | ||
|
|
||
| private static String decode(String value) { |
There was a problem hiding this comment.
Blocking. URLDecoder performs application/x-www-form-urlencoded decoding, so it turns a literal + into a space. That's correct for query-string values but wrong for URI userinfo and path components.
Concrete failure: jdbc:surrealdb://root:pa+ss@localhost:8000 parses the password as pa ss, so the root sign-in fails auth with no diagnostic. + is a common password character, and the existing percentEncodedPasswordWithSpecialCharacters test covers @ / : but not +, so CI won't catch it.
Suggest decoding userinfo/path with a percent-only decoder (expand %XX, leave + untouched) and keeping form-decoding for the query-string values.
There was a problem hiding this comment.
Fixed in 187c7ec. Reproduced first: jdbc:surrealdb://ro+ot:pa+ss@localhost:8000 parsed the password as pa ss (and /my+ns/my+db path segments were mangled the same way). Added a private percentDecode() that expands %XX (incl. multi-byte UTF-8, by re-encoding literal + as %2B before delegating to URLDecoder) and used it for userinfo and path segments; query-string values keep form-decoding, where + means space. New tests cover + in userinfo and path, %2B, multi-byte UTF-8 in userinfo/path, and pin the +-to-space behavior for query values.
| if (parsed.getUsername() != null) { | ||
| signin(new RootCredential(parsed.getUsername(), parsed.getPassword())); | ||
| } | ||
| if (parsed.getNamespace() != null) { |
There was a problem hiding this comment.
Non-blocking: useDb(...) is nested inside the getNamespace() != null guard, so a database provided without a namespace (reachable via ?db=foo) is silently dropped. SurrealDB can't select a db without a ns anyway, but silently ignoring the param is surprising — consider throwing or documenting.
There was a problem hiding this comment.
Fixed in 187c7ec — went with throwing. JdbcConnectionUrl.parse() now validates cross-field consistency after merging the userinfo/path and query-string sources, so ?db=foo with no namespace from either source throws IllegalArgumentException("database supplied without a namespace") instead of being silently dropped; this matches the parser's existing fail-loud handling of empty segments and missing hosts, and covers the memory/embedded forms too. Mixed sources remain valid (/ns?db=foo). With the invariant enforced at parse time, the useDb call in connect() is un-nested from the namespace guard. Tests: databaseWithoutNamespaceThrows, databaseWithoutNamespaceThrowsForMemory, pathNamespaceWithQueryStringDatabase.
| hostPort = remainder.substring(atIdx + 1); | ||
| final int colonIdx = userinfo.indexOf(':'); | ||
| if (colonIdx >= 0) { | ||
| userinfoUser = decode(userinfo.substring(0, colonIdx)); |
There was a problem hiding this comment.
Non-blocking: empty userinfo (jdbc:surrealdb://@localhost:8000) yields username="" rather than null, which then triggers a root sign-in with empty credentials. Consider normalizing empty userinfo to null (and/or requiring a password when a username is present).
There was a problem hiding this comment.
Fixed in 187c7ec. An empty userinfo (@host, and :@host) is now normalized to null credentials, so no sign-in is attempted, and it no longer shadows query-string credentials (jdbc:surrealdb://@host?user=root&pass=root works). Also took the second suggestion: credentials must be complete after merging both sources — a username without a password now throws (passing a null password through to the native signin fails with an unhelpful null-pointer error, so failing fast at parse seemed better), as does a password without a username; one source may still complete the other (root@host?pass=secret). An explicitly empty password (root:@host) is still accepted and fails at the server with a genuine auth error. Semantics are documented in the class javadoc and connect()'s @throws.
| @Test | ||
| void trailingSlashAfterNamespaceOnlyIsAllowed() { | ||
| // A single trailing slash with no database segment is fine (not the same as | ||
| // an empty database segment): "ns/" means "namespace=ns, no database". |
There was a problem hiding this comment.
Non-blocking: trailingSlashAfterNamespaceOnlyIsAllowed actually parses .../ns (no trailing slash); the real .../ns/ case isn't exercised. Worth renaming or adding the actual trailing-slash case.
There was a problem hiding this comment.
Fixed in 187c7ec. trailingSlashAfterNamespaceOnlyIsAllowed now parses the actual .../ns/ form (it does parse as namespace-only, so this was a coverage gap, not a code bug — the bare .../ns case was already covered by wsWithNamespaceOnlyPath), and added trailingSlashAfterNamespaceAndDatabaseIsAllowed for .../ns/db/.
…RL fields
- Decode userinfo and path segments with a percent-only decoder so a
literal '+' is preserved (RFC 3986); URLDecoder's form-decoding, where
'+' means space, now applies only to query-string values. Previously
"jdbc:surrealdb://root:pa+ss@localhost:8000" silently parsed the
password as "pa ss" and failed auth with no diagnostic.
- Reject half-specified combinations after merging userinfo/path and
query-string sources instead of silently dropping or passing them
through: a database without a namespace, a username without a
password, and a password without a username now throw
IllegalArgumentException. One source may still complete the other
(e.g. "root@host?pass=secret").
- Normalize an empty userinfo ("@host" or ":@host") to null
credentials so no sign-in is attempted, rather than signing in as
user "".
- Exercise the actual trailing-slash cases ("/ns/" and "/ns/db/") in
JdbcConnectionUrlTests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Closes #105
What this adds
Surreal.connect(String)now accepts a JDBC-style connection URL of the formjdbc:surrealdb:...as a convenience shorthand, in addition to the nativeconnect strings it already understood (
ws://...,memory,surrealkv://...,etc). When a JDBC-style URL is given,
connect()transparently connects,signs in (if credentials were supplied), and selects namespace/database (if
supplied) - equivalent to chaining
connect()+signin(Credential)+useNs()+useDb()manually.Example URLs:
jdbc:surrealdb://root:root@localhost:8000/my_ns/my_dbjdbc:surrealdb:memory?ns=my_ns&db=my_dbjdbc:surrealdb:surrealkv:///var/lib/surreal/data?ns=my_ns&db=my_dbhttp:///https://remote schemes and query-string aliases(
user/username,pass/password,ns/namespace,db/database) arealso supported; see the Javadoc on
Surreal.connect(String)and the newJdbcConnectionUrlclass for the full grammar.Credentials supplied this way always authenticate as root. Namespace- or
database-scoped users, or record users, cannot be represented in this
shorthand - callers that need those should keep using the existing
connect(String)+signin(Credential)path directly.Tests added
src/test/java/com/surrealdb/JdbcConnectionUrlTests.java(new, 29 unittests for the parser): remote ws/wss/http/https URLs with and without
userinfo/path/port, the default-to-
wsrule for a barejdbc:surrealdb://,query-string credential/ns/db aliases and their precedence vs userinfo/path,
memory/mem://handling, arbitrary embedded/file engine passthrough(
surrealkv://,rocksdb://), and error cases (missing prefix, wrongsubprotocol, null input, missing host, too many path segments, empty
namespace/database path segments, IPv6 host literals, percent-encoded
passwords containing reserved delimiter characters, and scheme
case-normalization).
src/test/java/com/surrealdb/ConnectionTests.java(modified): addedjdbcConnectWebsocket,jdbcConnectWebsocketUppercaseScheme,jdbcConnectMemory, andjdbcConnectSurrealKV, exercising the JDBC URLpath end-to-end against a real SurrealDB instance.
No native/Rust code was touched -
connect()on the native side is anunmodified thin passthrough to the
surrealdb::engine::anyconnector; all ofthe new logic lives in the Java-side URL parser.
Why draft
Opening this as a draft for maintainer review: this repo's
CONTRIBUTING.mdasks that new feature ideas normally go through a GitHubdiscussion first before a pull request. This PR was scoped directly from an
open, unclaimed feature-request issue (#105) rather than a prior discussion
thread, so flagging that here for maintainers to weigh in on scope/design
before this is considered for merge.