chore: create appsmith schema for postgres #36591
Conversation
AnaghHegde
requested review from
sharat87 and
pratapaprasanna
as code owners
WalkthroughThe changes introduce new shell scripts aimed at managing PostgreSQL database initialization and connection handling within Appsmith. The Changes
Possibly related PRs
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.
| # Generate a new password for local PostgreSQL | ||
| DB_USER="appsmith" | ||
| DB_PASSWORD=$(openssl rand -base64 12) # Generate a random password | ||
| echo "Generated new password for appsmith user: $DB_PASSWORD" |
There was a problem hiding this comment.
Caution: Avoid Logging Sensitive Passwords
Logging passwords can lead to security vulnerabilities by exposing sensitive information. The line:
echo "Generated new password for appsmith user: $DB_PASSWORD"should be modified to exclude the actual password from the output.
You can adjust the line as follows to prevent the password from being logged:
-echo "Generated new password for appsmith user: $DB_PASSWORD"
+echo "Generated new password for appsmith user."|
|
||
| # Update APPSMITH_DB_URL with the new username and password | ||
| APPSMITH_DB_URL=$(echo "$APPSMITH_DB_URL" | sed "s#postgres://[^:]*:[^@]*@#postgres://$DB_USER:$DB_PASSWORD@#") | ||
| echo "APPSMITH_DB_URL updated with new user and password: $APPSMITH_DB_URL" |
There was a problem hiding this comment.
Caution: Avoid Logging Database URLs with Credentials
Printing the full APPSMITH_DB_URL, which includes the username and password, can expose sensitive information. The line:
echo "APPSMITH_DB_URL updated with new user and password: $APPSMITH_DB_URL"should be altered to prevent credential leakage.
Consider modifying the echo statement to omit the credentials:
-echo "APPSMITH_DB_URL updated with new user and password: $APPSMITH_DB_URL"
+echo "APPSMITH_DB_URL updated with new user and password."| echo "Generated new password for appsmith user: $DB_PASSWORD" | ||
|
|
||
| # Update APPSMITH_DB_URL with the new username and password | ||
| APPSMITH_DB_URL=$(echo "$APPSMITH_DB_URL" | sed "s#postgres://[^:]*:[^@]*@#postgres://$DB_USER:$DB_PASSWORD@#") |
There was a problem hiding this comment.
Ensure Robust Handling of Special Characters in Credentials
When using sed to update APPSMITH_DB_URL, special characters in $DB_USER or $DB_PASSWORD could break the command. This is because sed might misinterpret these characters, leading to incorrect substitutions.
To safely handle special characters, consider using printf with URL encoding or use alternative tools like awk or parameter expansion. Here's one way to adjust the command:
-APPSMITH_DB_URL=$(echo "$APPSMITH_DB_URL" | sed "s#postgres://[^:]*:[^@]*@#postgres://$DB_USER:$DB_PASSWORD@#")
+APPSMITH_DB_URL=$(echo "$APPSMITH_DB_URL" | sed "s#\(postgres://\)[^@]*@#\1$DB_USER:$DB_PASSWORD@#")Alternatively, you could use parameter expansion in bash to avoid calling external utilities:
APPSMITH_DB_URL="postgres://$DB_USER:$DB_PASSWORD@${DB_HOST}/${DB_NAME}"Ensure that variables are properly URL-encoded if they may contain special characters.
| echo "APPSMITH_DB_URL updated with new user and password: $APPSMITH_DB_URL" | ||
|
|
||
| # Optionally: Save the updated APPSMITH_DB_URL back to the .env file or environment | ||
| sed -i "s#APPSMITH_DB_URL=.*#APPSMITH_DB_URL=$APPSMITH_DB_URL#g" "$ENV_PATH" |
There was a problem hiding this comment.
Handle Special Characters Safely When Updating Configuration
Directly inserting APPSMITH_DB_URL into a sed command can cause issues if the URL contains characters that sed interprets, such as forward slashes or hashes. The line:
sed -i "s#APPSMITH_DB_URL=.*#APPSMITH_DB_URL=$APPSMITH_DB_URL#g" "$ENV_PATH"To safely update the configuration, you could:
-
Use a delimiter that is less likely to appear in the URL, such as a control character.
-
Escape special characters in
APPSMITH_DB_URLbefore using it insed.
Here's an example using printf to escape the URL:
escaped_url=$(printf '%s\n' "$APPSMITH_DB_URL" | sed 's/[\/&]/\\&/g')
sed -i "s#^APPSMITH_DB_URL=.*#APPSMITH_DB_URL=$escaped_url#g" "$ENV_PATH"| echo "APPSMITH_DB_URL is a valid PostgreSQL URL." | ||
|
|
||
| # Extract the host from APPSMITH_DB_URL | ||
| DB_HOST=$(echo "$APPSMITH_DB_URL" | sed -n 's#.*://[^@]*@\([^:]*\):.*#\1#p') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Using URL Parsing Tools for Reliability
Parsing URLs with sed can be error-prone. Consider using a dedicated URL parsing tool or built-in shell features. For example, using parameter expansion in bash:
# Extract the host from APPSMITH_DB_URL
DB_HOST=$(echo "$APPSMITH_DB_URL" | cut -d@ -f2 | cut -d: -f1)Or use awk for more robust parsing.
💡 Codebase verification
Issue Found: DB_HOST Extraction Fails Without Credentials
The current sed command does not extract DB_HOST when the database URL lacks credentials, resulting in an empty value. To ensure reliable extraction in all scenarios, consider updating the extraction logic to handle URLs both with and without credentials.
🔗 Analysis chain
Verify Correct Extraction of DB Host from URL
The sed command used to extract DB_HOST assumes that the URL contains credentials. If the credentials are not present, the pattern may fail. The command:
DB_HOST=$(echo "$APPSMITH_DB_URL" | sed -n 's#.*://[^@]*@\([^:]*\):.*#\1#p')Consider testing the extraction with URLs both with and without credentials to ensure reliability.
You can run the following script to verify:
This script will help you identify if the extraction works correctly in different scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test DB_HOST extraction from APPSMITH_DB_URL with and without credentials.
urls=(
"postgres://user:password@localhost:5432/dbname"
"postgres://localhost:5432/dbname"
)
for url in "${urls[@]}"; do
DB_HOST=$(echo "$url" | sed -n 's#.*://[^@]*@\([^:]*\):.*#\1#p')
echo "URL: $url"
echo "Extracted DB_HOST: $DB_HOST"
echo "----"
done
Length of output: 705
|
/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!
Description
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11072318532
Commit: 833832d
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 27 Sep 2024 14:48:53 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation