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.

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

Speed up some requests #8290

Merged
merged 3 commits into from
Jan 6, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 2 additions & 1 deletion app/WebknossosModule.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import com.google.inject.AbstractModule
import com.scalableminds.webknossos.datastore.storage.DataVaultService
import controllers.InitialDataService
import controllers.{Application, InitialDataService}
import files.TempFileService
import mail.MailchimpTicker
import models.analytics.AnalyticsSessionService
Expand All @@ -17,6 +17,7 @@ import utils.sql.SqlClient

class WebknossosModule extends AbstractModule {
override def configure(): Unit = {
bind(classOf[Application]).asEagerSingleton()
bind(classOf[Startup]).asEagerSingleton()
bind(classOf[SqlClient]).asEagerSingleton()
bind(classOf[InitialDataService]).asEagerSingleton()
Expand Down
9 changes: 6 additions & 3 deletions app/controllers/Application.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import models.organization.OrganizationDAO
import models.user.UserService
import org.apache.pekko.actor.ActorSystem
import play.api.libs.json.Json
import play.api.mvc.{Action, AnyContent}
import play.api.mvc.{Action, AnyContent, Result}
import play.silhouette.api.Silhouette
import security.WkEnv
import utils.sql.{SimpleSQLDAO, SqlClient}
Expand Down Expand Up @@ -51,9 +51,12 @@ class Application @Inject()(actorSystem: ActorSystem,
}
}

// This only changes on server restart, so we can cache the full result.
private lazy val cachedFeaturesResult: Result = addNoCacheHeaderFallback(
Ok(conf.raw.underlying.getConfig("features").resolve.root.render(ConfigRenderOptions.concise())).as(jsonMimeType))

def features: Action[AnyContent] = sil.UserAwareAction {
addNoCacheHeaderFallback(
Ok(conf.raw.underlying.getConfig("features").resolve.root.render(ConfigRenderOptions.concise())).as(jsonMimeType))
cachedFeaturesResult
}

def health: Action[AnyContent] = Action {
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/AuthenticationController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,8 @@ class AuthenticationController @Inject()(
case None => Future.successful(NotFound(Messages("error.noUser")))
case Some(user) =>
for {
token <- bearerTokenAuthenticatorService.createAndInit(user.loginInfo, TokenType.ResetPassword)
token <- bearerTokenAuthenticatorService
.createAndInit(user.loginInfo, TokenType.ResetPassword, deleteOld = true)
} yield {
Mailer ! Send(defaultMails.resetPasswordMail(user.name, email.toLowerCase, token))
Ok
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/OrganizationController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ class OrganizationController @Inject()(

def organizationsIsEmpty: Action[AnyContent] = Action.async { implicit request =>
for {
allOrgs <- organizationDAO.findAll(GlobalAccessContext) ?~> "organization.list.failed"
orgaTableIsEmpty <- organizationDAO.isEmpty ?~> "organization.list.failed"
} yield {
Ok(Json.toJson(allOrgs.isEmpty))
Ok(Json.toJson(orgaTableIsEmpty))
}
}

Expand Down
6 changes: 6 additions & 0 deletions app/models/organization/Organization.scala
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ class OrganizationDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionCont
parsed <- parseAll(r)
} yield parsed

def isEmpty: Fox[Boolean] =
for {
rows <- run(q"SELECT COUNT(*) FROM $existingCollectionName".as[Int])
value <- rows.headOption
} yield value == 0

@deprecated("use findOne with string type instead", since = "")
override def findOne(id: ObjectId)(implicit ctx: DBAccessContext): Fox[Organization] =
Fox.failure("Cannot find organization by ObjectId. Use findOne with string type instead")
Expand Down
20 changes: 12 additions & 8 deletions app/security/BearerTokenAuthenticatorRepository.scala
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,20 @@ class BearerTokenAuthenticatorRepository(tokenDAO: TokenDAO)(implicit ec: Execut

def add(authenticator: BearerTokenAuthenticator,
tokenType: TokenType,
deleteOld: Boolean = true): Future[BearerTokenAuthenticator] =
deleteOld: Boolean = true): Future[BearerTokenAuthenticator] = {
if (deleteOld) {
removeByLoginInfoIfPresent(authenticator.loginInfo, tokenType)
}
for {
oldAuthenticatorOpt <- findOneByLoginInfo(authenticator.loginInfo, tokenType)
_ <- insert(authenticator, tokenType).futureBox
} yield {
if (deleteOld) {
oldAuthenticatorOpt.map(a => remove(a.id))
}
authenticator
}
} yield authenticator
}

private def removeByLoginInfoIfPresent(loginInfo: LoginInfo, tokenType: TokenType): Unit =
for {
oldOpt <- findOneByLoginInfo(loginInfo, tokenType)
_ = oldOpt.foreach(old => remove(old.id))
} yield ()
Comment on lines +68 to +72
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

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.

Suggested change
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(())
}


private def insert(authenticator: BearerTokenAuthenticator, tokenType: TokenType): Fox[Unit] =
for {
Expand Down
20 changes: 10 additions & 10 deletions app/security/CombinedAuthenticatorService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@ case class CombinedAuthenticatorService(cookieSettings: CookieAuthenticatorSetti

private val cookieSigner = new JcaSigner(JcaSignerSettings(conf.Silhouette.CookieAuthenticator.signerSecret))

val cookieAuthenticatorService = new CookieAuthenticatorService(cookieSettings,
None,
cookieSigner,
cookieHeaderEncoding,
new Base64AuthenticatorEncoder,
fingerprintGenerator,
idGenerator,
clock)
private val cookieAuthenticatorService = new CookieAuthenticatorService(cookieSettings,
None,
cookieSigner,
cookieHeaderEncoding,
new Base64AuthenticatorEncoder,
fingerprintGenerator,
idGenerator,
clock)

val tokenAuthenticatorService =
new WebknossosBearerTokenAuthenticatorService(tokenSettings, tokenDao, idGenerator, clock, userService, conf)
Expand All @@ -55,9 +55,9 @@ case class CombinedAuthenticatorService(cookieSettings: CookieAuthenticatorSetti
override def create(loginInfo: LoginInfo)(implicit request: RequestHeader): Future[CombinedAuthenticator] =
cookieAuthenticatorService.create(loginInfo).map(CombinedAuthenticator(_))

def createToken(loginInfo: LoginInfo): Future[CombinedAuthenticator] = {
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(_))
Comment on lines +58 to 61
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

Fix unused Future in createToken method

There's an issue with Future handling in this method:

  1. Line 60 creates a Future that's not used (the result of tokenAuthenticatorService.init is discarded)
  2. 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.

}

Expand Down
13 changes: 9 additions & 4 deletions app/security/WebknossosBearerTokenAuthenticatorService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class WebknossosBearerTokenAuthenticatorService(settings: BearerTokenAuthenticat
}
}

def init(authenticator: BearerTokenAuthenticator, tokenType: TokenType, deleteOld: Boolean = true): Future[String] =
def init(authenticator: BearerTokenAuthenticator, tokenType: TokenType, deleteOld: Boolean): Future[String] =
repository
.add(authenticator, tokenType, deleteOld)
.map { a =>
Expand All @@ -64,10 +64,15 @@ class WebknossosBearerTokenAuthenticatorService(settings: BearerTokenAuthenticat
case e => throw new AuthenticatorInitializationException(InitError.format(ID, authenticator), Some(e))
}

def createAndInitDataStoreTokenForUser(user: User): Fox[String] =
createAndInit(user.loginInfo, TokenType.DataStore, deleteOld = false)
def createAndInitDataStoreTokenForUser(user: User): Fox[String] = {
val before = Instant.now
for {
res <- createAndInit(user.loginInfo, TokenType.DataStore, deleteOld = false)
_ = Instant.logSince(before, "createAndInit")
} yield res
}

def createAndInit(loginInfo: LoginInfo, tokenType: TokenType, deleteOld: Boolean = true): Future[String] =
def createAndInit(loginInfo: LoginInfo, tokenType: TokenType, deleteOld: Boolean): Future[String] =
for {
tokenAuthenticator <- create(loginInfo, tokenType)
tokenId <- init(tokenAuthenticator, tokenType, deleteOld)
Expand Down