chore: Bust OAuth2 client cache for spring boot 3.3#36660
Conversation
WalkthroughThe changes introduced in this pull request involve modifications to the serialization and deserialization logic within the Changes
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: 4
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration063CacheBustSpringBoot3_3.java (1)
27-37: Add Javadoc comments to public methodsFor clarity and maintainability, it's helpful to document your public methods using Javadoc comments. This provides future developers with information about the method's purpose and how to use it.
/** * Clears Redis session keys that contain OAuth2 authorized client information. * * @param reactiveRedisOperations the ReactiveRedisOperations instance used to interact with Redis */ public static void doClearRedisOAuth2AuthClientKeys( ReactiveRedisOperations<String, String> reactiveRedisOperations) { // method implementation }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/RedisConfig.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration063CacheBustSpringBoot3_3.java (1 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/RedisConfig.java (2)
Line range hint
1-365: Excellent work, class! Let's summarize what we've learned today.We've made significant improvements to our Redis configuration, particularly in how we handle OAuth2 client data. Here's a quick recap:
- We've updated our serialization process to work with
OAuth2AuthorizedClientinstead ofOAuth2AuthorizedClientDTO.- Our deserialization process now converts data back to
OAuth2AuthorizedClient, maintaining consistency.- These changes align our code better with Spring Security standards and improve our overall data handling.
Remember, these changes might require updates in other parts of our codebase. It's crucial to ensure that all areas of our application are consistent with these new changes.
For homework, I want you all to review any code that interacts with OAuth2 client data and make sure it's compatible with these updates. Keep up the good work!
Line range hint
280-289: Now, class, let's examine the changes in our deserialization process!We've updated our
deserializemethod to convertOAuth2AuthorizedClientDTOback intoOAuth2AuthorizedClient. This is a crucial step to maintain consistency with our serialization changes.Here's what you need to remember:
- This change ensures that we're working with
OAuth2AuthorizedClientthroughout our application.- It completes the cycle of serialization and deserialization, maintaining data integrity.
- We need to be vigilant about any code that might still be expecting
OAuth2AuthorizedClientDTO.As good programmers, we always ensure our data flows smoothly through our application. This change helps us achieve that goal!
Let's double-check our work with this quick test:
#!/bin/bash # Search for any remaining uses of makeOAuth2AuthorizedClient in the codebase rg "makeOAuth2AuthorizedClient" --type java
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/RedisConfig.java
Show resolved
Hide resolved
| public static void doClearRedisOAuth2AuthClientKeys( | ||
| ReactiveRedisOperations<String, String> reactiveRedisOperations) { | ||
| final String authorizedClientsKey = | ||
| "sessionAttr:org.springframework.security.oauth2.client.web.server.WebSessionServerOAuth2AuthorizedClientRepository.AUTHORIZED_CLIENTS"; | ||
| final String script = | ||
| "for _,k in ipairs(redis.call('keys','spring:session:sessions:*')) do local fieldExists = redis.call('hexists', k, '" | ||
| + authorizedClientsKey + "'); if fieldExists == 1 then redis.call('del', k) end end"; | ||
| final Flux<Object> flushdb = reactiveRedisOperations.execute(RedisScript.of(script)); | ||
|
|
||
| flushdb.blockLast(); | ||
| } |
There was a problem hiding this comment.
Avoid blocking calls in reactive streams
Dear student, it's important to remember that using blocking calls like blockLast() in a reactive pipeline can undermine the benefits of reactive programming. Blocking operations can lead to performance bottlenecks and thread starvation.
To maintain a non-blocking, asynchronous flow, consider refactoring the code to use reactive operators and subscribing to the sequence without blocking. Here's how you might modify the code:
reactiveRedisOperations.execute(RedisScript.of(script))
.doOnError(error -> log.error("Error executing Redis script", error))
.doOnComplete(() -> log.info("Successfully cleared OAuth2 client keys from Redis"))
.subscribe();| final String script = | ||
| "for _,k in ipairs(redis.call('keys','spring:session:sessions:*')) do local fieldExists = redis.call('hexists', k, '" | ||
| + authorizedClientsKey + "'); if fieldExists == 1 then redis.call('del', k) end end"; |
There was a problem hiding this comment.
Avoid using KEYS command in Redis scripts
Remember that using the KEYS command in Redis can be problematic in production environments. It can block the Redis server if there are many keys, leading to performance issues.
Instead, consider using the SCAN command, which is non-blocking and more suitable for iterating over keys in a large keyspace. Here's an example of how you can modify the script:
final String script =
"local cursor = '0' " +
"repeat " +
" local result = redis.call('SCAN', cursor, 'MATCH', 'spring:session:sessions:*') " +
" cursor = result[1] " +
" local keys = result[2] " +
" for _,k in ipairs(keys) do " +
" local fieldExists = redis.call('hexists', k, '" + authorizedClientsKey + "') " +
" if fieldExists == 1 then " +
" redis.call('del', k) " +
" end " +
" end " +
"until cursor == '0'";| public void execute() { | ||
| doClearRedisOAuth2AuthClientKeys(reactiveRedisOperations); | ||
| } |
There was a problem hiding this comment.
Add exception handling in the execute method
It's good practice to handle exceptions to make your code more robust. In the execute() method, consider adding a try-catch block to catch any exceptions that may occur during the Redis operations. This way, you can log the error and handle it appropriately.
public void execute() {
try {
doClearRedisOAuth2AuthClientKeys(reactiveRedisOperations);
} catch (Exception e) {
log.error("An error occurred while clearing OAuth2 client keys from Redis", e);
// Optionally, handle the exception or rethrow it
}
}
Failed server tests
|
2 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes