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
Merged

Remove awaiting Future and more Improvements #501

merged 50 commits into from Mar 24, 2018

Conversation

Faithcaio
Copy link
Contributor

@Faithcaio Faithcaio 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 March 13, 2018 15:42
@progwml6
Copy link
Member

@Faithcaio this PR has conflicts

@phase
Copy link
Contributor

phase 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. 🎉 🎉

This includes the Play 2.6 updates.

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

phase commented Mar 14, 2018

I fixed the conflicts. 👍

@Katrix
Copy link
Member

Katrix commented Mar 14, 2018

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

@Faithcaio
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for scala.Seq here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing handling of None here.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use two for comprehensions here?

}

val channelCleanup = for {
_ <- rcUpdate
proj <- rcUpdate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use two for comprehensions here?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove space after parenthesis here

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fold might be clearer here

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

def writeUser(user: User): JsObject = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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("%")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fold?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fold

}

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flatMap

Copy link
Member

@Katrix Katrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future.successfull

implicit val r = request.request

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request should be marked as implicit again here

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do this in the for comprehension

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collapse this for comprehension and the previous one

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be parallelized

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import. Also remove Await further down.

Copy link
Member

@felixoi felixoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

fix category filter clearing always being visible
and some other stuff not getting displayed
@Faithcaio
Copy link
Contributor Author

project avatar seems to be broken on ore live too

@progwml6
Copy link
Member

file issues please for future work that needs to be done

felixoi
felixoi previously approved these changes Mar 22, 2018
Copy link
Member

@felixoi felixoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@felixoi felixoi dismissed their stale review March 22, 2018 16:23

Bugs bugs bugs

@progwml6 progwml6 merged commit b4205e5 into master Mar 24, 2018
@progwml6 progwml6 deleted the unawait branch March 24, 2018 15:22
@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
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants