Fix & refactor branch name generation+parsing

9 files changed, 71 insertions, 52 deletions

diff --git a/src/main/scala/com/getbootstrap/savage/github/CommitSha.scala b/src/main/scala/com/getbootstrap/savage/github/CommitSha.scala
index 75f77f3..e77deb4 100644
--- a/src/main/scala/com/getbootstrap/savage/github/CommitSha.scala
+++ b/src/main/scala/com/getbootstrap/savage/github/CommitSha.scala
@@ -8,6 +8,7 @@ object CommitSha {
       case _ => None
     }
   }
+  def unapply(sha: String): Option[CommitSha] = CommitSha(sha)
 }
 
 class CommitSha private(val sha: String) extends AnyVal {
diff --git a/src/main/scala/com/getbootstrap/savage/github/GitHubActorWithLogging.scala b/src/main/scala/com/getbootstrap/savage/github/GitHubActorWithLogging.scala
index 2dde9de..9b231f3 100644
--- a/src/main/scala/com/getbootstrap/savage/github/GitHubActorWithLogging.scala
+++ b/src/main/scala/com/getbootstrap/savage/github/GitHubActorWithLogging.scala
@@ -4,7 +4,7 @@ import org.eclipse.egit.github.core.client.GitHubClient
 import com.getbootstrap.savage.server.{Settings, ActorWithLogging}
 
 abstract class GitHubActorWithLogging extends ActorWithLogging {
-  protected val settings = Settings(context.system)
+  protected implicit val settings = Settings(context.system)
   protected val gitHubClient = new GitHubClient()
   gitHubClient.setUserAgent(settings.UserAgent)
   gitHubClient.setCredentials(settings.BotUsername, settings.BotPassword)
diff --git a/src/main/scala/com/getbootstrap/savage/github/PullRequestNumber.scala b/src/main/scala/com/getbootstrap/savage/github/PullRequestNumber.scala
index f7043cb..1ac41b8 100644
--- a/src/main/scala/com/getbootstrap/savage/github/PullRequestNumber.scala
+++ b/src/main/scala/com/getbootstrap/savage/github/PullRequestNumber.scala
@@ -9,6 +9,7 @@ object PullRequestNumber {
       None
     }
   }
+  def unapply(number: Int): Option[PullRequestNumber] = PullRequestNumber(number)
 }
 class PullRequestNumber private(val number: Int) extends AnyVal {
   override def toString = s"PullRequestNumber(${number})"
diff --git a/src/main/scala/com/getbootstrap/savage/github/SavageBranch.scala b/src/main/scala/com/getbootstrap/savage/github/SavageBranch.scala
new file mode 100644
index 0000000..94aa4c3
--- /dev/null
+++ b/src/main/scala/com/getbootstrap/savage/github/SavageBranch.scala
@@ -0,0 +1,17 @@
+package com.getbootstrap.savage.github
+
+import com.getbootstrap.savage.server.SettingsImpl
+import com.getbootstrap.savage.util.{IntFromStr, PrefixedString}
+
+object SavageBranch {
+  def apply(branch: Branch)(implicit settings: SettingsImpl): Option[SavageBranch] = {
+    branch.name.unprefix(settings.BranchPrefix).map{ _.split("-") } match {
+      case Some(Array(IntFromStr(PullRequestNumber(num)), CommitSha(sha))) => Some(SavageBranch(num, sha))
+      case _ => None
+    }
+  }
+  def unapply(branch: Branch)(implicit settings: SettingsImpl): Option[(PullRequestNumber, CommitSha)] = SavageBranch(branch).map{ savBr => (savBr.prNum, savBr.commitSha) }
+}
+case class SavageBranch(prNum: PullRequestNumber, commitSha: CommitSha) {
+  def branch(implicit settings: SettingsImpl): Branch = Branch(s"${settings.BranchPrefix}${prNum.number}-${commitSha.sha}").get
+}
diff --git a/src/main/scala/com/getbootstrap/savage/server/BranchDeleter.scala b/src/main/scala/com/getbootstrap/savage/server/BranchDeleter.scala
index c113d14..8bfd7fa 100644
--- a/src/main/scala/com/getbootstrap/savage/server/BranchDeleter.scala
+++ b/src/main/scala/com/getbootstrap/savage/server/BranchDeleter.scala
@@ -2,34 +2,27 @@ package com.getbootstrap.savage.server
 
 import scala.collection.JavaConverters._
 import org.eclipse.egit.github.core.service.RepositoryService
-import com.getbootstrap.savage.github.{GitHubActorWithLogging, Branch}
+import com.getbootstrap.savage.github.{SavageBranch, GitHubActorWithLogging}
 import com.getbootstrap.savage.github.util.RichRepositoryId
 import com.getbootstrap.savage.util.{SuccessfulExit, ErrorExit, SimpleSubprocess}
 
 class BranchDeleter extends GitHubActorWithLogging {
   override def receive = {
-    case branch:Branch => {
-      if (isSavageBranch(branch)) {
-        val repoService = new RepositoryService(gitHubClient)
-        val maybeRepoBranch = repoService.getBranches(settings.TestRepoId).asScala.find{ _.getName == branch.name }
-        maybeRepoBranch match {
-          case None => log.info(s"Nothing to delete; ${branch} does not exist in ${settings.TestRepoId}")
-          case Some(repoBranch) => {
-            val remote = settings.TestRepoId.asPushRemote
-            val process = SimpleSubprocess(Seq("git", "push", remote, ":" + branch.name))
-            log.info(s"Deleting ${branch} from remote ${remote}")
-            process.run() match {
-              case SuccessfulExit(_) => log.info(s"Successfully deleted ${branch} in ${remote}")
-              case ErrorExit(exitValue, output) => log.error(s"Error deleting ${branch} in ${remote} :\nExit code: ${exitValue}\n${output}")
-            }
+    case branch:SavageBranch => {
+      val repoService = new RepositoryService(gitHubClient)
+      val maybeRepoBranch = repoService.getBranches(settings.TestRepoId).asScala.find{ _.getName == branch.branch.name }
+      maybeRepoBranch match {
+        case None => log.info(s"Nothing to delete; ${branch} does not exist in ${settings.TestRepoId}")
+        case Some(repoBranch) => {
+          val remote = settings.TestRepoId.asPushRemote
+          val process = SimpleSubprocess(Seq("git", "push", remote, ":" + branch.branch.name))
+          log.info(s"Deleting ${branch} from remote ${remote}")
+          process.run() match {
+            case SuccessfulExit(_) => log.info(s"Successfully deleted ${branch} in ${remote}")
+            case ErrorExit(exitValue, output) => log.error(s"Error deleting ${branch} in ${remote} :\nExit code: ${exitValue}\n${output}")
           }
         }
       }
-      else {
-        log.error(s"Not deleting non-Savage branch : ${branch}")
-      }
     }
   }
-
-  private def isSavageBranch(branch: Branch): Boolean = branch.name startsWith settings.BranchPrefix
 }
diff --git a/src/main/scala/com/getbootstrap/savage/server/PullRequestPusher.scala b/src/main/scala/com/getbootstrap/savage/server/PullRequestPusher.scala
index 7d3c8ec..030736e 100644
--- a/src/main/scala/com/getbootstrap/savage/server/PullRequestPusher.scala
+++ b/src/main/scala/com/getbootstrap/savage/server/PullRequestPusher.scala
@@ -35,13 +35,8 @@ class PullRequestPusher(
   }
 
   def push(originRepo: RepositoryId, prNum: PullRequestNumber, commitSha: CommitSha): Boolean = {
-    val newBranch = {
-      val branchName = s"${settings.BranchPrefix}${prNum.number}-${commitSha.sha}"
-      Branch(branchName).getOrElse {
-        throw new SecurityException(s"Generated insecure branch name: ${branchName}")
-      }
-    }
-    val branchSpec = s"${commitSha.sha}:${newBranch.asRef}"
+    val newBranch = SavageBranch(prNum, commitSha)
+    val branchSpec = s"${commitSha.sha}:${newBranch.branch.asRef}"
     val destRemote = settings.TestRepoId.asPushRemote
     val success = SimpleSubprocess(Seq("git", "push", "-f", destRemote, branchSpec)).run() match {
       case SuccessfulExit(_) => {
@@ -60,7 +55,7 @@ class PullRequestPusher(
     success
   }
 
-  private def scheduleFailsafeBranchDeletion(branch: Branch) {
+  private def scheduleFailsafeBranchDeletion(branch: SavageBranch) {
     implicit val execContext = context.system.dispatcher
     context.system.scheduler.scheduleOnce(settings.TravisTimeout, branchDeleter, branch)
   }
diff --git a/src/main/scala/com/getbootstrap/savage/server/SavageWebService.scala b/src/main/scala/com/getbootstrap/savage/server/SavageWebService.scala
index 0b26fd5..40515d8 100644
--- a/src/main/scala/com/getbootstrap/savage/server/SavageWebService.scala
+++ b/src/main/scala/com/getbootstrap/savage/server/SavageWebService.scala
@@ -1,11 +1,10 @@
 package com.getbootstrap.savage.server
 
-import scala.util.{Try,Success,Failure}
 import akka.actor.ActorRef
 import spray.routing._
 import spray.http._
 import com.getbootstrap.savage.PullRequestBuildResult
-import com.getbootstrap.savage.github.{PullRequestNumber, commit_status, pr_action, event=>events}
+import com.getbootstrap.savage.github.{SavageBranch, commit_status, pr_action, event => events}
 import com.getbootstrap.savage.github.commit_status.StatusForCommit
 import com.getbootstrap.savage.github.util._
 
@@ -18,7 +17,7 @@ class SavageWebService(
   import GitHubWebHooksDirectives.{gitHubEvent,authenticatedPullRequestEvent,authenticatedIssueOrCommentEvent}
   import TravisWebHookDirectives.authenticatedTravisEvent
 
-  private val settings = Settings(context.system)
+  private implicit val settings = Settings(context.system)
   override def actorRefFactory = context
   override def receive = runRoute(theOnlyRoute)
 
@@ -71,28 +70,23 @@ class SavageWebService(
         pathEndOrSingleSlash {
           post {
             authenticatedTravisEvent(travisToken = settings.TravisToken, repo = settings.TestRepoId, log = log) { event =>
-              if (event.branchName.name.startsWith(settings.BranchPrefix)) {
-                Try { Integer.parseInt(event.branchName.name.stripPrefix(settings.BranchPrefix)) }.flatMap{ intStr => Try{ PullRequestNumber(intStr).get } } match {
-                  case Failure(exc) => log.error(exc, s"Invalid Savage branch name from Travis event: ${event.branchName}")
-                  case Success(prNum) => {
-                    branchDeleter ! event.branchName
-                    val commitStatus = if (event.status.isSuccessful) {
-                      commit_status.Success("CONFIRMED: Savage cross-browser JS tests passed", event.buildUrl)
-                    } else {
-                      commit_status.Failure("BUSTED: Savage cross-browser JS tests failed", event.buildUrl)
-                    }
-                    statusSetter ! StatusForCommit(event.commitSha, commitStatus)
-                    pullRequestCommenter ! PullRequestBuildResult(
-                      prNum = prNum,
-                      commitSha = event.commitSha,
-                      buildUrl = event.buildUrl,
-                      succeeded = event.status.isSuccessful
-                    )
+              event.branchName match {
+                case branch@SavageBranch(prNum, _) => {
+                  branchDeleter ! branch
+                  val commitStatus = if (event.status.isSuccessful) {
+                    commit_status.Success("CONFIRMED: Savage cross-browser JS tests passed", event.buildUrl)
+                  } else {
+                    commit_status.Failure("BUSTED: Savage cross-browser JS tests failed", event.buildUrl)
                   }
+                  statusSetter ! StatusForCommit(event.commitSha, commitStatus)
+                  pullRequestCommenter ! PullRequestBuildResult(
+                    prNum = prNum,
+                    commitSha = event.commitSha,
+                    buildUrl = event.buildUrl,
+                    succeeded = event.status.isSuccessful
+                  )
                 }
-              }
-              else {
-                log.info(s"Ignoring authentic Travis event from irrelevant ${event.branchName}")
+                case badBranch => log.info(s"Ignoring authentic Travis event from irrelevant or invalid ${badBranch}")
               }
               complete(StatusCodes.OK)
             }
diff --git a/src/main/scala/com/getbootstrap/savage/util/IntFromStr.scala b/src/main/scala/com/getbootstrap/savage/util/IntFromStr.scala
new file mode 100644
index 0000000..2aa2d7d
--- /dev/null
+++ b/src/main/scala/com/getbootstrap/savage/util/IntFromStr.scala
@@ -0,0 +1,7 @@
+package com.getbootstrap.savage.util
+
+import scala.util.Try
+
+object IntFromStr {
+  def unapply(str: String): Option[Int] = Try{ Integer.parseInt(str) }.toOption
+}
diff --git a/src/main/scala/com/getbootstrap/savage/util/package.scala b/src/main/scala/com/getbootstrap/savage/util/package.scala
index 7fc20da..68b1e93 100644
--- a/src/main/scala/com/getbootstrap/savage/util/package.scala
+++ b/src/main/scala/com/getbootstrap/savage/util/package.scala
@@ -20,6 +20,17 @@ package object util {
     def utf8String: Try[String] = Try { new String(bytes, utf8) }
   }
 
+  implicit class PrefixedString(str: String) {
+    def unprefix(prefix: String): Option[String] = {
+      if (str.startsWith(prefix)) {
+        Some(str.stripPrefix(prefix))
+      }
+      else {
+        None
+      }
+    }
+  }
+
   private object UnixFileSystemString {
     private lazy val unixFileSystem: FileSystem = {
       // get a Unix-y FileSystem, or fail hard