chore: create appsmith schema for postgres #36591
Conversation
AnaghHegde
requested review from
sharat87 and
pratapaprasanna
as code owners
WalkthroughThe changes introduce new shell scripts focused on PostgreSQL database initialization and connection management within Appsmith. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
deploy/docker/fs/opt/appsmith/pg-default-schema.sh (1)
42-50: Consider adding retry logic for schema creationIf the schema creation fails due to transient issues, it would be helpful to implement retry logic. Retrying a few times before exiting can make the script more robust against temporary network issues or database locks.
deploy/docker/fs/opt/appsmith/pg-utils.sh (1)
22-22: Enhance clarity in log messageIn line 22, the log message uses "e.g." twice, which can be redundant and may affect readability. Let's refine the message for better clarity.
Suggested change:
- 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." + tlog "PostgreSQL host '$PG_DB_HOST' is rejecting connections, possibly due to being in recovery mode or connections being maxed out."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- deploy/docker/fs/opt/appsmith/pg-default-schema.sh (1 hunks)
- deploy/docker/fs/opt/appsmith/pg-utils.sh (1 hunks)
- deploy/docker/fs/opt/appsmith/run-java.sh (3 hunks)
🧰 Additional context used
🪛 Shellcheck
deploy/docker/fs/opt/appsmith/pg-utils.sh
[warning] 65-80: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (3)
deploy/docker/fs/opt/appsmith/run-java.sh (1)
3-4: Excellent inclusion of the PostgreSQL utilities scriptBy sourcing
pg-utils.sh, you're ensuring that all the necessary PostgreSQL functions are available for use in this script. This promotes modularity and reusability, which are important principles in scripting.deploy/docker/fs/opt/appsmith/pg-utils.sh (2)
3-48: Well-structured function for PostgreSQL availability checkThe
waitForPostgresAvailabilityfunction is thoughtfully designed, covering various PostgreSQL status codes and incorporating a robust retry mechanism. Excellent work in ensuring the application waits appropriately for the database to become ready before proceeding.
61-88: Consider potential issues with spaces or special characters in connection parametersIf any of the connection parameters (username, password, hostname, database name) contain spaces or special characters, the current parsing approach may fail. It's advisable to handle such cases to make your script more robust.
You can test this by using a connection string with spaces in the credentials:
🧰 Tools
🪛 Shellcheck
[warning] 65-80: Quote this to prevent word splitting.
(SC2046)
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11070738411. |
|
Deploy-Preview-URL: https://ce-36591.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
deploy/docker/fs/opt/appsmith/templates/docker.env.sh (3)
66-66: Good job updating the database connection string!Students, notice how we've correctly updated our MongoDB connection string to use the new
DB_PASSWORDvariable. This ensures our application can still connect to the database after our variable renaming exercise.However, I have a little homework for you: Consider using environment variables for the hostname and port as well. This would make our configuration even more flexible for different deployment scenarios. What do you think about that?
Here's an example of how you could further improve this line:
-APPSMITH_DB_URL=mongodb://$MONGO_USER:$DB_PASSWORD@localhost:27017/appsmith +APPSMITH_DB_URL=mongodb://$MONGO_USER:$DB_PASSWORD@$DB_HOST:$DB_PORT/appsmithDon't forget to define
DB_HOSTandDB_PORTvariables at the beginning of the script!
67-67: Excellent addition of the PostgreSQL connection string!Class, this is a perfect example of forward-thinking in our code. By adding this commented-out PostgreSQL connection string, we're preparing our application to work with different database systems. It's like having a backup plan – always a smart move!
However, I have a small suggestion to make this even better. For consistency with our MongoDB string, let's use the
$MONGO_USERvariable here too, instead of hardcoding 'appsmith'. This way, we maintain flexibility in our user configuration across different database systems.Here's a small improvement we could make:
-#APPSMITH_DB_URL=postgresql://appsmith:$DB_PASSWORD@localhost:5432/postgres +#APPSMITH_DB_URL=postgresql://$MONGO_USER:$DB_PASSWORD@localhost:5432/postgresRemember, consistency is key in coding, just like in your homework assignments!
69-69: Well done on updating the MongoDB password variable!Class, observe how we've correctly updated the
APPSMITH_MONGODB_PASSWORDto use our newDB_PASSWORDvariable. This ensures that our MongoDB configuration remains consistent with our earlier changes.However, I have a thought-provoking question for you: Given that we're now supporting multiple database types, should we consider renaming this variable to something more generic? This could help avoid confusion if we switch to a different database system in the future.
Here's a suggestion to ponder:
-APPSMITH_MONGODB_PASSWORD=$DB_PASSWORD +APPSMITH_DB_PASSWORD=$DB_PASSWORDWhat do you think about this change? How might it impact our application's flexibility and maintainability? Let's discuss!
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)
95-95: Ensure Consistent Definition of Database UserThe
DB_USERvariable is set to"appsmith":DB_USER="appsmith"Confirm that this username aligns with the expected database user elsewhere in your configuration to maintain consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- deploy/docker/fs/opt/appsmith/entrypoint.sh (2 hunks)
- deploy/docker/fs/opt/appsmith/templates/docker.env.sh (2 hunks)
🔇 Additional comments (3)
deploy/docker/fs/opt/appsmith/templates/docker.env.sh (2)
5-5: Excellent work on generalizing the database password variable!Class, let's take a moment to appreciate this change. By renaming
MONGO_PASSWORDtoDB_PASSWORD, we're making our code more flexible. This allows us to potentially use different types of databases in the future, not just MongoDB. It's like having a universal key that can open many doors!
Line range hint
1-95: Excellent work on adapting the environment configuration for multiple databases!Class, let's recap what we've learned from this code review:
- We've successfully generalized our database configuration, making it easier to switch between MongoDB and PostgreSQL.
- The changes are consistent and well-thought-out, showing good attention to detail.
- We've identified a few areas where we can further improve flexibility and consistency.
Remember, in the world of software development, adaptability is key. By making these changes, we're preparing our application for future growth and different deployment scenarios.
For your next assignment, I want you to think about other parts of the application that might need similar updates to support multiple database types. How can we ensure our entire system is as flexible as this configuration file?
Keep up the great work!
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)
96-96: Use Secure Password GenerationWhile
openssl randis effective, specifying the length in bytes might not result in the desired number of characters. The command:DB_PASSWORD=$(openssl rand -base64 12) # Generate a random passwordEnsure that the generated password meets your security requirements regarding length and complexity. You might want to increase the length parameter to generate a longer password.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11071132873. |
|
Deploy-Preview-URL: https://ce-36591.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonDBConfig.java (1)
71-84: Excellent work on improving the URL construction, students!Your new logic for handling query parameters is a significant improvement. Let's break it down for the class:
- You check if the URL already has query parameters.
- If parameters exist, you append the
currentSchema=appsmithto the existing query string.- If no parameters exist, you add
currentSchema=appsmithas the first parameter.This approach ensures that the
currentSchemais always set without overwriting existing parameters. Well done!However, to make your code even more readable for your classmates, consider extracting the URL format strings into constants. Here's a little homework for you:
private static final String URL_FORMAT_WITH_QUERY = "%s%s://%s:%s%s?%s¤tSchema=appsmith"; private static final String URL_FORMAT_WITHOUT_QUERY = "%s%s://%s:%s%s?currentSchema=appsmith"; // Then use these constants in your String.format callsThis small change will make your code easier to maintain and understand. Remember, clear code is happy code!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonDBConfig.java (1 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonDBConfig.java (1)
68-69: Good job clarifying the default port comment, class!You've done well in updating the comment to specify that 5432 is the standard PostgreSQL port. This kind of clarity is important for your fellow students who might be new to database configurations.
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CommonDBConfigTest.java (1)
35-35: Good job on covering different scenarios, class!I'm impressed that you've consistently applied the "?currentSchema=appsmith" addition across different test cases, including this one without username and password. This demonstrates attention to detail and thorough testing practices.
However, to make your test suite even more comprehensive, consider adding one more test case:
@Test public void testExtractAndSaveJdbcParams_validDbUrlWithUsernamePasswordAndExistingQueryParams() { CommonDBConfig commonDBConfig = new CommonDBConfig(); String dbUrl = "postgresql://postgres:password@localhost:5432/postgres?sslmode=require"; DataSourceProperties ds = commonDBConfig.extractJdbcProperties(dbUrl); assertEquals("postgres", ds.getUsername()); assertEquals("password", ds.getPassword()); assertEquals("jdbc:postgresql://localhost:5432/postgres?sslmode=require¤tSchema=appsmith", ds.getUrl()); }This additional test will ensure that the
currentSchemaparameter is correctly appended when other query parameters already exist in the URL. Remember, in software development, it's always better to be thorough!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CommonDBConfigTest.java (2 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CommonDBConfigTest.java (2)
19-19: Well done, class! Let's examine this change.The addition of "?currentSchema=appsmith" to the expected JDBC URL is a smart move. It ensures that our Appsmith application will use a specific schema when connecting to PostgreSQL. This change will help maintain consistency across different environments and prevent potential conflicts with other schemas.
25-25: Excellent work on consistency, students!I'm pleased to see that you've applied the same "?currentSchema=appsmith" addition to this test case as well. It's crucial to maintain consistency across different scenarios. By testing with a non-standard port (1234), you're demonstrating good test coverage. Remember, thorough testing is the foundation of robust software!
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonDBConfig.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-server/src/test/java/com/appsmith/server/configurations/CommonDBConfigTest.java
Show resolved
Hide resolved
| # 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 | ||
| echo "Local PostgreSQL detected, switching to postgres user." | ||
| PGPASSWORD=$PG_DB_PASSWORD su - postgres -c "psql -h $PG_DB_HOST -p $PG_DB_PORT -U $PG_DB_USER -d $PG_DB_NAME -c 'CREATE SCHEMA IF NOT EXISTS appsmith;'" |
There was a problem hiding this comment.
We are su to postgres and then again passing the user via -U, have we tested this change?
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11105610023. |
|
Deploy-Preview-URL: https://ce-36591.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11109372703. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/run-java.sh (1)
Attention to Detail Needed on
pg-default-schema.shInvocation 🛠️Excellent progress on enhancing error handling! However, we've identified that
pg-default-schema.shis not being invoked anywhere in the current scripts. This omission could potentially reintroduce the race conditions we aimed to resolve withinit_pg_db.Action Items:
- Confirm Invocation: Verify if
pg-default-schema.shis invoked elsewhere in the codebase or if its responsibilities have been relocated.- Reintegrate if Necessary: If it's required, ensure that
pg-default-schema.shis properly called withinrun-java.shor the appropriate initialization scripts to prevent race conditions.Let's make sure our setup is as reliable as our intentions! 👍
🔗 Analysis chain
Line range hint
28-39: Let's revisit some previous lessons, class!We had some excellent feedback in our last review that we should address:
Error Handling: We need to ensure our PostgreSQL functions (
extract_postgres_db_params,waitForPostgresAvailability, andinit_pg_db) handle errors gracefully. Remember, proper error handling is like wearing a safety helmet – it protects us from unexpected bumps!Race Conditions: The addition of
init_pg_dbis a step in the right direction to address potential race conditions. However, we should verify that this indeed solves the issue withpg-default-schema.sh.Let's check the implementation of these functions to ensure they include proper error handling:
This will help us ensure our functions are handling errors properly and provide meaningful error messages.
To verify the resolution of the race condition issue:
This will show us where
pg-default-schema.shis being executed in relation to our PostgreSQL initialization steps.
Great job addressing the initial concerns! 🏆 Let's dive a bit deeper to ensure everything is shipshape:
Error Handling:
Let's make sure that all PostgreSQL-related functions (extract_postgres_db_params,waitForPostgresAvailability, andinit_pg_db) are equipped with comprehensive error handling. This ensures our script doesn't stumble unexpectedly.Race Conditions:
Sincepg-default-schema.shisn't being called inrun-java.sh, we need to confirm if it's handled elsewhere or if additional steps are needed to prevent race conditions.To thoroughly verify these aspects, let's run a couple more checks:
[/run_scripts]
Let's gather these results to ensure our script is as robust as it can be!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling in PostgreSQL-related functions grep -n -A 10 "extract_postgres_db_params()" deploy/docker/fs/opt/appsmith/pg-utils.sh echo "---" grep -n -A 10 "waitForPostgresAvailability()" deploy/docker/fs/opt/appsmith/pg-utils.sh echo "---" grep -n -A 10 "init_pg_db()" deploy/docker/fs/opt/appsmith/pg-utils.shLength of output: 1636
Script:
#!/bin/bash # Description: Check the execution order of pg-default-schema.sh grep -n "pg-default-schema.sh" deploy/docker/fs/opt/appsmith/run-java.shLength of output: 72
Script:
#!/bin/bash # Description: Check error handling in extract_postgres_db_params and waitForPostgresAvailability functions grep -n -A 10 "extract_postgres_db_params()" deploy/docker/fs/opt/appsmith/pg-utils.sh echo "---" grep -n -A 10 "waitForPostgresAvailability()" deploy/docker/fs/opt/appsmith/pg-utils.shLength of output: 1029
Script:
#!/bin/bash # Description: Search for any invocation of pg-default-schema.sh in the codebase rg "pg-default-schema.sh"Length of output: 25
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- deploy/docker/fs/opt/appsmith/pg-utils.sh (1 hunks)
- deploy/docker/fs/opt/appsmith/run-java.sh (1 hunks)
🔇 Additional comments (4)
deploy/docker/fs/opt/appsmith/run-java.sh (3)
Line range hint
18-70: Excellent work on improving our proxy configuration, class!I'm impressed with the enhancements made to our proxy handling. Let's review the key improvements:
The
match-proxy-urlfunction: This is a great example of modular programming. It extracts proxy details from environment variables, making our code more readable and maintainable.Separate handling for HTTP and HTTPS proxies: This allows for more flexible configurations.
Setting
NO_PROXYto an empty string if not defined: This is a thoughtful addition that prevents potential issues down the line.Adding proxy settings to
extra_args: This ensures that our Java application will use the configured proxy settings.Remember, class, clear and modular code like this makes our lives as developers much easier in the long run!
Line range hint
1-93: Class, let's summarize our lesson for today!We've made some excellent progress with our
run-java.shscript. Here's what we've learned:
Database Initialization: We've added the
init_pg_dbfunction, which is a great step towards ensuring our PostgreSQL database is properly set up before our application starts.Proxy Configuration: Our proxy handling has improved significantly. We now have a more robust and flexible system for configuring HTTP and HTTPS proxies.
Error Handling: While we've made improvements, we still need to ensure all our PostgreSQL-related functions have proper error handling. Remember, good error handling is like a good explanation – it helps us understand what went wrong and how to fix it!
Race Conditions: We've taken steps to address potential race conditions, but we should verify that our changes have fully resolved the issue.
For homework, I'd like you all to review the error handling in our PostgreSQL functions and think about how we can make it even better. Remember, in programming as in life, it's not just about getting things right – it's about being prepared for when things go wrong!
39-39: Class, let's examine the new addition to our script!I see we've introduced a new function call
init_pg_dbhere. This is an excellent step towards proper database initialization. However, we must ensure that this function is well-defined and handles potential errors gracefully.Remember, class, in programming as in life, it's important to be prepared for all scenarios. Let's make sure this function:
- Initializes the PostgreSQL database correctly
- Handles any potential errors that may occur during initialization
- Provides clear feedback or logging for troubleshooting purposes
To ensure our
init_pg_dbfunction is properly implemented, let's take a look at its definition:#!/bin/bash # Description: Check the implementation of init_pg_db function grep -n -A 10 "init_pg_db()" deploy/docker/fs/opt/appsmith/pg-utils.shThis will help us understand how the function is implemented and if it addresses our concerns about error handling and logging.
deploy/docker/fs/opt/appsmith/pg-utils.sh (1)
90-139: Class, let's summarize our review of the newinit_pg_dbfunction.Overall, you've done a commendable job in creating this function to initialize the PostgreSQL database and schema for Appsmith. The structure is logical, and it handles both local and remote database scenarios.
However, as we've discussed, there are several areas for improvement:
- Enhance URL validation and host type determination.
- Implement better security practices, especially in handling user credentials.
- Improve error handling throughout the function.
- Add more robust verification steps.
These changes will make the function more secure, reliable, and maintainable. Remember, in database management, we must always prioritize data integrity and security.
Keep up the good work, and don't forget to apply these lessons in your future coding assignments!
| # 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 "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;" | ||
|
|
||
| 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';" | ||
| else | ||
| echo "Schema 'appsmith' already exists." | ||
| fi |
There was a problem hiding this comment.
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:
-
Excellent use of conditional checks before creating the database and schema!
-
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') -
There's a syntax error in the user creation SQL. The username should not be in single 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';"
-
It's generally not a good practice to log passwords, even partially. Let's remove that from the echo statement on line 119.
-
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!
|
Deploy-Preview-URL: https://ce-36591.dp.appsmith.com |
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- deploy/docker/fs/opt/appsmith/entrypoint.sh (2 hunks)
- deploy/docker/fs/opt/appsmith/pg-utils.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/docker/fs/opt/appsmith/entrypoint.sh
| # 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;" | ||
| else | ||
| echo "Schema 'appsmith' already exists." | ||
| fi |
There was a problem hiding this comment.
Let's examine the schema and user creation, class.
You've made a good start here, but there are several areas we need to improve:
-
In the user creation SQL, the username should not be in 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';"
-
We need to add error handling for both schema and user creation:
if ! psql -h "$PG_DB_HOST" -p "$PG_DB_PORT" -U postgres -d "$PG_DB_NAME" -c "CREATE SCHEMA appsmith;"; then echo "Failed to create schema 'appsmith'. Please check your permissions and try again." exit 1 fi if ! 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';"; then echo "Failed to create user $PG_DB_USER. Please check your permissions and try again." exit 1 fi
-
It's not a good practice to log passwords, even partially. Let's remove that from the echo statement:
echo "Creating user '$PG_DB_USER'"
Remember, class, security and error handling are paramount in database management!
| # 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 |
There was a problem hiding this comment.
🛠️ 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:
- The condition
"$APPSMITH_DB_URL" == postgres*://*might not catch all valid PostgreSQL URLs. Let's use a more robust regex pattern. - 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
fiThis 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 |
There was a problem hiding this comment.
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:
-
Excellent use of conditional checks before creating the database and schema!
-
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') -
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';"
-
It's generally not a good practice to log passwords, even partially. Let's remove that from the echo statement on line 116.
-
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 |
There was a problem hiding this comment.
🛠️ 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:
-
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.
-
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
-
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 |
There was a problem hiding this comment.
🛠️ 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:
-
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
-
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!
## Description The current state is default schema or public schema. This schema is accessible by default when user connects to the pg database. Hence create `appsmith` schema for Appsmith server to use. This is to avoid anyone accidentally modifying the appsmith data. ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/11111681323> > Commit: 32f91e8 > Workflow: `PR Automation test suite` > Tags: `@tag.Sanity` > Spec: `` > <hr>Mon, 30 Sep 2024 18:08:23 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced a script to initialize the PostgreSQL database schema for Appsmith. - Added utilities for managing PostgreSQL database connections, including availability checks and parameter extraction. - Enhanced scripts for managing PostgreSQL connections and initialization. - Improved environment configuration for PostgreSQL database connections, including automatic password generation for local setups. - Updated JDBC URL handling to include schema parameters for PostgreSQL connections. - Added support for proxy configuration in the application setup. - **Bug Fixes** - Improved error handling and connection retry mechanisms for PostgreSQL setup. - **Documentation** - Updated comments and logging for better clarity on database operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Abhijeet <[email protected]>
Description
The current state is default schema or public schema. This schema is accessible by default when user connects to the pg database. Hence create
appsmithschema for Appsmith server to use. This is to avoid anyone accidentally modifying the appsmith data.Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11111681323
Commit: 32f91e8
Cypress dashboard.
Tags: @tag.Sanity
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Tue, 01 Oct 2024 02:47:18 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation