fix(ENGKNOW-3691): accept db credentials from the host application - #134
Conversation
The design spec calls for no public signature changes beyond the new package-private overload of initializeDbSources. parseEnvForDbSourceInstallation and driverClassForUrl were left public despite having no callers outside org.gorpipe.gor.model (only DbConnectionCache itself and UTestDbSource, both in-package, use them). Drop the public modifier on both, keeping them static; parseLinesForDbSourceInstallation is untouched since it is genuine public API used elsewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR extends DbConnectionCache (model module) to support installing a DB source from environment variables (specifically APPSERVER_RDA_*) in addition to the existing TSV credentials-file mechanism, enabling credential rotation via external secret management. It also bumps the project version to 5.11.2 and adds unit tests covering the new env-driven behavior and precedence rules.
Changes:
- Add env-variable parsing (
APPSERVER_RDA_URL/USERNAME/PASSWORD[/DRIVER]) to install anrdaDB source before file-based sources. - Relax missing credentials-file behavior when env-derived sources are present.
- Add unit tests for env parsing, precedence (file overrides env), and missing-file branching.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
VERSION |
Bumps version to 5.11.2. |
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java |
Adds env-derived DB source parsing/installation and adjusts missing-file behavior. |
model/src/test/java/org/gorpipe/gor/model/UTestDbSource.java |
Adds tests for env parsing and env/file interaction behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gor.sql.credentials is retired: gor.db.credentials now carries the additional db resources it was intended for. resolveSqlCredPath falls back to the db credentials path when gor.sql.credentials is unset, so userConnections keeps loading without it. Without this, dropping the property would silently empty userConnections on gor-worker and break every sql:// source. An explicitly configured gor.sql.credentials still wins, so existing setups are unaffected. Also documents which cache each credential source feeds: rotating credentials arrive through APPSERVER_RDA_* for systemConnections, while the credentials file supplies the additional databases reached through userConnections. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:86
haveEnvSourcesis derived from!envParts.isEmpty(), which treats an env source as present even if installation failed (e.g., missing JDBC driver causingClassNotFoundException). That can incorrectly suppress theFileNotFoundExceptionfor a missing credentials file and leave the cache with no sources installed.
List<String[]> envParts = parseEnvForDbSourceInstallation(env);
installAllFromParts(envParts);
installAllFromParts(readFileForDbSourceInstallation(credpath, !envParts.isEmpty()));
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:171
APPSERVER_RDA_PASSWORDis not treated as blank-as-unset: an empty/whitespace value becomes an installed empty password, which changes behavior vs an unset password (e.g., disables prompting via pwdCallback when set to spaces). This also contradicts the stated "blank-as-unset" behavior used for other env vars.
String url = trimToNull(env.get(ENV_RDA_URL));
String user = trimToNull(env.get(ENV_RDA_USERNAME));
String pwd = env.get(ENV_RDA_PASSWORD);
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:297
gor.sql.credentialsis treated as configured whenever the system property is non-null, even if it is blank. With the new dbCredpath fallback, a blank property should be treated as unset so userConnections can fall back to db credentials instead of ending up with no configured sql credential path.
return sqlCredpath != null ? sqlCredpath : dbCredpath;
The db credentials file now carries the additional databases gor can reach, which is exactly what gor.sql.credentials existed for, so the separate source is redundant. Both initInConsoleApp and initInServer now load systemConnections and userConnections from gor.db.credentials. Removes the property, the ~/gor.sql.credentials home-file lookup, and the resolveSqlCredPath fallback introduced earlier on this branch. This is a breaking change for anyone setting the property. Migrates the three tests that configured it (UTestSQLInputSource, UTestInputSourceParsing, UTestSqlSource) to gor.db.credentials, and updates the DbConnection class docs, the database design note, and the SQL/GORSQL/NORSQL command docs. Also treats a blank APPSERVER_RDA_PASSWORD as unset, matching how url and username are already handled. A secret manager or template rendering an empty value would otherwise install it as a real empty password. Covered at both the parse and install level. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
documentation/design/datbase.md:100
- This line still refers to "these files" even though gor.sql.credentials has been removed; it should refer to a single file.
* The separate gor.sql.credentials file has been removed. It previously held the user databases; those now
* live in gor.db.credentials alongside the rest.
* <p><br>
* The format for these files is:
* <pre>
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:93
- When credpath is unset but env-derived sources were installed, this still logs "No db credential path specified", which can be misleading in env-only deployments. Consider logging a message that reflects that env sources are being used (or suppressing the info log when env sources exist).
private List<String[]> readFileForDbSourceInstallation(String credpath, boolean haveEnvSources) throws IOException {
if (credpath == null || credpath.trim().length() == 0) {
log.info("No db credential path specified");
return Collections.emptyList();
}
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:284
- initInServer() only initializes the caches when the gor.db.credentials system property is set. That means env-only configuration (APPSERVER_RDA_*) will be ignored unless the property is present, which conflicts with the stated behavior that env can supply rotating credentials without maintaining a credentials file.
public static void initInServer() throws ClassNotFoundException, IOException {
final String dbCredpath = System.getProperty("gor.db.credentials");
if (dbCredpath != null) {
systemConnections.initializeDbSources(dbCredpath);
userConnections.initializeDbSources(dbCredpath);
}
documentation/src/command/SQL.rst:40
- Typo in option description: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
documentation/src/command/GORSQL.rst:36
- Typo in option description: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
documentation/design/datbase.md:86
- This section still says "two different configuration files" even though the content now describes a single credentials file (with optional env overrides). Updating the wording will keep the design doc consistent with the new behavior.
This issue also appears on line 96 of the same file.
We have two different configuration files:
*
* <li> gor.db.credentials - contains all the databases gor can reach. These feed both the system connections,
APPSERVER_RDA_URL/USERNAME/PASSWORD/DRIVER become GREGOR_DB_URL/USERNAME/ PASSWORD/DRIVER. The old names were borrowed from SM, which GOR does not otherwise depend on; the GREGOR_ prefix keeps them distinct from generic DB_* vars that a base image, sidecar, or platform default might inject. The installed source is still named "rda". LORD_DB_* is unrelated and untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:86
initializeDbSourcesdecides whether a missing credentials file can be tolerated based on!envParts.isEmpty(), butinstallAllFromParts(envParts)can fail (e.g.ClassNotFoundExceptionfor the derived/overridden driver) and still leave zero env sources installed. In that case, a missing file would be silently ignored even though there are no usable sources. Consider basinghaveEnvSourceson what was actually installed (e.g.!mapSources.isEmpty()after env installation) rather than on the parsed env parts list.
List<String[]> envParts = parseEnvForDbSourceInstallation(env);
installAllFromParts(envParts);
installAllFromParts(readFileForDbSourceInstallation(credpath, !envParts.isEmpty()));
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:35
- PR description specifies env vars named
APPSERVER_RDA_URL/USERNAME/PASSWORD(/DRIVER), but the implementation and docs useGREGOR_DB_URL/GREGOR_DB_USERNAME/GREGOR_DB_PASSWORD(/GREGOR_DB_DRIVER). This mismatch will affect deployments relying on the documented variable names. Please align either the implementation/docs/tests toAPPSERVER_RDA_*or update the PR description (and any external deployment docs) to matchGREGOR_DB_*.
static final String ENV_GREGOR_DB_SOURCE_NAME = "rda";
static final String ENV_GREGOR_DB_URL = "GREGOR_DB_URL";
static final String ENV_GREGOR_DB_USERNAME = "GREGOR_DB_USERNAME";
static final String ENV_GREGOR_DB_PASSWORD = "GREGOR_DB_PASSWORD";
static final String ENV_GREGOR_DB_DRIVER = "GREGOR_DB_DRIVER";
documentation/src/command/SQL.rst:40
- Typo: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
documentation/src/command/GORSQL.rst:36
- Typo: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
documentation/design/datbase.md:86
- This section still says "We have two different configuration files" even though the updated content describes a single
gor.db.credentialsfile (and explicitly saysgor.sql.credentialswas removed). Updating the heading avoids contradicting the rest of the section.
We have two different configuration files:
*
* <li> gor.db.credentials - contains all the databases gor can reach. These feed both the system connections,
…ake-gor-tolerant-for-db-password-rotations
…onment Gor no longer reads credentials from the environment. Instead the host application passes them in as DbCredentials, a new public record, via DbConnectionCache.initializeDbSources(String, List) or DbConnection.initInServer(List). Supplied credentials are installed before the file is read, so a file row of the same name still takes precedence. This keeps deployment-specific env var naming out of the library: gor does not care whether the host sourced them from its own config, a secret manager, or environment variables it names itself. toPartsForInstallation converts supplied credentials into the same shape the file parser produces, so installDbSourceFromParts remains the single place that loads drivers and constructs DbConnection. Driver derivation from the url prefix, blank-as-unset handling, and skip-with-warning on incomplete input are unchanged in behaviour, just moved off the env path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (8)
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:57
- The PR title/description says
initializeDbSourcesinstalls an env-derived source fromAPPSERVER_RDA_*, but this implementation never reads environment variables (and there are noAPPSERVER_RDA_*references in the repo). Either implement the env parsing here (e.g., build aDbCredentialsfromSystem.getenv()and pass it to the new overload) or adjust the PR metadata/tests to reflect that credentials are only caller-supplied.
public void initializeDbSources(String credpath) throws IOException {
initializeDbSources(credpath, List.of());
}
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:260
initInConsoleAppno longer considersgor.sql.credentials. This is a breaking config change for deployments/tests that only set that property (or rely on separate db vs sql credential files), and it also contradicts the PR description’s claim that behavior is unchanged when env vars are unset. Consider retaininggor.sql.credentialsas a deprecated fallback (at least whengor.db.credentialsis unset) to preserve compatibility.
File homeDbCredFile = new File(System.getProperty("user.home"), "gor.db.credentials");
final String dbCredpath = homeDbCredFile.exists() ? homeDbCredFile.getCanonicalPath() : System.getProperty("gor.db.credentials");
systemConnections.initializeDbSources(dbCredpath);
userConnections.initializeDbSources(dbCredpath);
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:300
initInServer(List<DbCredentials>)now initializes both caches only fromgor.db.credentials, and skips initialization entirely when that property is unset and no credentials are supplied. If an existing host config only setsgor.sql.credentials(legacy), user connections will silently not be initialized. Consider a deprecated fallback togor.sql.credentialswhengor.db.credentialsis unset, or at least a warning, to avoid surprising breakage.
final String dbCredpath = System.getProperty("gor.db.credentials");
if (dbCredpath != null || !credentials.isEmpty()) {
systemConnections.initializeDbSources(dbCredpath, credentials);
userConnections.initializeDbSources(dbCredpath, credentials);
}
model/src/test/java/org/gorpipe/gor/model/UTestDbSource.java:36
- Unused import
java.util.Map(not referenced anywhere in this test) adds noise and may fail builds if unused-import checks are enabled.
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
model/src/test/java/org/gorpipe/gor/model/UTestDbSource.java:181
- This assertion message says the source is installed "from the environment", but the test is actually installing caller-supplied
DbCredentials. Keeping this message accurate helps when diagnosing failures.
Assert.assertNotNull("rda source should be installed from the environment", rda);
documentation/src/command/SQL.rst:40
- Typo in docs: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
documentation/src/command/GORSQL.rst:36
- Typo in docs: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
documentation/design/datbase.md:85
- This section still says there are "two different configuration files", but the updated text below describes a single
gor.db.credentialsfile and explicitly statesgor.sql.credentialshas been removed. Update the heading to avoid contradicting the rest of the section.
We have two different configuration files:
*
* <li> gor.db.credentials - contains all the databases gor can reach. These feed both the system connections,
The Configuration section was a block of javadoc pasted into markdown - li tags, pre blocks, and a TBD - and still described two credentials files. Rewrites it as markdown covering the two sources that exist now: the gor.db.credentials file and credentials the host passes in as DbCredentials, with the precedence rule between them and the missing-file behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:299
- initInServer now ignores the gor.sql.credentials property entirely and feeds both caches only from gor.db.credentials (plus supplied credentials). This breaks setups that previously configured only gor.sql.credentials for userConnections. Consider supporting gor.sql.credentials as a fallback for userConnections, or documenting/migrating as a breaking change.
public static void initInServer(List<DbCredentials> credentials) throws ClassNotFoundException, IOException {
final String dbCredpath = System.getProperty("gor.db.credentials");
if (dbCredpath != null || !credentials.isEmpty()) {
systemConnections.initializeDbSources(dbCredpath, credentials);
userConnections.initializeDbSources(dbCredpath, credentials);
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:259
- initInConsoleApp no longer considers the gor.sql.credentials property/file at all. This is a behavior-breaking change for deployments that previously configured userConnections via gor.sql.credentials without also setting gor.db.credentials (or having ~/gor.db.credentials). Consider keeping gor.sql.credentials as a fallback for userConnections (at least for one release) or explicitly treating this as a breaking change (versioning/release notes).
This issue also appears on line 295 of the same file.
public static void initInConsoleApp() throws ClassNotFoundException, IOException {
File homeDbCredFile = new File(System.getProperty("user.home"), "gor.db.credentials");
final String dbCredpath = homeDbCredFile.exists() ? homeDbCredFile.getCanonicalPath() : System.getProperty("gor.db.credentials");
systemConnections.initializeDbSources(dbCredpath);
userConnections.initializeDbSources(dbCredpath);
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:66
- PR description/title mention DbConnectionCache reading credentials directly from environment variables (and an initializeDbSources(String, Map) overload), but the implementation here only supports caller-supplied List and does not read env vars. This is a mismatch that could confuse integrators; either update the PR description/title to reflect programmatic credentials injection, or add the env-parsing API described in the PR metadata.
/**
* Read database sources from caller-supplied credentials and from the configuration file.
*
* Sources may come from two places. The caller can pass credentials directly — useful for
* credentials that rotate and so cannot be baked into a file, which the host application may
* source from its own configuration or a secret manager. The credentials file carries the
* static set of additional databases gor can reach.
*
model/src/test/java/org/gorpipe/gor/model/UTestDbSource.java:36
- Unused import: java.util.Map is not referenced in this test file and may fail style checks (or at least adds noise).
import java.io.File;
import java.io.FileNotFoundException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
documentation/src/command/SQL.rst:40
- Typo in documentation: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
documentation/src/command/GORSQL.rst:36
- Typo in documentation: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:291
initInServer(List<DbCredentials>)similarly dropsgor.sql.credentialssupport foruserConnections, which can break existing server deployments that relied on the separate property. Consider keeping it as an override (or explicitly documenting/removing it with a deprecation path).
public static void initInServer(List<DbCredentials> credentials) throws ClassNotFoundException, IOException {
final String dbCredpath = System.getProperty("gor.db.credentials");
if (dbCredpath != null || !credentials.isEmpty()) {
systemConnections.initializeDbSources(dbCredpath, credentials);
userConnections.initializeDbSources(dbCredpath, credentials);
}
model/src/test/java/org/gorpipe/gor/model/UTestDbSource.java:36
- Unused import
java.util.Mapwill cause the test compilation to fail (Java forbids unused imports).
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
documentation/src/command/SQL.rst:40
- Typo in the updated docs: “defied” should be “defined”.
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
model/src/test/java/org/gorpipe/gor/model/UTestDbSource.java:181
- This assertion message says the source is installed “from the environment”, but the test is exercising caller-supplied
DbCredentials(no environment parsing). This makes failures misleading.
DbConnection rda = cache.lookup("rda");
Assert.assertNotNull("rda source should be installed from the environment", rda);
Assert.assertEquals("jdbc:postgresql://db:5432/csa", rda.url);
documentation/src/command/GORSQL.rst:36
- Typo in the updated docs: “defied” should be “defined”.
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:254
initInConsoleApp()no longer considersgor.sql.credentials/~/gor.sql.credentials. That’s a breaking configuration change for users who still have separate db vs sql credential files. If the intent is to consolidate, consider keepinggor.sql.credentialsas a backward-compatible override (at least foruserConnections) and deprecate it separately.
This issue also appears on line 286 of the same file.
public static void initInConsoleApp() throws ClassNotFoundException, IOException {
File homeDbCredFile = new File(System.getProperty("user.home"), "gor.db.credentials");
final String dbCredpath = homeDbCredFile.exists() ? homeDbCredFile.getCanonicalPath() : System.getProperty("gor.db.credentials");
systemConnections.initializeDbSources(dbCredpath);
userConnections.initializeDbSources(dbCredpath);
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:59
- The PR title/description says GOR reads credentials directly from environment variables (e.g.
APPSERVER_RDA_*) and mentions ainitializeDbSources(String, Map)overload. This code explicitly states “it never reads credentials from the environment itself” and there is no env-var parsing in the repo. Please align the PR metadata with the actual implementation (programmaticDbCredentialsinjection), or add the promised env-var behavior.
* Credentials that rotate, and so cannot be baked into a file, can instead be passed in by the host
* application as {@link DbCredentials} — see {@link #initInServer(java.util.List)} and
* {@link DbConnectionCache#initializeDbSources(String, java.util.List)}. Gor does not care where the
* host got them; it never reads credentials from the environment itself. A row in the credentials
* file overrides a supplied source of the same name, so a host that wants its supplied credentials to
* be authoritative must keep that name out of the file.
systemConnections now comes from the credentials the host passes in, and userConnections from gor.db.credentials. Previously both caches were fed from both sources with the file taking precedence. Keeping them apart matches what each is for: system credentials rotate and cannot be baked into a file without going stale, and the rotating system credentials are not the ones user queries should reach. It also removes the collision between the two, so there is no precedence rule left to get wrong. The two initializeDbSources overloads are now alternatives rather than a merge - initializeDbSources(String) reads the file, initializeDbSources(List) installs supplied credentials, and each clears the cache first. initInServer's parameter is renamed systemCredentials to say which cache it feeds. A console app has no host to supply credentials, so initInConsoleApp still loads both caches from the file. Note passing no credentials now leaves the system cache empty rather than falling back to the file, so db:// will not resolve in a server that supplies none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Now that the system and user caches are fed from different sources, which cache an access method resolves against determines which credentials it gets - so the document has to say. Adds that to the notes for each of the four methods: sql commands and sql:// resolve against the user connections, db:// and //db: against the system connections. The SQL, GORSQL and NORSQL command pages now say the databases they reach are the user connections rather than just "the database". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:73
- The PR description and public API list an overload
initializeDbSources(String credpath, List<DbCredentials> credentials)(and describe precedence where file rows override supplied sources, plus a missing-file warning path when credentials were supplied), but the implementation currently only exposesinitializeDbSources(String)andinitializeDbSources(List<DbCredentials>). Without the combined overload, host applications cannot supply rotating creds while still allowing the credentials file to participate (or implement the documented precedence/missing-file behavior).
public void initializeDbSources(List<DbCredentials> credentials) {
clearDbSources();
installAllFromParts(toPartsForInstallation(credentials));
}
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:52
- The class-level Javadoc says
gor.db.credentials"feeds both the system connections" and the user connections, but the updated server initialization explicitly loadssystemConnectionsfrom host-suppliedDbCredentialsand only loadsuserConnectionsfromgor.db.credentials. The current wording is internally inconsistent and could mislead integrators.
* <li> gor.db.credentials - contains all the databases gor can reach. It feeds both the system connections,
* used internally (e.g. session management) and by operations with strict access
* controls (db://, //db:), and the user connections behind the user available
* commands/sources (SQL, GORSQL, NORSQL, sql://).
model/src/test/java/org/gorpipe/gor/model/UTestDbSource.java:224
- There are good unit tests for
toPartsForInstallationand for file-only / supplied-only initialization, but no test covers the behavior described in the PR summary where supplied credentials are installed and then a file row of the same name overrides them (nor the missing-file branch when credentials were supplied). Adding tests for the combined path would help prevent regressions once the combined overload exists.
@Test
public void fileInstallsEveryRow() throws Exception {
String credpath = writeCredentialsFile(
"name\tdriver\turl\tuser\tpwd",
documentation/src/command/SQL.rst:40
- Typo in option description: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
documentation/src/command/GORSQL.rst:36
- Typo in option description: "defied" should be "defined".
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:271
initInServer()now callsinitInServer(List.of()), which clearssystemConnectionsand no longer loads system DB sources from thegor.db.credentialspath. This is a behavior change from the prior implementation (which loaded system connections fromgor.db.credentialswhen set) and conflicts with the PR description’s claim that the no-arg forms are unchanged/additive for existing callers. Either restore the legacy fallback behavior or update the PR description/release notes/tests to reflect the breaking change.
public static void initInServer() throws ClassNotFoundException, IOException {
initInServer(List.of());
}
model/src/main/java/org/gorpipe/gor/model/DbCredentials.java:31
- Javadoc links to a non-existent overload
DbConnectionCache#initializeDbSources(String, List). The only overload taking credentials isinitializeDbSources(List<DbCredentials>), so this link will break Javadoc generation and mislead API consumers.
* {@link DbConnectionCache#initializeDbSources(String, java.util.List)} without gor needing to know
model/src/main/java/org/gorpipe/gor/model/DbCredentials.java:35
- This Javadoc claims supplied credentials are installed before the credentials file is read and that the file takes precedence on name clashes. In the current implementation,
initializeDbSources(List<DbCredentials>)clears the cache and does not read any file, so this precedence behavior is not true unless an additional combined initializer is added.
* Credentials passed this way are installed before the credentials file is read, so a file row with
* the same name takes precedence.
documentation/src/command/GORSQL.rst:36
- Typo: “defied” should be “defined”.
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
model/src/main/java/org/gorpipe/gor/model/DbConnection.java:52
- This bullet says
gor.db.credentialsfeeds both system and user connections, but the Javadoc immediately below states that in server modesystemConnectionscome from host-suppliedDbCredentials. Clarifying this here would prevent confusion about where system credentials are expected to come from.
This issue also appears on line 269 of the same file.
* <li> gor.db.credentials - contains all the databases gor can reach. It feeds both the system connections,
* used internally (e.g. session management) and by operations with strict access
* controls (db://, //db:), and the user connections behind the user available
* commands/sources (SQL, GORSQL, NORSQL, sql://).
documentation/src/command/SQL.rst:40
- Typo: “defied” should be “defined”.
| ``-db database`` | Database alias as defied in ``gor.db.credentials``. |
model/src/main/java/org/gorpipe/gor/model/DbConnectionCache.java:73
- The PR description and
DbCredentialsJavadoc describe an overloadinitializeDbSources(String credpath, List<DbCredentials> credentials)with precedence rules (file overrides supplied) and relaxed missing-file behavior when credentials are supplied. That overload is not present here, and with the current two overloads there is no way to combine supplied credentials with a file read (each call clears the cache), so the documented precedence behavior cannot be achieved.
/**
* Install database sources from credentials supplied by the host application, replacing any
* sources already installed.
*
* This is the counterpart to {@link #initializeDbSources(String)} for credentials that rotate and
Summary
Two related changes to how gor gets its database credentials, plus a
VERSIONbump to 5.11.2.gor.db.credentials. This lets a deployment supply credentials that rotate — from its own configuration, a secret manager, or environment variables it owns the naming of — without gor needing to know where they came from.gor.sql.credentialsis removed. It held the user databases; those now live ingor.db.credentialsalongside the rest.Passing credentials in
New public record and overloads:
The no-arg forms are unchanged and behave exactly as before, so this is additive for existing callers.
toPartsForInstallationconverts them into the sameString[]shape the file parser produces, soinstallDbSourceFromPartsremains the single place that loads drivers and constructsDbConnection.driveris derived from the url prefix (jdbc:postgresql:,jdbc:oracle:).Gor itself reads nothing from the environment.
One behaviour change on the existing path: a missing credentials file previously always threw
FileNotFoundException. It now warns and continues when credentials were supplied, and still throws otherwise.missingFileStillThrowsWithoutSuppliedSourcespins that boundary.Removing
gor.sql.credentialsThe property and its
~/gor.sql.credentialshome-file lookup are gone.initInConsoleAppandinitInServerload bothsystemConnectionsanduserConnectionsfromgor.db.credentials.This is a breaking change for anyone setting that property — the migration is to move those rows into
gor.db.credentials. Worth calling out in the 5.11.2 release notes.Note that removing only the file while leaving the property in place is not a safe middle ground: it would leave
userConnectionsempty in server deployments and silently break everysql://source, sinceSqlSourceroutessql://there and onlydb://tosystemConnections.Tests
UTestDbSourcecovers the credential path end to end: parts-shape conversion, driver derivation for postgres and oracle, explicit driver override, an underivable driver, blank-as-unset for each field, file-overrides-supplied precedence, a file-supplied auxiliary source alongside a supplied one, and both missing-file branches. The four pre-existing file-parser tests are untouched and green — they are the guarantee that file-based behaviour is unchanged.Three tests that configured
gor.sql.credentialswere migrated togor.db.credentials:UTestSQLInputSource,UTestInputSourceParsing,UTestSqlSource.:model:testand:gortools:testgreen.Docs
Rewrote the Configuration section of
documentation/design/datbase.md, which was javadoc pasted into markdown and still described two credentials files. Updated theDbConnectionclass javadoc and theSQL,GORSQL, andNORSQLcommand pages, all of which namedgor.sql.credentialsas the file backing those commands.🤖 Generated with Claude Code