Added a fix for POSTGRES_CONNECTION_STRING_PATH handling

[aayushchouhan09]

Describe the Problem

The issue is with the NooBaa core code handling of the POSTGRES_CONNECTION_STRING_PATH parameter. When the NooBaa operator sets this environment variable but the connection string file doesn't exist or is not readable, the code uses fs.readFileSync() directly which throws an unhandled exception. This causes the NooBaa core pod to crash during operator installation, leading to CI job failures with CrashLoopBackOff errors.

Explain the Changes

1. Added proper error handling for POSTGRES_CONNECTION_STRING_PATH by wrapping fs.readFileSync() in a try-catch block.

Issues: Fixed #xxx / Gap #xxx

1. gap: Add missing envs for external pg db noobaa-operator#1689

Testing Instructions:

• Doc added/updated
• Tests added

Summary by CodeRabbit

• New Features

  • Support configuring the database via a single Postgres connection string from an environment variable or a file, with automatic fallback to host/user/password/port/database settings. No breaking changes.

• Bug Fixes

  • Improved error reporting when reading the connection string from a file, providing clearer failure messages to aid troubleshooting.

[coderabbitai]

Walkthrough

Constructor now wraps reading POSTGRES_CONNECTION_STRING_PATH in a try-catch and throws a descriptive Error on read failure. Retrieval precedence (ENV → file → discrete params) and public/exported signatures remain unchanged.

Changes

Cohort / File(s)	Summary
Postgres connection string handling src/util/postgres_client.js	Reading POSTGRES_CONNECTION_STRING_PATH is now wrapped in a try-catch; on read failure the constructor throws Error("Failed to read connection string file '${process.env.POSTGRES_CONNECTION_STRING_PATH}': ${err.message}"). Retains existing precedence (ENV > file > host/user/password/database/port) and no exported API signature changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Env as Environment
  participant PC as PostgresClient
  participant FS as File System
  participant PG as PG Pool

  Env->>PC: POSTGRES_CONNECTION_STRING?
  alt Env var present
    PC->>PC: use env connection_string
  else Env var absent
    Env->>PC: POSTGRES_CONNECTION_STRING_PATH?
    alt Path present
      PC->>FS: read(path) wrapped in try-catch
      alt read success
        FS-->>PC: contents (connection_string)
      else read error
        FS-->>PC: error
        PC->>PC: throw Error("Failed to read connection string file 'PATH': ...")
        Note right of PC: constructor throws — initialization aborts
      end
    else Path absent or empty
      PC->>PC: No connection_string available
    end
  end

  alt connection_string available
    PC->>PG: new Pool({ connectionString, ...params })
  else
    PC->>PG: new Pool({ host,user,password,database,port,...params })
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Added a fix for POSTGRES_CONNECTION_STRING_PATH handling' is directly related to the main change—wrapping fs.readFileSync() for POSTGRES_CONNECTION_STRING_PATH in try-catch error handling to prevent crashes.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/util/postgres_client.js (1)

1426-1439: Override the database in admin ops when using connectionString
pg prioritizes the URL over any database field (so database: undefined won’t unset it). Extract and parse connectionString to point at a known admin DB (e.g. postgres) for createDatabase/dropDatabase. For example:

// in src/util/postgres_client.js
getAdminClientConfig() {
  const base = { ...this.new_pool_params };
  if (base.connectionString) {
    try {
      const u = new URL(base.connectionString);
      u.pathname = '/postgres';
      return { ...base, connectionString: u.toString(), database: undefined };
    } catch {
      return { ...base, database: undefined };
    }
  }
  return { ...base, database: undefined };
}

// then use:
pg_client = new Client(this.getAdminClientConfig());

🧹 Nitpick comments (1)

src/util/postgres_client.js (1)

1501-1508: Normalize the connection string (trim, handle Buffer) and prefer nullish coalescing.

Prevents subtle failures from trailing newlines and treats empty string as “set but unusable” only if you want that.

-const connection_string = process.env.POSTGRES_CONNECTION_STRING ||
-    fs_utils.try_read_file_sync(process.env.POSTGRES_CONNECTION_STRING_PATH);
+const raw_cs = process.env.POSTGRES_CONNECTION_STRING ?? fs_utils.try_read_file_sync(process.env.POSTGRES_CONNECTION_STRING_PATH);
+const connection_string = raw_cs && raw_cs.toString ? raw_cs.toString().trim() : raw_cs;

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0bf3bf6 and 2794be1.

📒 Files selected for processing (1)

• src/util/postgres_client.js (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/util/postgres_client.js (2)

src/hosted_agents/hosted_agents.js (1)

• fs_utils (13-13)

src/server/system_services/cluster_server.js (1)

• fs_utils (17-17)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: run-package-lock-validation
• GitHub Check: Build Noobaa Image
• GitHub Check: run-jest-unit-tests

🔇 Additional comments (1)

src/util/postgres_client.js (1)

1501-1503: Good hardening: stop crashing when POSTGRES_CONNECTION_STRING_PATH is unset.

Using fs_utils.try_read_file_sync avoids the throw from fs.readFileSync and meets the PR goal.

[dannyzaken]

I commented here on the necessary changes in the mentioned operator PR. If there is an environment variable for POSTGRES_CONNECTION_STRING_PATH but the path does not exist, the code should fail instead of silently falling back to the else code.
We might want to catch the error from fs.readFileSync and throw a more descriptive error, so it is clear why the process crashes.

[aayushchouhan09]

I commented here on the necessary changes in the mentioned operator PR. If there is an environment variable for POSTGRES_CONNECTION_STRING_PATH but the path does not exist, the code should fail instead of silently falling back to the else code. We might want to catch the error from fs.readFileSync and throw a more descriptive error, so it is clear why the process crashes.

This makes sense to me. I will remove the unnecessary code and update it to catch the error and throw a more descriptive one. Thanks!

[github-actions]

This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed.