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

Remove awaiting Future and more Improvements #501

Merged
merged 50 commits into from Mar 24, 2018

Conversation

@Faithcaio
Copy link
Contributor

commented Mar 13, 2018

phew thats a big one..

This PR aims to

  • never await
  • never hide DB access (in templates / with assign operator / implicit conversions / json writes)
  • do fewer database queries - if the database could figure out something in one query it should do that
  • reduce None.get | Try.get etc. without checking
  • reduce crazy unreadable nesting

In more detail:

  • OreAction/OreRequest - always includes HeaderData (if not authed default with no permissions)
  • HeaderData contains User specific data (global permissions and infos for display in header)
  • JoinableData: OrganizationData and ProjectData (data for one orga/project
  • ScopedOrganizationData (Orga Permissions for current user)
  • ScopedProjectData (Project Permissions for current user and more userspecific data)
  • UserData (User/OrgaUser permissions for currentuser and more data)
  • VersionData (ProjectData + more data)
  • replaced all xxx_= methods with setXxx (But they still do DB updates without returning Future)
  • removed all implicit conversions that query DB
  • removed (almost) all await
  • Wrote manual queries for:
    • Project its recommended Version and its Channel together (e.g. Home Page)
    • ReviewQueue (Version Author Project Channel) and Review and its user
    • Users for Orga Notifications (Orga Member Role User => User RoleType)
    • Bulk Insert Notifications (not everywhere)
    • MissingFiles (Project Version => Owner Name Version)
    • Project Pages (with parent)
    • QueryRole for Trust (ProjectMember ProjectRole)
    • Permission Logic (trustIn) (Project Members Orga OrgaMember Role => RoleType)
    • JsonWrites that made DB queries
      • Projects (Project Version Channel Tags)
      • Version (Verson Project Channel author Tags)
      • User (User Projects Stars)
  • other queries were only reordered and/or extracted to be in one place instead of spread out in templates etc.

TODO: Review and Cleanup

  • implicit Requests are not everywhere working because e.g. ProjectRequest is not OreRequest
  • missing members in OreRestfulApi

I left out caching for a Future PR.
Also the queries can still be optimized in a lot of places. (looking at Future.sequence usages and other sequential selects)


nice to have maybe in a future PR?

  • caching of Data case classes
  • and also invalidate when appropriate (maybe we need separate caches?)
  • actual case class models without variables - return futures on insert/update - single update for multiple columns
  • move queries out of controllers
  • bulk select everything when possible
    ...?

@phase phase self-requested a review Mar 13, 2018

@progwml6

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

@Faithcaio this PR has conflicts

@phase

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

I'd appreciate better commit messages since we're not squashing. I can fix the conflicts tonight if you're busy. There are definitely a lot of great improvements in here that will greatly improve the code quality and performance. 🎉 🎉

@Faithcaio Faithcaio force-pushed the unawait branch from 84db3c3 to 7b26f1a Mar 14, 2018

@phase phase assigned phase and unassigned phase Mar 14, 2018

Merge branch 'master' into unawait
This includes the Play 2.6 updates.

Signed-off-by: Jadon Fowler <jadonflower@gmail.com>
@phase

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

I fixed the conflicts. 👍

@Katrix

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

Maybe use Future.successful instead of Future.apply for pure values?

@Faithcaio

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

I know. Was a bit rusty with my scala in the beginning.
But its just a simple search and replace

fixing more awaits
introduces HeaderData/ProjectData/VersionData case classes for use in templates and caching
}
}
fullList map { list =>
val lists = list.partition(_._3 == 0)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

Deconstruct tuple here so both lists can be named?

case Some(id) =>
val reviews = this.service.access[Review](classOf[Review])
.filter(_.userId === id)
.map(_.take(20).map(review => { review ->

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

Curly braces for map

val userTable = TableQuery[UserTable]

// Query Orga Members
val q1: Query[(Rep[Int], Rep[Option[RoleType]]), (Int, Option[RoleType]), scala.Seq] = for {

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

Any reason for scala.Seq here?

This comment has been minimized.

Copy link
@pschichtel

pschichtel Mar 15, 2018

Contributor

probably copied the infered type from intellij or similar. I'm wondering why the explicit type at all. Did it cause problems with the union down below?

This comment has been minimized.

Copy link
@Faithcaio

Faithcaio Mar 16, 2018

Author Contributor

yes IntelliJ was confused with the types so I helped it

}

// Query version author
val q2: Query[(Rep[Int], Rep[Option[RoleType]]), (Int, Option[RoleType]), scala.Seq] = for {

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

Any reason for scala.Seq here?

this.service.insert(review)
Redirect(routes.Reviews.showReviews(author, slug, versionString))
val closeOldReview = version.mostRecentUnfinishedReview.flatMap {
case Some(oldreview) =>

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

Missing handling of None here.

}

Try(this.forums.await(this.forums.createProjectTopic(newProject)))
// Create the project and it's settings
for {

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

Why use two for comprehensions here?

}

val channelCleanup = for {
_ <- rcUpdate
proj <- rcUpdate

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

Why use two for comprehensions here?

"tags" -> tags.map(toJson(_)),
"downloads" -> v.downloadCount
)
author.map(a => json.+(("author", JsString(a)))).getOrElse(json)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

Any reason for not using infix + here? Also, fold is probably clearer here.

@@ -10,11 +10,11 @@ trait PendingAction[R] {
/**
* Completes the action.
*/
def complete(): Try[R]
def complete(implicit ec: ExecutionContext): Future[R]

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

This is a side-effecting method, so reintroducing a the standard parenthesis would be nice.


/**
* Cancels the action.
*/
def cancel()
def cancel(implicit ec: ExecutionContext)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 14, 2018

Member

This is a side-effecting method, so reintroducing a the standard parenthesis would be nice.

This comment has been minimized.

Copy link
@pschichtel

pschichtel Mar 15, 2018

Contributor

I'd also remove the execution context from trait methods, I'd rather inject it into the implementations. Also: public methods should have explicit return types.

fix more errors
remove database access from templates
@@ -107,7 +109,7 @@ class Versions @Inject()(stats: StatTracker,
VersionEditAction(author, slug).async { implicit request =>
implicit val project = request.project
withVersion(versionString) { version =>
project.recommendedVersion = version
project.setRecommendedVersion( version)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 15, 2018

Member

Remove space after parenthesis here

} sortBy { case (p, v, c) =>
ordering
//categories.map(_.toSeq).map { cats =>
val filtered = cats.map { ca =>

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 15, 2018

Member

A fold might be clearer here

userList.map(ul => toJson(ul.map(writeUser)))
}

def writeUser(user: User): JsObject = {

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 15, 2018

Member

Is that signature correct, and if it is, why not use a Writes?

@@ -34,14 +36,17 @@ final class DataHelper @Inject()(config: OreConfig,
/**
* Resets the application to factory defaults.
*/
def reset(): Unit = {
def reset(implicit ec: ExecutionContext): Unit = {

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 15, 2018

Member

This method is not pure, and as such should have an extra set of parenthesis.

val currentUserId = request.data.currentUser.flatMap(_.id).getOrElse(-1)

// TODO platform filter is not implemented
val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform]))

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

Use collectFirst here instead of find and map for single traversal


// TODO platform filter is not implemented
val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform]))
val platformFilter = pform.map(actions.platformFilter).getOrElse(ModelFilter.Empty)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

Fold?

// TODO platform filter is not implemented
val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform]))
val platformFilter = pform.map(actions.platformFilter).getOrElse(ModelFilter.Empty)
var categoryList: Seq[Category] = categories.map(Categories.fromString).map(_.toSeq).getOrElse(Seq.empty)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

Fold?

val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform]))
val platformFilter = pform.map(actions.platformFilter).getOrElse(ModelFilter.Empty)
var categoryList: Seq[Category] = categories.map(Categories.fromString).map(_.toSeq).getOrElse(Seq.empty)
val q = query.map(queryString => s"%${queryString.toLowerCase}%").getOrElse("%")

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

Fold?

(p.description.toLowerCase like q) ||
(p.ownerName.toLowerCase like q) ||
(p.pluginId.toLowerCase like q)
} drop offset take pageSize

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

Unsure if queries have it, but slice often does the job of both drop and take in one.

This comment has been minimized.

Copy link
@Faithcaio

Faithcaio Mar 17, 2018

Author Contributor

doesnt look like it does


projects.queryProjectPages(project.p) flatMap { pages =>
val pageCount = pages.size + pages.values.map(_.size).sum
this.stats.projectViewed(request => Ok(views.pages.view(project, pages, project.p.homePage, None, pageCount)))(request)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

Any reason request is no longer marked as implicit, but instead passed in explicitly here?

@@ -135,7 +135,7 @@ case class Page(override val id: Option[Int] = None,
*
* @return String
*/
def fullSlug: String = if (parentPage.isDefined) { s"${parentPage.get.slug}/${slug}" } else { slug }
def fullSlug(parentPage: Option[Page]): String = if (parentPage.isDefined) { s"${parentPage.get.slug}/$slug" } else { slug }

This comment has been minimized.

Copy link
@Katrix
}

val pform = platform.flatMap(p => Platforms.values.find(_.name.equalsIgnoreCase(p)).map(_.asInstanceOf[Platform]))
val platformFilter = pform.map(actions.platformFilter).getOrElse(ModelFilter.Empty)
if (categoryList != null && Categories.visible.toSet.equals(categoryList.toSet))

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

Fairly certain that categoryList can't be null, only empty.

versions <- futureVersions
r <- this.stats.projectViewed { request =>
Ok(views.list(project, allChannels, versions, visibleNames, p))
}(request)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

Any reason request is no longer marked as implicit, but instead passed in explicitly here?


def of[A](request: Request[A])(implicit cache: AsyncCacheApi, db: JdbcBackend#DatabaseDef, ec: ExecutionContext,
service: ModelService): Future[HeaderData] = {
request.cookies.get("_oretoken") match {

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 17, 2018

Member

flatMap

@Katrix
Copy link
Member

left a comment

Do a favor and run optimize imports on changed files? I counted a lot of unused imports that you've added when I went through everything.

For the stuff that can be parallelized. If you want I can add them together with EitherT and OptionT instead.

I should also say that I haven't looked at the views in any way. Someone else should probably do that.

case None => Future.successful(NotFound)
case Some(channel) =>
for {
nonEmpty <- channel.versions.nonEmpty

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

These futures can be run in parallel

} else {
(result, false)
project.pages.find((ModelFilter[Page](_.slug === parts(0)) +&& ModelFilter[Page](_.parentId === -1)).fn).flatMap {
case Some(r) => Future { (Some(r), false) }

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

Future.successfull

implicit val r = request.request

withPage(data.project, page).flatMap {
case (None, b) => Future.successful(notFound)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

If the b is not used here, mark it as ignored

case (Some(p), b) =>
projects.queryProjectPages(data.project) flatMap { pages =>
val pageCount = pages.size + pages.map(_._2.size).sum
val parentPage = if (pages.contains(p)) None

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

Comparing different types.

val pageCount = pages.size + pages.map(_._2.size).sum
val parentPage = if (pages.contains(p)) None
else pages.collectFirst { case (pp, page) if page.contains(p) => pp }
this.stats.projectViewed(_ => Ok(views.view(data, request.scoped, pages, p, parentPage, pageCount, b)))(cache, request)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

request should be marked as implicit again here

newVersion.addTag(tag)
}
}
val newVersion = channel.flatMap { channel =>

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

Do this in the for comprehension

addTags(ForgeId, "Forge", TagColors.Forge)
for {
channel <- channel
newVersion <- newVersion

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

Collapse this for comprehension and the previous one

for {
channel <- channel
newVersion <- newVersion
spongeTag <- addTags(newVersion, SpongeApiId, "Sponge", TagColors.Sponge)

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

This can be parallelized

}
//categories.map(_.toSeq).map { cats =>
val filtered = cats.map { ca =>
query.filter { case (p, v, c) =>

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

Mark v and c as ignored

@@ -9,7 +9,7 @@ import play.api.libs.functional.syntax._
import play.api.libs.json.Reads._
import util.WSUtils.parseJson
import play.api.libs.json._
import play.api.libs.ws.{WSClient, WSResponse}
import play.api.libs.ws.{WSClient, WSRequest, WSResponse}

This comment has been minimized.

Copy link
@Katrix

Katrix Mar 20, 2018

Member

Unused import. Also remove Await further down.

@felixoi
Copy link
Member

left a comment

  • If you change the project visibility in your branch the visiblity is changed but the view is not updated. I've to reload, I think in master the view changes automatically
  • Remove filter button is always visible also if no categories are selected https://gyazo.com/7d0390cfa4f4de055fca0be7954ff1ed
  • Adding members to a project is not working (always outputs user not found)
  • Adding comments to a review is not working
  • Flag history is broken. It displays there is one entry in the dropdown but opening the history returns a blank one.
  • Child pages are wrongly displayed in the side nav
  • Slugs of child pages are wrong

Faithcaio added some commits Mar 21, 2018

fix pages
fix category filter clearing always being visible
and some other stuff not getting displayed
@Faithcaio

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

project avatar seems to be broken on ore live too

@progwml6

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

file issues please for future work that needs to be done

@felixoi
Copy link
Member

left a comment

I think I've tested mostly everything and I couldn't find more bugs.

Bugs bugs bugs

@progwml6 progwml6 merged commit b4205e5 into master Mar 24, 2018

@progwml6 progwml6 deleted the unawait branch Mar 24, 2018

@Katrix Katrix added this to Done in Backend Improvements Jul 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.