Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/pr-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ jobs:

env:
# Your code can read these, or just use the defaults above
TEST_PUBKY_CONNECTION_STRING: postgres://test_user:test_pass@localhost:5432/postgres?pubky-test=true
TEST_PUBKY_CONNECTION_STRING: postgres://test_user:test_pass@localhost:5432/postgres
Comment thread
86667 marked this conversation as resolved.

strategy:
matrix:
Expand Down Expand Up @@ -193,7 +193,7 @@ jobs:
--health-retries=5
env:
# Your code can read these, or just use the defaults above
TEST_PUBKY_CONNECTION_STRING: postgres://test_user:test_pass@localhost:5433/postgres?pubky-test=true
TEST_PUBKY_CONNECTION_STRING: postgres://test_user:test_pass@localhost:5433/postgres

runs-on: ubuntu-latest
steps:
Expand Down
10 changes: 5 additions & 5 deletions docs/TESTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ docker run --name pubky-postgres \
Then run tests with a test connection string:

```bash
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres?pubky-test=true' \
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres' \
cargo test -p pubky-homeserver --all-features
```

The `?pubky-test=true` parameter tells the test helpers to create an ephemeral `pubky_test_*` database inside the configured PostgreSQL instance. Databases are cleaned up after each test.
In test builds, each test automatically gets its own ephemeral `pubky_test_{uuid}` database on the configured server. Databases are cleaned up after each test.

## Automatic Database Cleanup

Expand Down Expand Up @@ -93,7 +93,7 @@ async fn test_one() {
The [`e2e`](../e2e) crate contains tests that cover cross-crate workflows using `pubky-testnet`. Run them with:

```bash
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres?pubky-test=true' \
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres' \
cargo test -p e2e
```

Expand All @@ -102,7 +102,7 @@ TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgr
Run the homeserver tests against external PostgreSQL:

```bash
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres?pubky-test=true' \
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres' \
cargo test -p pubky-homeserver --all-features
```

Expand All @@ -115,6 +115,6 @@ cargo test -p pubky-testnet --features docker-postgres
Run the full workspace test suite:

```bash
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres?pubky-test=true' \
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres' \
cargo test --workspace --all-features
```
4 changes: 2 additions & 2 deletions examples/rust/7-logging/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ cargo run --bin logging -- --level debug --external-postgres
You can specify a custom connection via the `TEST_PUBKY_CONNECTION_STRING` environment variable:

```bash
TEST_PUBKY_CONNECTION_STRING=postgres://user:pass@localhost:5432/mydb?pubky-test=true cargo run --bin logging -- --level debug --external-postgres
TEST_PUBKY_CONNECTION_STRING=postgres://user:pass@localhost:5432/mydb cargo run --bin logging -- --level debug --external-postgres
```

The `?pubky-test=true` parameter indicates that an ephemeral test database should be created.
Each testnet automatically gets its own ephemeral `pubky_test_{uuid}` database on the configured server.
4 changes: 2 additions & 2 deletions examples/rust/8-testnet/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ cargo run --bin testnet -- --external-postgres
You can specify a custom connection via the `TEST_PUBKY_CONNECTION_STRING` environment variable:

```bash
TEST_PUBKY_CONNECTION_STRING=postgres://user:pass@localhost:5432/mydb?pubky-test=true cargo run --bin testnet -- --external-postgres
TEST_PUBKY_CONNECTION_STRING=postgres://user:pass@localhost:5432/mydb cargo run --bin testnet -- --external-postgres
```

The `?pubky-test=true` parameter indicates that an ephemeral test database should be created.
Each testnet automatically gets its own ephemeral `pubky_test_{uuid}` database on the configured server.
2 changes: 1 addition & 1 deletion examples/rust/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ docker run --name pubky-postgres \
-d postgres:18

# Start the testnet (keep this terminal open)
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres?pubky-test=true' \
TEST_PUBKY_CONNECTION_STRING='postgres://postgres:postgres@localhost:5432/postgres' \
cargo run -p pubky-testnet
```

Expand Down
65 changes: 29 additions & 36 deletions pubky-homeserver/src/app_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ pub enum AppContextConversionError {
/// Failed to open SQL DB.
#[error("Failed to open SQL DB: {0}")]
SqlDb(sqlx::Error),
/// Failed to resolve the database mode (e.g. missing URL or invalid TEST_PUBKY_CONNECTION_STRING).
#[error("Failed to resolve database mode: {0}")]
DatabaseResolution(anyhow::Error),
/// Failed to run migrations.
#[error("Failed to run migrations: {0}")]
Migrations(anyhow::Error),
Expand Down Expand Up @@ -151,7 +154,12 @@ impl AppContext {
.read_or_create_keypair()
.map_err(AppContextConversionError::Keypair)?;

let sql_db = Self::connect_to_sql_db(&conf).await?;
let db_mode = dir
.resolve_database_mode(&conf)
.map_err(AppContextConversionError::DatabaseResolution)?;
let sql_db = SqlDb::connect(db_mode)
.await
.map_err(AppContextConversionError::SqlDb)?;
Migrator::new(&sql_db)
.run()
.await
Expand Down Expand Up @@ -203,7 +211,9 @@ impl AppContext {
fn build_pkarr_builder_from_config(config_toml: &ConfigToml) -> pkarr::ClientBuilder {
let mut builder = pkarr::ClientBuilder::default();
#[cfg(any(test, feature = "testing"))]
if config_toml.general.database_url.is_test_db() {
// In test builds, no explicit database_url means we're in a test environment
// where we must avoid contacting the public DHT.
if config_toml.general.database_url.is_none() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we must avoid contacting the public DHT

This only happens if the database_url is unset. So if a MockDataDir is created with an explicit database_url:

let mut config = ConfigToml::minimal_test_config();
config.general.database_url = Some(postgres_url);

let dir = MockDataDir::new(config, None)?;
HomeserverApp::start_with_mock_data_dir(dir).await?;

then the test can publish test records to the public DHT, and / or fail the test if the environment is sandboxed and outbound connections are denied.

I guess in our test suites we wouldn't do that, but the README documents how library users might write their own tests, so others could inadvertently trigger this edge-case:

let config = ConfigToml::default_test_config();
let mock_dir = MockDataDir::new(config, None).unwrap();
let app = HomeserverApp::start_with_mock_data_dir(mock_dir).await.unwrap();

One solution could be to pass db_mode: &DatabaseMode as an extra arg to build_pkarr_builder_from_config then internally check it directly:

- if config_toml.general.database_url.is_none()
+ if matches!(db_mode, DatabaseMode::EphemeralTest(_))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is equal in fragility as before, when a provide database_url without ?pubky-test=true would not isolate pkarr. I'll fix this properly when i refactor the whole data dir/db/file system config situation, if that okay with you.

builder
.no_default_network()
// Keep the client buildable without contacting the public DHT.
Expand Down Expand Up @@ -235,50 +245,33 @@ impl AppContext {
}
builder
}

/// Connect to the SQL database.
/// If we are in a test environment and it's a test db connection string,
/// we use an empheral test db.
/// Otherwise, we use the normal db connection.
async fn connect_to_sql_db(
config_toml: &ConfigToml,
) -> Result<SqlDb, AppContextConversionError> {
#[cfg(any(test, feature = "testing"))]
{
// If we are in a test environment and it's a test db connection string,
// we use an empheral test db.
return if config_toml.general.database_url.is_test_db() {
Ok(SqlDb::test().await)
} else {
SqlDb::connect(&config_toml.general.database_url)
.await
.map_err(AppContextConversionError::SqlDb)
};
}

#[cfg(not(any(test, feature = "testing")))]
{
// If we are not in a test environment, we use the normal db connection.
return SqlDb::connect(&config_toml.general.database_url)
.await
.map_err(AppContextConversionError::SqlDb);
}
}
}

#[cfg(test)]
mod tests {
use super::*;

/// Verifies that the test pkarr builder doesn't contact the public DHT
/// when database_url is None (the default test config).
#[test]
fn test_pkarr_builder_does_not_use_default_network() {
let builder =
AppContext::build_pkarr_builder_from_config(&ConfigToml::default_test_config());
fn pkarr_builder_does_not_use_default_network() {
let config = ConfigToml::default_test_config();
assert!(
config.general.database_url.is_none(),
"default_test_config should have database_url = None"
);
let builder = AppContext::build_pkarr_builder_from_config(&config);
let builder_debug = format!("{builder:?}");

assert!(builder_debug.contains("127.0.0.1:9"));
assert!(
builder_debug.contains("127.0.0.1:9"),
"expected sentinel bootstrap node in builder: {builder_debug}"
);
for relay in pkarr::DEFAULT_RELAYS {
assert!(!builder_debug.contains(relay));
assert!(
!builder_debug.contains(relay),
"default relay {relay} should not appear in test builder: {builder_debug}"
);
}
builder.build().expect("isolated pkarr client should build");
}
Expand Down
14 changes: 7 additions & 7 deletions pubky-homeserver/src/client_server/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,8 @@ mod tests {
use crate::{
app_context::AppContext,
client_server::ClientServer,
data_directory::{ConfigToml, MockDataDir},
shared::quota::{GlobPattern, HttpMethod, LimitKeyType, PathLimit},
ConfigToml, MockDataDir,
};

#[tokio::test]
Expand Down Expand Up @@ -335,9 +335,9 @@ mod tests {
#[pubky_test_utils::test]
async fn storage_metrics_only_count_resolved_requests_with_low_cardinality_labels() {
let data_dir = MockDataDir::new(ConfigToml::minimal_test_config(), None).unwrap();
let context = AppContext::read_from(data_dir).await.unwrap();
let context = Arc::new(AppContext::read_from(data_dir).await.unwrap());
let metrics = context.metrics.clone();
let router = ClientServer::create_router(Arc::new(context)).unwrap();
let router = ClientServer::create_router(Arc::clone(&context)).unwrap();
let server = TestServer::new(router).unwrap();
let user = Keypair::random();
let cookie = signup_cookie(&server, &user).await;
Expand Down Expand Up @@ -380,25 +380,25 @@ mod tests {
.collect::<Vec<_>>();

assert_eq!(samples.len(), 4, "unexpected metric samples:\n{output}");
assert!(samples.iter().any(|sample| {
assert!(samples.iter().any(|sample: &&str| {
sample.contains("addressing_mode=\"path\"")
&& sample.contains("auth_method=\"cookie\"")
&& sample.contains("pubky_host_header=\"matching\"")
&& sample.contains("pubky_host_query=\"true\"")
}));
assert!(samples.iter().any(|sample| {
assert!(samples.iter().any(|sample: &&str| {
sample.contains("addressing_mode=\"legacy\"")
&& sample.contains("auth_method=\"none\"")
&& sample.contains("pubky_host_header=\"matching\"")
&& sample.contains("pubky_host_query=\"false\"")
}));
assert!(samples.iter().any(|sample| {
assert!(samples.iter().any(|sample: &&str| {
sample.contains("addressing_mode=\"path\"")
&& sample.contains("auth_method=\"none\"")
&& sample.contains("pubky_host_header=\"other\"")
&& sample.contains("pubky_host_query=\"false\"")
}));
assert!(samples.iter().any(|sample| {
assert!(samples.iter().any(|sample: &&str| {
sample.contains("addressing_mode=\"legacy\"")
&& sample.contains("auth_method=\"none\"")
&& sample.contains("pubky_host_header=\"absent\"")
Expand Down
5 changes: 3 additions & 2 deletions pubky-homeserver/src/data_directory/config_toml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ pub struct GeneralToml {
)]
#[serde(default)]
pub user_storage_quota_mb: u64,
pub database_url: ConnectionString,
#[serde(default)]
pub database_url: Option<ConnectionString>,
}

/// A config for Homeserver tracing subscriber configuration
Expand Down Expand Up @@ -213,7 +214,7 @@ impl ConfigToml {
#[cfg(any(test, feature = "testing"))]
pub fn default_test_config() -> Self {
let mut config = Self::default();
config.general.database_url = ConnectionString::default_test_db(); // Mark this db as test. This indicates that the db is not real.
config.general.database_url = None; // Resolved downstream via env var or default fallback.
config.general.signup_mode = SignupMode::Open;
// Use ephemeral ports (0) so parallel tests don't collide.
config.drive.icann_listen_socket = SocketAddr::from(([127, 0, 0, 1], 0));
Expand Down
6 changes: 6 additions & 0 deletions pubky-homeserver/src/data_directory/data_dir.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use super::ConfigToml;
use crate::persistence::sql::DatabaseMode;
use dyn_clone::DynClone;
use std::path::Path;

Expand All @@ -20,6 +21,11 @@ pub trait DataDir: std::fmt::Debug + DynClone + Send + Sync {
/// Reads the secret file from the data directory.
/// Creates a new secret file if it doesn't exist.
fn read_or_create_keypair(&self) -> anyhow::Result<pubky_common::crypto::Keypair>;

/// Resolve how the homeserver should connect to its database.
///
/// Each implementation selects the appropriate database lifecycle.
fn resolve_database_mode(&self, conf: &ConfigToml) -> anyhow::Result<DatabaseMode>;
}

dyn_clone::clone_trait_object!(DataDir);
38 changes: 38 additions & 0 deletions pubky-homeserver/src/data_directory/mock_data_dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ impl DataDir for MockDataDir {
self.temp_dir.path()
}

/// Creates a temporary database with [`DatabaseMode::EphemeralTest`](crate::persistence::sql::DatabaseMode::EphemeralTest).
fn resolve_database_mode(
&self,
conf: &super::ConfigToml,
) -> anyhow::Result<crate::persistence::sql::DatabaseMode> {
crate::persistence::sql::DatabaseMode::resolve_test(conf.general.database_url.clone())
}

fn ensure_data_dir_exists_and_is_writable(&self) -> anyhow::Result<()> {
Ok(()) // Always ok because this is validated by the tempfile crate.
}
Expand All @@ -68,3 +76,33 @@ impl DataDir for MockDataDir {
Ok(self.keypair.clone())
}
}

#[cfg(test)]
mod tests {
use super::*;
use crate::persistence::sql::{ConnectionString, DatabaseMode};

#[test]
fn resolve_database_mode_returns_ephemeral() {
let mock = MockDataDir::test();
let conf = mock.config_toml.clone();
let mode = mock.resolve_database_mode(&conf).unwrap();
assert!(
matches!(mode, DatabaseMode::EphemeralTest(_)),
"MockDataDir should always resolve to EphemeralTest"
);
}

#[test]
fn resolve_database_mode_with_explicit_url_returns_ephemeral() {
let mut config = super::super::ConfigToml::default_test_config();
config.general.database_url =
Some(ConnectionString::new("postgres://custom:5432/mydb").unwrap());
let mock = MockDataDir::new(config.clone(), None).unwrap();
let mode = mock.resolve_database_mode(&config).unwrap();
assert!(
matches!(mode, DatabaseMode::EphemeralTest(_)),
"MockDataDir should return EphemeralTest even with an explicit URL"
);
}
}
47 changes: 47 additions & 0 deletions pubky-homeserver/src/data_directory/persistent_data_dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,22 @@ impl DataDir for PersistentDataDir {
&self.expanded_path
}

/// Connects to the configured URL with [`DatabaseMode::Direct`](crate::persistence::sql::DatabaseMode::Direct).
fn resolve_database_mode(
&self,
conf: &ConfigToml,
) -> anyhow::Result<crate::persistence::sql::DatabaseMode> {
conf.general
.database_url
.clone()
.map(crate::persistence::sql::DatabaseMode::Direct)
.ok_or_else(|| {
anyhow::anyhow!(
"No database_url configured. Set [general].database_url in config.toml."
)
})
}

/// Makes sure the data directory exists.
/// Create the directory if it doesn't exist.
fn ensure_data_dir_exists_and_is_writable(&self) -> anyhow::Result<()> {
Expand Down Expand Up @@ -247,6 +263,37 @@ mod tests {
assert_eq!(content, "test");
}

#[test]
fn resolve_database_mode_returns_direct_when_url_set() {
use crate::persistence::sql::{ConnectionString, DatabaseMode};

let mut conf = ConfigToml::default();
conf.general.database_url =
Some(ConnectionString::new("postgres://localhost:5432/mydb").unwrap());

let temp_dir = TempDir::new().unwrap();
let data_dir = PersistentDataDir::new(temp_dir.path().to_path_buf());
let mode = data_dir.resolve_database_mode(&conf).unwrap();
assert!(
matches!(mode, DatabaseMode::Direct(_)),
"PersistentDataDir should resolve to Direct"
);
}

#[test]
fn resolve_database_mode_errors_when_no_url() {
let mut conf = ConfigToml::default();
conf.general.database_url = None;

let temp_dir = TempDir::new().unwrap();
let data_dir = PersistentDataDir::new(temp_dir.path().to_path_buf());
let result = data_dir.resolve_database_mode(&conf);
assert!(
result.is_err(),
"PersistentDataDir should error when database_url is None"
);
}

#[test]
pub fn test_trim_secret_file_content() {
let temp_dir = TempDir::new().unwrap();
Expand Down
2 changes: 1 addition & 1 deletion pubky-homeserver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub use data_directory::{
};
pub use homeserver_app::{HomeserverApp, HomeserverAppBuildError};
pub use metrics_server::{MetricsServer, MetricsServerBuildError};
pub use persistence::sql::ConnectionString;
pub use persistence::sql::{ConnectionString, DatabaseMode};
pub use shared::quota::{
BandwidthQuota, DefaultQuotasToml, GlobPattern, HttpMethod, LimitKey, LimitKeyType, PathLimit,
RequestCountQuota, TimeUnit,
Expand Down
Loading
Loading