Avoid database access in model

Modified to pass assignee from outside of model instead.
This commit is contained in:
Naoki Takezoe
2017-09-05 19:27:31 +09:00
parent 884fc5318a
commit 9251d64de8
6 changed files with 30 additions and 22 deletions

View File

@@ -2,10 +2,6 @@ package gitbucket.core.api
import gitbucket.core.model.{Account, Issue, IssueComment, PullRequest} import gitbucket.core.model.{Account, Issue, IssueComment, PullRequest}
import java.util.Date import java.util.Date
import gitbucket.core.service.AccountService
import gitbucket.core.model.Profile._
import gitbucket.core.model.Profile.profile.blockingApi._
/** /**
* https://developer.github.com/v3/pulls/ * https://developer.github.com/v3/pulls/
@@ -23,7 +19,7 @@ case class ApiPullRequest(
title: String, title: String,
body: String, body: String,
user: ApiUser, user: ApiUser,
assignee: Either[ApiUser,AnyRef]) extends AccountService { assignee: Option[ApiUser]){
val html_url = ApiPath(s"${base.repo.html_url.path}/pull/${number}") val html_url = ApiPath(s"${base.repo.html_url.path}/pull/${number}")
//val diff_url = ApiPath(s"${base.repo.html_url.path}/pull/${number}.diff") //val diff_url = ApiPath(s"${base.repo.html_url.path}/pull/${number}.diff")
//val patch_url = ApiPath(s"${base.repo.html_url.path}/pull/${number}.patch") //val patch_url = ApiPath(s"${base.repo.html_url.path}/pull/${number}.patch")
@@ -43,8 +39,9 @@ object ApiPullRequest{
headRepo: ApiRepository, headRepo: ApiRepository,
baseRepo: ApiRepository, baseRepo: ApiRepository,
user: ApiUser, user: ApiUser,
assignee: Option[ApiUser],
mergedComment: Option[(IssueComment, Account)] mergedComment: Option[(IssueComment, Account)]
)(implicit s: Session): ApiPullRequest = ): ApiPullRequest =
ApiPullRequest( ApiPullRequest(
number = issue.issueId, number = issue.issueId,
updated_at = issue.updatedDate, updated_at = issue.updatedDate,
@@ -64,17 +61,15 @@ object ApiPullRequest{
title = issue.title, title = issue.title,
body = issue.content.getOrElse(""), body = issue.content.getOrElse(""),
user = user, user = user,
assignee = if (issue.assignedUserName == None) Right(null) else Left(ApiUser(getAccountByUserName(issue.assignedUserName.getOrElse("")).get)) assignee = assignee
) )
case class Commit( case class Commit(
sha: String, sha: String,
ref: String, ref: String,
repo: ApiRepository)(baseOwner:String){ repo: ApiRepository)(baseOwner:String){
val label = if( baseOwner == repo.owner.login ){ ref }else{ s"${repo.owner.login}:${ref}" } val label = if( baseOwner == repo.owner.login ){ ref } else { s"${repo.owner.login}:${ref}" }
val user = repo.owner val user = repo.owner
} }
def getAccountByUserName(userName: String, includeRemoved: Boolean = false)(implicit s: Session): Option[Account] =
Accounts filter(t => (t.userName === userName.bind) && (t.removed === false.bind, !includeRemoved)) firstOption
} }

View File

@@ -20,7 +20,7 @@ object JsonFormat {
{ case x: Date => JString(parserISO.print(new DateTime(x).withZone(DateTimeZone.UTC))) } { case x: Date => JString(parserISO.print(new DateTime(x).withZone(DateTimeZone.UTC))) }
) )
) + FieldSerializer[ApiUser]() + ) + FieldSerializer[ApiUser]() +
FieldSerializer[ApiPullRequest](FieldSerializer.ignore("gitbucket$core$service$AccountService$$logger")) + FieldSerializer[ApiPullRequest]() +
FieldSerializer[ApiRepository]() + FieldSerializer[ApiRepository]() +
FieldSerializer[ApiCommitListItem.Parent]() + FieldSerializer[ApiCommitListItem.Parent]() +
FieldSerializer[ApiCommitListItem]() + FieldSerializer[ApiCommitListItem]() +

View File

@@ -498,7 +498,7 @@ trait ApiControllerBase extends ControllerBase {
val condition = IssueSearchCondition(request) val condition = IssueSearchCondition(request)
val baseOwner = getAccountByUserName(repository.owner).get val baseOwner = getAccountByUserName(repository.owner).get
val issues: List[(Issue, Account, Int, PullRequest, Repository, Account, Account)] = val issues: List[(Issue, Account, Int, PullRequest, Repository, Account, Option[Account])] =
searchPullRequestByApi( searchPullRequestByApi(
condition = condition, condition = condition,
offset = (page - 1) * PullRequestLimit, offset = (page - 1) * PullRequestLimit,
@@ -513,6 +513,7 @@ trait ApiControllerBase extends ControllerBase {
headRepo = ApiRepository(headRepo, ApiUser(headOwner)), headRepo = ApiRepository(headRepo, ApiUser(headOwner)),
baseRepo = ApiRepository(repository, ApiUser(baseOwner)), baseRepo = ApiRepository(repository, ApiUser(baseOwner)),
user = ApiUser(issueUser), user = ApiUser(issueUser),
assignee = assignee.map(ApiUser.apply),
mergedComment = getMergedComment(repository.owner, repository.name, issue.issueId) mergedComment = getMergedComment(repository.owner, repository.name, issue.issueId)
) )
}) })
@@ -529,6 +530,7 @@ trait ApiControllerBase extends ControllerBase {
baseOwner <- users.get(repository.owner) baseOwner <- users.get(repository.owner)
headOwner <- users.get(pullRequest.requestUserName) headOwner <- users.get(pullRequest.requestUserName)
issueUser <- users.get(issue.openedUserName) issueUser <- users.get(issue.openedUserName)
assignee = issue.assignedUserName.flatMap { userName => getAccountByUserName(userName, false) }
headRepo <- getRepository(pullRequest.requestUserName, pullRequest.requestRepositoryName) headRepo <- getRepository(pullRequest.requestUserName, pullRequest.requestRepositoryName)
} yield { } yield {
JsonFormat(ApiPullRequest( JsonFormat(ApiPullRequest(
@@ -537,6 +539,7 @@ trait ApiControllerBase extends ControllerBase {
headRepo = ApiRepository(headRepo, ApiUser(headOwner)), headRepo = ApiRepository(headRepo, ApiUser(headOwner)),
baseRepo = ApiRepository(repository, ApiUser(baseOwner)), baseRepo = ApiRepository(repository, ApiUser(baseOwner)),
user = ApiUser(issueUser), user = ApiUser(issueUser),
assignee = assignee.map(ApiUser.apply),
mergedComment = getMergedComment(repository.owner, repository.name, issue.issueId) mergedComment = getMergedComment(repository.owner, repository.name, issue.issueId)
)) ))
}) getOrElse NotFound() }) getOrElse NotFound()

View File

@@ -199,14 +199,14 @@ trait IssuesService {
* @return (issue, issueUser, commentCount, pullRequest, headRepo, headOwner) * @return (issue, issueUser, commentCount, pullRequest, headRepo, headOwner)
*/ */
def searchPullRequestByApi(condition: IssueSearchCondition, offset: Int, limit: Int, repos: (String, String)*) def searchPullRequestByApi(condition: IssueSearchCondition, offset: Int, limit: Int, repos: (String, String)*)
(implicit s: Session): List[(Issue, Account, Int, PullRequest, Repository, Account, Account)] = { (implicit s: Session): List[(Issue, Account, Int, PullRequest, Repository, Account, Option[Account])] = {
// get issues and comment count and labels // get issues and comment count and labels
searchIssueQueryBase(condition, true, offset, limit, repos) searchIssueQueryBase(condition, true, offset, limit, repos)
.join(PullRequests).on { case t1 ~ t2 ~ i ~ t3 => t3.byPrimaryKey(t1.userName, t1.repositoryName, t1.issueId) } .join (PullRequests).on { case t1 ~ t2 ~ i ~ t3 => t3.byPrimaryKey(t1.userName, t1.repositoryName, t1.issueId) }
.join(Repositories).on { case t1 ~ t2 ~ i ~ t3 ~ t4 => t4.byRepository(t1.userName, t1.repositoryName) } .join (Repositories).on { case t1 ~ t2 ~ i ~ t3 ~ t4 => t4.byRepository(t1.userName, t1.repositoryName) }
.join(Accounts ).on { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 => t5.userName === t1.openedUserName } .join (Accounts ).on { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 => t5.userName === t1.openedUserName }
.join(Accounts ).on { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 ~ t6 => t6.userName === t4.userName } .join (Accounts ).on { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 ~ t6 => t6.userName === t4.userName }
.join(Accounts ).on { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 ~ t6 ~ t7 => t7.userName === t1.assignedUserName} .joinLeft(Accounts ).on { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 ~ t6 ~ t7 => t7.userName === t1.assignedUserName}
.sortBy { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 ~ t6 ~ t7 => i asc } .sortBy { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 ~ t6 ~ t7 => i asc }
.map { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 ~ t6 ~ t7 => (t1, t5, t2.commentCount, t3, t4, t6, t7) } .map { case t1 ~ t2 ~ i ~ t3 ~ t4 ~ t5 ~ t6 ~ t7 => (t1, t5, t2.commentCount, t3, t4, t6, t7) }
.list .list

View File

@@ -228,16 +228,18 @@ trait WebHookPullRequestService extends WebHookService {
callWebHookOf(repository.owner, repository.name, WebHook.PullRequest){ callWebHookOf(repository.owner, repository.name, WebHook.PullRequest){
for{ for{
(issue, pullRequest) <- getPullRequest(repository.owner, repository.name, issueId) (issue, pullRequest) <- getPullRequest(repository.owner, repository.name, issueId)
users = getAccountsByUserNames(Set(repository.owner, pullRequest.requestUserName, issue.openedUserName), Set(sender)) users = getAccountsByUserNames(Set(repository.owner, pullRequest.requestUserName, issue.openedUserName), Set(sender))
baseOwner <- users.get(repository.owner) baseOwner <- users.get(repository.owner)
headOwner <- users.get(pullRequest.requestUserName) headOwner <- users.get(pullRequest.requestUserName)
issueUser <- users.get(issue.openedUserName) issueUser <- users.get(issue.openedUserName)
assignee = issue.assignedUserName.flatMap { userName => getAccountByUserName(userName, false) }
headRepo <- getRepository(pullRequest.requestUserName, pullRequest.requestRepositoryName) headRepo <- getRepository(pullRequest.requestUserName, pullRequest.requestRepositoryName)
} yield { } yield {
WebHookPullRequestPayload( WebHookPullRequestPayload(
action = action, action = action,
issue = issue, issue = issue,
issueUser = issueUser, issueUser = issueUser,
assignee = assignee,
pullRequest = pullRequest, pullRequest = pullRequest,
headRepository = headRepo, headRepository = headRepo,
headOwner = headOwner, headOwner = headOwner,
@@ -273,12 +275,14 @@ trait WebHookPullRequestService extends WebHookService {
import WebHookService._ import WebHookService._
for{ for{
((issue, issueUser, pullRequest, baseOwner, headOwner), webHooks) <- getPullRequestsByRequestForWebhook(requestRepository.owner, requestRepository.name, requestBranch) ((issue, issueUser, pullRequest, baseOwner, headOwner), webHooks) <- getPullRequestsByRequestForWebhook(requestRepository.owner, requestRepository.name, requestBranch)
assignee = issue.assignedUserName.flatMap { userName => getAccountByUserName(userName, false) }
baseRepo <- getRepository(pullRequest.userName, pullRequest.repositoryName) baseRepo <- getRepository(pullRequest.userName, pullRequest.repositoryName)
} yield { } yield {
val payload = WebHookPullRequestPayload( val payload = WebHookPullRequestPayload(
action = action, action = action,
issue = issue, issue = issue,
issueUser = issueUser, issueUser = issueUser,
assignee = assignee,
pullRequest = pullRequest, pullRequest = pullRequest,
headRepository = requestRepository, headRepository = requestRepository,
headOwner = headOwner, headOwner = headOwner,
@@ -306,6 +310,7 @@ trait WebHookPullRequestReviewCommentService extends WebHookService {
baseOwner <- users.get(repository.owner) baseOwner <- users.get(repository.owner)
headOwner <- users.get(pullRequest.requestUserName) headOwner <- users.get(pullRequest.requestUserName)
issueUser <- users.get(issue.openedUserName) issueUser <- users.get(issue.openedUserName)
assignee = issue.assignedUserName.flatMap { userName => getAccountByUserName(userName, false) }
headRepo <- getRepository(pullRequest.requestUserName, pullRequest.requestRepositoryName) headRepo <- getRepository(pullRequest.requestUserName, pullRequest.requestRepositoryName)
} yield { } yield {
WebHookPullRequestReviewCommentPayload( WebHookPullRequestReviewCommentPayload(
@@ -313,6 +318,7 @@ trait WebHookPullRequestReviewCommentService extends WebHookService {
comment = comment, comment = comment,
issue = issue, issue = issue,
issueUser = issueUser, issueUser = issueUser,
assignee = assignee,
pullRequest = pullRequest, pullRequest = pullRequest,
headRepository = headRepo, headRepository = headRepo,
headOwner = headOwner, headOwner = headOwner,
@@ -424,13 +430,14 @@ object WebHookService {
def apply(action: String, def apply(action: String,
issue: Issue, issue: Issue,
issueUser: Account, issueUser: Account,
assignee: Option[Account],
pullRequest: PullRequest, pullRequest: PullRequest,
headRepository: RepositoryInfo, headRepository: RepositoryInfo,
headOwner: Account, headOwner: Account,
baseRepository: RepositoryInfo, baseRepository: RepositoryInfo,
baseOwner: Account, baseOwner: Account,
sender: Account, sender: Account,
mergedComment: Option[(IssueComment, Account)])(implicit s: Session): WebHookPullRequestPayload = { mergedComment: Option[(IssueComment, Account)]): WebHookPullRequestPayload = {
val headRepoPayload = ApiRepository(headRepository, headOwner) val headRepoPayload = ApiRepository(headRepository, headOwner)
val baseRepoPayload = ApiRepository(baseRepository, baseOwner) val baseRepoPayload = ApiRepository(baseRepository, baseOwner)
@@ -441,6 +448,7 @@ object WebHookService {
headRepo = headRepoPayload, headRepo = headRepoPayload,
baseRepo = baseRepoPayload, baseRepo = baseRepoPayload,
user = ApiUser(issueUser), user = ApiUser(issueUser),
assignee = assignee.map(ApiUser.apply),
mergedComment = mergedComment mergedComment = mergedComment
) )
@@ -495,6 +503,7 @@ object WebHookService {
comment: CommitComment, comment: CommitComment,
issue: Issue, issue: Issue,
issueUser: Account, issueUser: Account,
assignee: Option[Account],
pullRequest: PullRequest, pullRequest: PullRequest,
headRepository: RepositoryInfo, headRepository: RepositoryInfo,
headOwner: Account, headOwner: Account,
@@ -502,7 +511,7 @@ object WebHookService {
baseOwner: Account, baseOwner: Account,
sender: Account, sender: Account,
mergedComment: Option[(IssueComment, Account)] mergedComment: Option[(IssueComment, Account)]
)(implicit s: Session) : WebHookPullRequestReviewCommentPayload = { ): WebHookPullRequestReviewCommentPayload = {
val headRepoPayload = ApiRepository(headRepository, headOwner) val headRepoPayload = ApiRepository(headRepository, headOwner)
val baseRepoPayload = ApiRepository(baseRepository, baseOwner) val baseRepoPayload = ApiRepository(baseRepository, baseOwner)
val senderPayload = ApiUser(sender) val senderPayload = ApiUser(sender)
@@ -521,6 +530,7 @@ object WebHookService {
headRepo = headRepoPayload, headRepo = headRepoPayload,
baseRepo = baseRepoPayload, baseRepo = baseRepoPayload,
user = ApiUser(issueUser), user = ApiUser(issueUser),
assignee = assignee.map(ApiUser.apply),
mergedComment = mergedComment mergedComment = mergedComment
), ),
repository = baseRepoPayload, repository = baseRepoPayload,

View File

@@ -282,7 +282,7 @@ class JsonFormatSpec extends FunSuite {
title = "new-feature", title = "new-feature",
body = "Please pull these awesome changes", body = "Please pull these awesome changes",
user = apiUser, user = apiUser,
assignee = Left(apiUser) assignee = Some(apiUser)
) )
val apiPullRequestJson = s"""{ val apiPullRequestJson = s"""{