Speed up some requests #8290
Speed up some requests #8290
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces modifications across several Scala files in the application, focusing on dependency injection, authentication, and organization management. The changes primarily involve updating method signatures, adding new methods, and refining the logic in controllers and services. Key modifications include enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/models/organization/Organization.scala (1)
83-87: Excellent performance optimization!The implementation efficiently uses
COUNT(*)instead of retrieving all records, which aligns perfectly with the PR objectives for improving request performance.Consider adding an index on the table if not already present, as this query might be frequently used during application startup.
app/security/CombinedAuthenticatorService.scala (1)
Line range hint
42-61: Consider improving documentation for token managementGiven the recent changes to token management (particularly the
deleteOldparameter), it would be helpful to:
- Document the token cleanup behavior in the class-level comments
- Add scaladoc to the
createTokenmethod explaining when old tokens are deleted- Consider adding logging for token cleanup operations
Example documentation:
/** * Combines cookie and bearer token authentication strategies. * Token management: * - New tokens are created with automatic cleanup of old tokens * - Old tokens for the same login info are removed during token initialization */app/WebknossosModule.scala (1)
20-20: LGTM! Consider monitoring cache memory impact.The eager singleton binding for Application class aligns well with the caching strategy. This ensures the features response is cached at startup and readily available for subsequent requests.
Consider implementing cache size monitoring and potentially adding cache eviction policies if the cached response grows significantly in production. This would help prevent memory issues if the features configuration becomes large.
app/controllers/Application.scala (1)
59-59: Consider adding cache hit/miss metrics.To validate the caching effectiveness in production, consider adding metrics to track cache usage.
def features: Action[AnyContent] = sil.UserAwareAction { + metrics.counter("features.cache.hit").increment() cachedFeaturesResult }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/WebknossosModule.scala(2 hunks)app/controllers/Application.scala(2 hunks)app/controllers/AuthenticationController.scala(1 hunks)app/controllers/OrganizationController.scala(1 hunks)app/models/organization/Organization.scala(1 hunks)app/security/BearerTokenAuthenticatorRepository.scala(1 hunks)app/security/CombinedAuthenticatorService.scala(2 hunks)app/security/WebknossosBearerTokenAuthenticatorService.scala(2 hunks)
🔇 Additional comments (8)
app/controllers/OrganizationController.scala (1)
44-46: Well-implemented optimization!
The controller now efficiently uses the optimized isEmpty method from the DAO, maintaining the same API contract while improving performance.
Let's verify the API contract hasn't changed:
✅ Verification successful
Let me verify the API contract by checking the implementation details of the frontend consumer.
API contract verification successful
The frontend consumer correctly expects and handles a JSON boolean response, negating it to determine organization existence. The optimized isEmpty implementation in the controller maintains this contract by returning the boolean wrapped in JSON, ensuring compatibility with the existing frontend implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any direct consumers of this endpoint that might be affected
rg -A 2 "organizationsIsEmpty" --type ts --type js
Length of output: 256
Script:
#!/bin/bash
# Check the implementation context of the frontend consumer
rg -B 2 -A 5 "organizationsIsEmpty" frontend/javascripts/admin/admin_rest_api.ts
Length of output: 401
app/security/CombinedAuthenticatorService.scala (1)
42-49: LGTM: Good encapsulation improvement
Making cookieAuthenticatorService private is a good practice as it's an implementation detail that should only be accessed through the public methods of CombinedAuthenticatorService.
app/security/BearerTokenAuthenticatorRepository.scala (1)
59-66: Verify backward compatibility with deleteOld parameter.
The default value of true for deleteOld parameter might affect existing callers. Ensure all call sites are updated appropriately.
✅ Verification successful
Let me search for more context about the usage of this method.
Let me search for the method's history to understand if this is a new parameter or an existing one.
Let me try a broader search to understand the method's usage and inheritance.
Based on the search results, I can generate the final response:
The deleteOld parameter appears to be safe for backward compatibility
The codebase search shows only one direct caller of the add method in BearerTokenAuthenticatorRepository, which is an internal call using TokenType.Authentication without explicitly setting the deleteOld parameter. Since the parameter has a default value of true, this maintains the existing behavior for the single identified caller.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to add method to verify parameter usage
rg -A 2 "add\(.*TokenType"
Length of output: 344
Script:
#!/bin/bash
# Search for the complete class definition to understand the method signature
ast-grep --pattern 'class BearerTokenAuthenticatorRepository {
$$$
}'
# Search for any other potential callers of this method
rg -A 3 "add.*authenticator.*TokenType|add.*TokenType.*authenticator"
Length of output: 575
Script:
#!/bin/bash
# Search for the method definition to understand its visibility and inheritance
rg -B 3 "def add.*deleteOld.*BearerTokenAuthenticator"
# Search for any potential parent traits or interfaces
rg "trait.*BearerTokenAuthenticator.*Repository"
Length of output: 107
Script:
#!/bin/bash
# Search for the add method definition with more context
rg -B 5 -A 5 "def add.*authenticator.*tokenType"
# Look for any abstract or trait definitions related to this repository
rg "trait.*Repository.*extends|abstract.*Repository.*extends"
Length of output: 114
app/controllers/Application.scala (1)
54-57: LGTM! Consider monitoring response size.
The caching implementation is appropriate for static configuration that only changes on server restart.
Let's verify the features configuration structure:
✅ Verification successful
LGTM! Features configuration structure verified.
The caching implementation is appropriate, and the features configuration structure is well-defined in conf/application.conf with clear settings:
- discussionBoard
- discussionBoardRequiresAdmin
- hideNavbarLogin
- isWkorgInstance
- recommendWkorgInstance
The cached response will be compact as it uses ConfigRenderOptions.concise() for rendering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the features configuration structure
rg -l "features\s*{" --type conf
Length of output: 122
Script:
#!/bin/bash
# Let's try a different approach to find configuration files and check their content
fd -e conf -e config -e json | xargs cat 2>/dev/null | grep -A5 "features"
Length of output: 894
Script:
#!/bin/bash
# Also search for references to features configuration in the code
rg "getConfig\(\"features\""
Length of output: 181
Script:
#!/bin/bash
# Look for configuration files directly
fd -e conf -e config -e json
Length of output: 397
app/security/WebknossosBearerTokenAuthenticatorService.scala (3)
67-73: LGTM! Good addition of performance logging.
The method properly implements token creation with performance monitoring, which aligns with the PR's performance focus.
Line range hint 75-79: LGTM! Method signature change is consistent.
The method properly implements the token creation and initialization flow with explicit deleteOld parameter.
Line range hint 57-65: LGTM! Verify all callers pass explicit deleteOld parameter.
The removal of the default parameter value improves code clarity by making token deletion behavior explicit.
✅ Verification successful
Let me search for any potential callers that might not be using the three-parameter version to ensure completeness.
Let me check the CombinedAuthenticatorService implementation to understand how it's using the init method.
All callers correctly pass the deleteOld parameter
The verification confirms that all usages of the init method in WebknossosBearerTokenAuthenticatorService properly include the deleteOld parameter. There are two types of callers:
-
Direct call in the same file with explicit parameters:
- Line 78:
init(tokenAuthenticator, tokenType, deleteOld)
- Line 78:
-
Indirect call through
CombinedAuthenticatorService:- Line 52:
tokenAuthenticatorService.init(_, TokenType.Authentication, deleteOld = true)
- Line 52:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all callers of init method pass the deleteOld parameter
ast-grep --pattern 'init($_, $_, $_)'
Length of output: 167
Script:
#!/bin/bash
# Search for any calls to init method with different parameter counts
rg "\.init\(" -A 1
Length of output: 2959
Script:
#!/bin/bash
# Get the implementation of CombinedAuthenticatorService
cat app/security/CombinedAuthenticatorService.scala
Length of output: 6658
app/controllers/AuthenticationController.scala (1)
280-281: LGTM! Proper handling of password reset tokens.
Setting deleteOld=true is the correct security practice for password reset tokens, ensuring that old reset tokens are invalidated when a new one is generated.
| private def createToken(loginInfo: LoginInfo): Future[CombinedAuthenticator] = { | ||
| val tokenAuthenticator = tokenAuthenticatorService.create(loginInfo, TokenType.Authentication) | ||
| tokenAuthenticator.map(tokenAuthenticatorService.init(_, TokenType.Authentication)) | ||
| tokenAuthenticator.map(tokenAuthenticatorService.init(_, TokenType.Authentication, deleteOld = true)) | ||
| tokenAuthenticator.map(CombinedAuthenticator(_)) |
There was a problem hiding this comment.
Fix unused Future in createToken method
There's an issue with Future handling in this method:
- Line 60 creates a Future that's not used (the result of
tokenAuthenticatorService.initis discarded) - Line 61 creates a new Future without waiting for the initialization
Consider this fix to properly chain the operations:
private def createToken(loginInfo: LoginInfo): Future[CombinedAuthenticator] = {
val tokenAuthenticator = tokenAuthenticatorService.create(loginInfo, TokenType.Authentication)
- tokenAuthenticator.map(tokenAuthenticatorService.init(_, TokenType.Authentication, deleteOld = true))
- tokenAuthenticator.map(CombinedAuthenticator(_))
+ tokenAuthenticator.flatMap { auth =>
+ tokenAuthenticatorService.init(auth, TokenType.Authentication, deleteOld = true)
+ Future.successful(CombinedAuthenticator(auth))
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| private def removeByLoginInfoIfPresent(loginInfo: LoginInfo, tokenType: TokenType): Unit = | ||
| for { | ||
| oldOpt <- findOneByLoginInfo(loginInfo, tokenType) | ||
| _ = oldOpt.foreach(old => remove(old.id)) | ||
| } yield () |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Future handling in removeByLoginInfoIfPresent.
The nested Future operations could potentially block. Consider flattening the Future chain:
- private def removeByLoginInfoIfPresent(loginInfo: LoginInfo, tokenType: TokenType): Unit =
- for {
- oldOpt <- findOneByLoginInfo(loginInfo, tokenType)
- _ = oldOpt.foreach(old => remove(old.id))
- } yield ()
+ private def removeByLoginInfoIfPresent(loginInfo: LoginInfo, tokenType: TokenType): Future[Unit] =
+ findOneByLoginInfo(loginInfo, tokenType).flatMap {
+ case Some(old) => remove(old.id)
+ case None => Future.successful(())
+ }📝 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.
| private def removeByLoginInfoIfPresent(loginInfo: LoginInfo, tokenType: TokenType): Unit = | |
| for { | |
| oldOpt <- findOneByLoginInfo(loginInfo, tokenType) | |
| _ = oldOpt.foreach(old => remove(old.id)) | |
| } yield () | |
| private def removeByLoginInfoIfPresent(loginInfo: LoginInfo, tokenType: TokenType): Future[Unit] = | |
| findOneByLoginInfo(loginInfo, tokenType).flatMap { | |
| case Some(old) => remove(old.id) | |
| case None => Future.successful(()) | |
| } |
Speeding up some of the requests that are needed before actual data can be loaded.
The speedups on my local machine are pretty small, but I think some may yield more in production if the respective tables are bigger.
The biggest waits, for me at least, are still olvy (700ms!) and some frontend code (shader compilation?)
URL of deployed dev instance (used for testing):
Steps to test:
Issues: