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.

Sign up for GitHub

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: create appsmith schema for postgres #36591

Merged
merged 20 commits into from
Sep 30, 2024
Merged

chore: create appsmith schema for postgres #36591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
598ef61
chore: Add postgres dependency for server to startup
abhvsn Sep 27, 2024
62fb7d2
chore: Extract the common functionality to utils file
abhvsn Sep 27, 2024
76e95c3
chore: Forgot to add the remaining changes
abhvsn Sep 27, 2024
d89a596
create appsmith schema during the pg start
AnaghHegde Sep 27, 2024
1edfc6f
add conditional evaluation for pg schema creation
AnaghHegde Sep 27, 2024
1413b1e
env variables changes for supporting the postgres changes for build
AnaghHegde Sep 27, 2024
cb4a295
add current schema to the db url config
AnaghHegde Sep 27, 2024
833832d
tests
AnaghHegde Sep 27, 2024
ca8d57a
review comments
AnaghHegde Sep 30, 2024
65ea2db
syntax error
AnaghHegde Sep 30, 2024
d3bf74c
add support for backward compatibility for existing mongo
AnaghHegde Sep 30, 2024
0254049
refactor
AnaghHegde Sep 30, 2024
bca4afc
Merge branch 'release' of github.com:appsmithorg/appsmith into chore/…
AnaghHegde Sep 30, 2024
846a63b
update schema creation flow
AnaghHegde Sep 30, 2024
43089a6
update schema creation flow
AnaghHegde Sep 30, 2024
342e4d4
create user
AnaghHegde Sep 30, 2024
80da201
use default user for schema creation locally
AnaghHegde Sep 30, 2024
14a0f35
refactor
AnaghHegde Sep 30, 2024
d38ca4d
refactor
AnaghHegde Sep 30, 2024
32f91e8
remore redundant log
AnaghHegde Sep 30, 2024
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
19 changes: 16 additions & 3 deletions ...rver/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonDBConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,23 @@ public DataSourceProperties extractJdbcProperties(String dbUrl) {
ds.setUsername(userDetails[0]);
ds.setPassword(userDetails[1]);
}
// If the port is not mentioned default it to standard 5432
// If the port is not mentioned, default it to the standard PostgreSQL port 5432
int port = uri.getPort() == -1 ? 5432 : uri.getPort();
String updatedUrl =
String.format("%s%s://%s:%s%s", JDBC_PREFIX, uri.getScheme(), uri.getHost(), port, uri.getPath());

// Check if the URL already has query parameters
String query = uri.getQuery();
String updatedUrl;
if (StringUtils.hasLength(query)) {
// Append currentSchema=appsmith if there are already parameters
updatedUrl = String.format(
"%s%s://%s:%s%s?%s&currentSchema=appsmith",
JDBC_PREFIX, uri.getScheme(), uri.getHost(), port, uri.getPath(), query);
} else {
// No parameters, just append currentSchema
updatedUrl = String.format(
"%s%s://%s:%s%s?currentSchema=appsmith",
JDBC_PREFIX, uri.getScheme(), uri.getHost(), port, uri.getPath());
}
ds.setUrl(updatedUrl);
return ds;
} catch (URISyntaxException e) {
Expand Down
33 changes: 30 additions & 3 deletions .../appsmith-server/src/test/java/com/appsmith/server/configurations/CommonDBConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ public void testExtractAndSaveJdbcParams_validDbUrlWithUsernameAndPassword() {
DataSourceProperties ds = commonDBConfig.extractJdbcProperties(dbUrl);
assertEquals("postgres", ds.getUsername());
assertEquals("password", ds.getPassword());
assertEquals("jdbc:postgresql://localhost:5432/postgres", ds.getUrl());
assertEquals("jdbc:postgresql://localhost:5432/postgres?currentSchema=appsmith", ds.getUrl());

String dbUrlWithPort = "postgresql://postgres:password@localhost:1234/postgres";
ds = commonDBConfig.extractJdbcProperties(dbUrlWithPort);
assertEquals("postgres", ds.getUsername());
assertEquals("password", ds.getPassword());
assertEquals("jdbc:postgresql://localhost:1234/postgres", ds.getUrl());
assertEquals("jdbc:postgresql://localhost:1234/postgres?currentSchema=appsmith", ds.getUrl());
}

@Test
Expand All @@ -32,7 +32,7 @@ public void testExtractAndSaveJdbcParams_validDbUrlWithoutUsernameAndPassword()
DataSourceProperties ds = commonDBConfig.extractJdbcProperties(dbUrl);
assertNull(ds.getUsername());
assertNull(ds.getPassword());
assertEquals("jdbc:postgresql://localhost:5432/postgres", ds.getUrl());
assertEquals("jdbc:postgresql://localhost:5432/postgres?currentSchema=appsmith", ds.getUrl());
}

@Test
Expand All @@ -44,4 +44,31 @@ public void testExtractAndSaveJdbcParams_invalidDbUrl() {
dbUrl);
assertThrows(IllegalArgumentException.class, () -> commonDBConfig.extractJdbcProperties(dbUrl), errorString);
}

@Test
void testExtractJdbcPropertiesWithQueryParams() {
CommonDBConfig commonDBConfig = new CommonDBConfig();
String dbUrl = "postgresql://user:password@localhost:5432/mydb?sslmode=require";

DataSourceProperties dataSourceProperties = commonDBConfig.extractJdbcProperties(dbUrl);

String expectedUrl = "jdbc:postgresql://localhost:5432/mydb?sslmode=require&currentSchema=appsmith";
assertEquals(
expectedUrl,
dataSourceProperties.getUrl(),
"URL with existing query params should append the currentSchema correctly.");
}

@Test
void testExtractJdbcPropertiesWithoutQueryParams() {
CommonDBConfig commonDBConfig = new CommonDBConfig();
String dbUrl = "postgresql://user:password@localhost:5432/mydb";
DataSourceProperties dataSourceProperties = commonDBConfig.extractJdbcProperties(dbUrl);

String expectedUrl = "jdbc:postgresql://localhost:5432/mydb?currentSchema=appsmith";
assertEquals(
expectedUrl,
dataSourceProperties.getUrl(),
"URL without query params should append the currentSchema correctly.");
}
}
3 changes: 2 additions & 1 deletion deploy/docker/fs/opt/appsmith/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ init_env_file() {
# Generate new docker.env file when initializing container for first time or in Heroku which does not have persistent volume
tlog "Generating default configuration file"
mkdir -p "$CONF_PATH"

local default_appsmith_mongodb_user="appsmith"
local generated_appsmith_mongodb_password=$(
tr -dc A-Za-z0-9 </dev/urandom | head -c 13
Expand All @@ -75,10 +76,10 @@ init_env_file() {
tr -dc A-Za-z0-9 </dev/urandom | head -c 13
echo ''
)

bash "$TEMPLATES_PATH/docker.env.sh" "$default_appsmith_mongodb_user" "$generated_appsmith_mongodb_password" "$generated_appsmith_encryption_password" "$generated_appsmith_encription_salt" "$generated_appsmith_supervisor_password" > "$ENV_PATH"
fi


tlog "Load environment configuration"

# Load the ones in `docker.env` in the stacks folder.
Expand Down
50 changes: 49 additions & 1 deletion deploy/docker/fs/opt/appsmith/pg-utils.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,54 @@ extract_postgres_db_params() {
export PG_DB_NAME="$DB"
}

init_pg_db() {
# Create the appsmith schema
echo "Initializing PostgreSQL with schema..."

# Check if APPSMITH_DB_URL is a PostgreSQL URL
if [[ -n "$APPSMITH_DB_URL" && "$APPSMITH_DB_URL" == postgres*://* ]]; then
echo "APPSMITH_DB_URL is a valid PostgreSQL URL."

# Check if the DB_HOST is local (localhost or 127.0.0.1)
if [[ "$PG_DB_HOST" == "localhost" || "$PG_DB_HOST" == "127.0.0.1" ]]; then
Comment on lines +94 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Class, let's examine the URL validation and host determination.

Good effort on validating the APPSMITH_DB_URL! However, we can improve this section:

  1. The condition "$APPSMITH_DB_URL" == postgres*://* might not catch all valid PostgreSQL URLs. Let's use a more robust regex pattern.
  2. Our local host check is incomplete. We should include all possible local addresses.

Here's a suggested improvement:

if [[ -n "$APPSMITH_DB_URL" && "$APPSMITH_DB_URL" =~ ^postgres(ql)?:// ]]; then
    echo "APPSMITH_DB_URL is a valid PostgreSQL URL."

    if [[ "$PG_DB_HOST" == "localhost" || "$PG_DB_HOST" == "127.0.0.1" || "$PG_DB_HOST" == "::1" ]]; then
        echo "Local PostgreSQL instance detected."
    else
        echo "Remote PostgreSQL instance detected."
    fi
else
    echo "Invalid or missing PostgreSQL URL."
    exit 1
fi

This change will catch more valid PostgreSQL URLs and include the IPv6 localhost address. Remember, students, attention to detail is crucial in database management!


# Check if the database exists
DB_CHECK=$(psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U postgres -d "postgres" -tAc "SELECT 1 FROM pg_database WHERE datname='$PG_DB_NAME'")

if [ "$DB_CHECK" != "1" ]; then
echo "Database $PG_DB_NAME does not exist. Creating database..."
psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U postgres -d "postgres" -c "CREATE DATABASE $PG_DB_NAME;"
else
echo "Database $PG_DB_NAME already exists."
fi

# Check if the schema exists
SCHEMA_CHECK=$(psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U postgres -d "$PG_DB_NAME" -tAc "SELECT 1 FROM information_schema.schemata WHERE schema_name='appsmith'")

# Create schema and user if not exists
if [ "$SCHEMA_CHECK" != "1" ]; then
echo "Creating user '$PG_DB_USER' with password "
psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U postgres -d "$PG_DB_NAME" -c "CREATE USER \"$PG_DB_USER\" WITH PASSWORD '$PG_DB_PASSWORD';"

echo "Schema 'appsmith' does not exist. Creating schema..."
psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U postgres -d "$PG_DB_NAME" -c "CREATE SCHEMA appsmith;"
fi
Comment on lines +101 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Now, let's turn our attention to the local database and schema creation, class.

You've done a good job structuring the database and schema creation logic. However, there are a few points we need to address:

  1. Excellent use of conditional checks before creating the database and schema!

  2. Be cautious about potential SQL injection in the database name. Let's sanitize it:

    PG_DB_NAME=$(echo "$PG_DB_NAME" | sed 's/[^a-zA-Z0-9_]//g')

  3. There's a syntax error in the user creation SQL. The username should not be in double quotes. Let's fix that:

    psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U postgres -d "$PG_DB_NAME" -c "CREATE USER $PG_DB_USER WITH PASSWORD '$PG_DB_PASSWORD';"

  4. It's generally not a good practice to log passwords, even partially. Let's remove that from the echo statement on line 116.

  5. Consider adding error handling for each psql command to ensure the script exits if any operation fails.

Remember, class, security and error handling are crucial in database management scripts!

else
echo "Remote PostgreSQL detected, running as current user."
PGPASSWORD=$PG_DB_PASSWORD psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U "$PG_DB_USER" -d "$PG_DB_NAME" -c "CREATE SCHEMA IF NOT EXISTS appsmith;"
fi
Comment on lines +122 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Now, let's examine the remote host schema creation, students.

While this section is concise, there are a few important points we need to address:

  1. Using PGPASSWORD in the command line is not the most secure method. However, for scripts, it's acceptable. Just ensure this script is only accessible to authorized users.

  2. We're missing error handling here. Let's add some:

    if ! PGPASSWORD=$PG_DB_PASSWORD psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U "$PG_DB_USER" -d "$PG_DB_NAME" -c "CREATE SCHEMA IF NOT EXISTS appsmith;"; then
    echo "Failed to create schema 'appsmith' on remote host."
    exit 1
fi

  3. Consider using a connection timeout to prevent the script from hanging indefinitely:

    PGCONNECT_TIMEOUT=10 PGPASSWORD=$PG_DB_PASSWORD psql ...

Remember, class, always think about error handling and timeouts when dealing with remote connections!


# Check if the schema creation was successful
if [ $? -eq 0 ]; then
echo "Schema 'appsmith' created or already exists."
else
echo "Failed to create schema 'appsmith'."
exit 1
fi
echo "PostgreSQL initialization completed."
fi
Comment on lines +127 to +135
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Finally, let's review the schema creation verification, class.

This section shows good attention to error handling, but we can make it even better:

  1. The success message could be more specific. Instead of just saying the schema exists, let's confirm it:

    if psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U "$PG_DB_USER" -d "$PG_DB_NAME" -tAc "SELECT 1 FROM information_schema.schemata WHERE schema_name='appsmith'" | grep -q 1; then
    echo "Successfully confirmed the existence of schema 'appsmith'."
else
    echo "Failed to confirm the existence of schema 'appsmith'. Please check the database manually."
    exit 1
fi

  2. Let's add a final check to ensure the user has the necessary permissions:

    if psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U "$PG_DB_USER" -d "$PG_DB_NAME" -tAc "SELECT has_schema_privilege('$PG_DB_USER', 'appsmith', 'usage')" | grep -q t; then
    echo "User '$PG_DB_USER' has the necessary permissions on schema 'appsmith'."
else
    echo "Warning: User '$PG_DB_USER' may not have the necessary permissions on schema 'appsmith'."
fi

Remember, students, thorough verification and clear error messages are key to maintaining a robust system!

}

# Example usage of the functions
# waitForPostgresAvailability
# extract_postgres_db_params "postgresql://user:password@localhost:5432/dbname"
# extract_postgres_db_params "postgresql://user:password@localhost:5432/dbname"
1 change: 1 addition & 0 deletions deploy/docker/fs/opt/appsmith/run-java.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ match-proxy-url() {
if [[ "$mode" == "pg" ]]; then
extract_postgres_db_params "$APPSMITH_DB_URL"
waitForPostgresAvailability
init_pg_db
fi

if match-proxy-url "${HTTP_PROXY-}"; then
Expand Down
7 changes: 4 additions & 3 deletions deploy/docker/fs/opt/appsmith/templates/docker.env.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

set -o nounset
MONGO_USER="$1"
MONGO_PASSWORD="$2"
DB_PASSWORD="$2"
ENCRYPTION_PASSWORD="$3"
ENCRYPTION_SALT="$4"
SUPERVISOR_PASSWORD="$5"
Expand Down Expand Up @@ -63,9 +63,10 @@ APPSMITH_RECAPTCHA_SITE_KEY=
APPSMITH_RECAPTCHA_SECRET_KEY=
APPSMITH_RECAPTCHA_ENABLED=

APPSMITH_DB_URL=mongodb://$MONGO_USER:$MONGO_PASSWORD@localhost:27017/appsmith
APPSMITH_DB_URL=mongodb://$MONGO_USER:$DB_PASSWORD@localhost:27017/appsmith
APPSMITH_POSTGRES_DB_URL=postgresql://appsmith:$DB_PASSWORD@localhost:5432/appsmith
APPSMITH_MONGODB_USER=$MONGO_USER
APPSMITH_MONGODB_PASSWORD=$MONGO_PASSWORD
APPSMITH_MONGODB_PASSWORD=$DB_PASSWORD
APPSMITH_API_BASE_URL=http://localhost:8080/api/v1

APPSMITH_REDIS_URL=redis://127.0.0.1:6379
Expand Down
Loading