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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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 ()

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
4 changes: 2 additions & 2 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 @@ -67,7 +67,7 @@ class WebknossosBearerTokenAuthenticatorService(settings: BearerTokenAuthenticat
def createAndInitDataStoreTokenForUser(user: User): Fox[String] =
createAndInit(user.loginInfo, TokenType.DataStore, deleteOld = false)

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