Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Add postgres dependency for server to startup #36585

Merged
merged 9 commits into from
Sep 30, 2024
92 changes: 92 additions & 0 deletions deploy/docker/fs/opt/appsmith/pg-utils.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
#!/bin/bash

waitForPostgresAvailability() {
if [ -z "$PG_DB_HOST" ]; then
tlog "PostgreSQL host name is empty. Check env variables. Error. Exiting java setup"
exit 2
else

MAX_RETRIES=50
RETRYSECONDS=10
retry_count=0
while true; do
su postgres -c "pg_isready -h '${PG_DB_HOST}' -p '${PG_DB_PORT}'"
status=$?

case $status in
0)
tlog "PostgreSQL host '$PG_DB_HOST' is ready."
break
;;
1)
tlog "PostgreSQL host '$PG_DB_HOST' is rejecting connections e.g. due to being in recovery mode or not accepting connections eg. connections maxed out."
;;
2)
tlog "PostgreSQL host '$PG_DB_HOST' is not responding or running."
;;
3)
tlog "The connection check failed e.g. due to network issues or incorrect parameters."
;;
*)
tlog "pg_isready exited with unexpected status code: $status"
break
;;
esac

retry_count=$((retry_count + 1))
if [ $retry_count -le $MAX_RETRIES ]; then
tlog "PostgreSQL connection failed. Retrying attempt $retry_count/$MAX_RETRIES in $RETRYSECONDS seconds..."
sleep $RETRYSECONDS
else
tlog "Exceeded maximum retry attempts ($MAX_RETRIES). Exiting."
# use exit code 2 to indicate that the script failed to connect to postgres and supervisor conf is set not to restart the program for 2.
exit 2
fi

done
fi
}

# for PostgreSQL, we use APPSMITH_DB_URL=postgresql://username:password@postgresserver:5432/dbname
# Args:
# conn_string (string): PostgreSQL connection string
# Returns:
# None
# Example:
# postgres syntax
# "postgresql://user:password@localhost:5432/appsmith"
# "postgresql://user:password@localhost/appsmith"
# "postgresql://user@localhost:5432/appsmith"
# "postgresql://user@localhost/appsmith"
extract_postgres_db_params() {
local conn_string=$1

# Use node to parse the URI and extract components
IFS=' ' read -r USER PASSWORD HOST PORT DB <<<$(node -e "
const connectionString = process.argv[1];
const pgUri = connectionString.startsWith(\"postgresql://\")
? connectionString
: 'http://' + connectionString; //Prepend a fake scheme for URL parsing
const url = require('url');
const parsedUrl = new url.URL(pgUri);

// Extract the pathname and remove the leading '/'
const db = parsedUrl.pathname.substring(1);

// Default the port to 5432 if it's empty
const port = parsedUrl.port || '5432';

console.log(\`\${parsedUrl.username || '-'} \${parsedUrl.password || '-'} \${parsedUrl.hostname} \${port} \${db}\`);
" "$conn_string")
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 27, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Quote Command Substitution to Prevent Word Splitting

In the extract_postgres_db_params function, the output of the node command should be enclosed in quotes to prevent word splitting. Without quotes, parameters containing spaces could lead to incorrect parsing of database credentials.

Here's how you can fix it:

-    IFS=' ' read -r USER PASSWORD HOST PORT DB <<<$(node -e "
+    IFS=' ' read -r USER PASSWORD HOST PORT DB <<<"$(node -e "
      const connectionString = process.argv[1];
      const pgUri = connectionString.startsWith(\"postgresql://\")
        ? connectionString
        : 'http://' + connectionString; // Prepend a fake scheme for URL parsing
      const url = require('url');
      const parsedUrl = new url.URL(pgUri);

      // Extract the pathname and remove the leading '/'
      const db = parsedUrl.pathname.substring(1);

      // Default the port to 5432 if it's empty
      const port = parsedUrl.port || '5432';

      console.log(\`\${parsedUrl.username || '-'} \${parsedUrl.password || '-'} \${parsedUrl.hostname} \${port} \${db}\`);
-    " "$conn_string")
+    " "$conn_string")"

By quoting the command substitution, you ensure that all the variables are read correctly, even if they contain spaces.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
IFS=' ' read -r USER PASSWORD HOST PORT DB <<<$(node -e "
const connectionString = process.argv[1];
const pgUri = connectionString.startsWith(\"postgresql://\")
? connectionString
: 'http://' + connectionString; //Prepend a fake scheme for URL parsing
const url = require('url');
const parsedUrl = new url.URL(pgUri);
// Extract the pathname and remove the leading '/'
const db = parsedUrl.pathname.substring(1);
// Default the port to 5432 if it's empty
const port = parsedUrl.port || '5432';
console.log(\`\${parsedUrl.username || '-'} \${parsedUrl.password || '-'} \${parsedUrl.hostname} \${port} \${db}\`);
" "$conn_string")
IFS=' ' read -r USER PASSWORD HOST PORT DB <<<"$(node -e "
const connectionString = process.argv[1];
const pgUri = connectionString.startsWith(\"postgresql://\")
? connectionString
: 'http://' + connectionString; //Prepend a fake scheme for URL parsing
const url = require('url');
const parsedUrl = new url.URL(pgUri);
// Extract the pathname and remove the leading '/'
const db = parsedUrl.pathname.substring(1);
// Default the port to 5432 if it's empty
const port = parsedUrl.port || '5432';
console.log(\`\${parsedUrl.username || '-'} \${parsedUrl.password || '-'} \${parsedUrl.hostname} \${port} \${db}\`);
" "$conn_string")"
🧰 Tools
🪛 Shellcheck

[warning] 65-80: Quote this to prevent word splitting.

(SC2046)

Copy link
Member

Choose a reason for hiding this comment

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

@abhvsn Can you please modify the code to resolve this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor Author

@abhvsn abhvsn Sep 30, 2024

Choose a reason for hiding this comment

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

@mohanarpit I am not able to understand the advantage for this. I have written the tests to see the effect of this change but couldn't find anything, tests are written for following APPSMITH_DB_URL input for which the response was exactly same with or without quotes:

  1. postgresql://user:password@localhost:5432/dbname
  2. postgresql://user:p a s s w o r d@localhost:5432/dbname
  3. postgresql://user:password@localhost:5432/db name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/coderabbitai can you please check the above comment, if I'm missing something?


# Now, set the environment variables
export PG_DB_USER="$USER"
export PG_DB_PASSWORD="$PASSWORD"
export PG_DB_HOST="$HOST"
export PG_DB_PORT="$PORT"
export PG_DB_NAME="$DB"
}

# Example usage of the functions
# waitForPostgresAvailability
# extract_postgres_db_params "postgresql://user:password@localhost:5432/dbname"
9 changes: 9 additions & 0 deletions deploy/docker/fs/opt/appsmith/run-java.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
#!/bin/bash

# Source the helper script
source pg-utils.sh

set -o errexit
set -o pipefail
set -o nounset
Expand Down Expand Up @@ -29,6 +32,12 @@ match-proxy-url() {
[[ -n $proxy_host ]]
}

# Extract the database parameters from the APPSMITH_DB_URL and wait for the database to be available
if [[ "$mode" == "pg" ]]; then
extract_postgres_db_params "$APPSMITH_DB_URL"
waitForPostgresAvailability
fi

if match-proxy-url "${HTTP_PROXY-}"; then
extra_args+=(-Dhttp.proxyHost="$proxy_host" -Dhttp.proxyPort="$proxy_port")
if [[ -n $proxy_user ]]; then
Expand Down
Loading