From 34016375e67badcb91987daf6db30b84c0632e1b Mon Sep 17 00:00:00 2001 From: Anatoly Kislov Date: Wed, 27 Sep 2017 16:28:40 +0300 Subject: [PATCH 1/3] feat: common error handling for observers (APP-106) --- src/main/kotlin/app/Logger.kt | 2 +- src/main/kotlin/app/hashers/CommitHasher.kt | 10 ++---- src/main/kotlin/app/hashers/FactHasher.kt | 11 +++--- src/main/kotlin/app/hashers/RepoHasher.kt | 33 ++++++++++------- .../test/tests/hashers/CommitHasherTest.kt | 36 +++++++++++++++---- .../test/tests/hashers/FactHasherTest.kt | 8 +++-- 6 files changed, 65 insertions(+), 35 deletions(-) diff --git a/src/main/kotlin/app/Logger.kt b/src/main/kotlin/app/Logger.kt index 1cc15a2f..a8bf6cd8 100644 --- a/src/main/kotlin/app/Logger.kt +++ b/src/main/kotlin/app/Logger.kt @@ -44,7 +44,7 @@ object Logger { /** * Log error message with exception info. */ - fun error(message: String, e: Exception) { + fun error(message: String, e: Throwable) { if (LEVEL >= ERROR) { println("[e] $message: $e") } diff --git a/src/main/kotlin/app/hashers/CommitHasher.kt b/src/main/kotlin/app/hashers/CommitHasher.kt index e9cc1d81..1ff90693 100644 --- a/src/main/kotlin/app/hashers/CommitHasher.kt +++ b/src/main/kotlin/app/hashers/CommitHasher.kt @@ -61,12 +61,11 @@ class CommitHasher(private val localRepo: LocalRepo, } // Hash added and missing server commits and send them to server. - fun updateFromObservable(observable: Observable) { + fun updateFromObservable(observable: Observable, + onError: (Throwable) -> Unit) { val lastKnownCommit = serverRepo.commits.lastOrNull() val knownCommits = serverRepo.commits.toHashSet() - val throwables = mutableListOf() - observable .takeWhile { new -> // Hash until last known commit. new.rehash != lastKnownCommit?.rehash @@ -94,9 +93,6 @@ class CommitHasher(private val localRepo: LocalRepo, .buffer(20, TimeUnit.SECONDS) // Group ready commits by time. .subscribe({ commitsBundle -> // OnNext. postCommitsToServer(commitsBundle) // Send ready commits. - }, { e -> // OnError. - Logger.error("Hashing error: " + e.message) - throwables.add(e) // TODO(anatoly): Top-class handling errors. - }) + }, onError) } } diff --git a/src/main/kotlin/app/hashers/FactHasher.kt b/src/main/kotlin/app/hashers/FactHasher.kt index 08de2450..6e9ba6cd 100644 --- a/src/main/kotlin/app/hashers/FactHasher.kt +++ b/src/main/kotlin/app/hashers/FactHasher.kt @@ -29,12 +29,11 @@ class FactHasher(private val localRepo: LocalRepo, } } - fun updateFromObservable(observable: Observable) { + fun updateFromObservable(observable: Observable, + onError: (Throwable) -> Unit) { val factsDayWeek = hashMapOf>() val factsDayTime = hashMapOf>() - val throwables = mutableListOf() - // TODO(anatoly): Filter hashing by email as in CommitHasher. observable .subscribe({ commit -> // OnNext. @@ -51,9 +50,7 @@ class FactHasher(private val localRepo: LocalRepo, factDayTime[dateTime.hour] += 1 factsDayWeek[author] = factDayWeek factsDayTime[author] = factDayTime - }, { e -> // OnError. - throwables.add(e) // TODO(anatoly): Top-class handling errors. - }, { // OnComplete. + }, onError, { // OnComplete. try { val facts = mutableListOf() factsDayTime.map { (author, list) -> @@ -76,7 +73,7 @@ class FactHasher(private val localRepo: LocalRepo, } postFactsToServer(facts) } catch (e: Throwable) { - throwables.add(e) + onError(e) } }) } diff --git a/src/main/kotlin/app/hashers/RepoHasher.kt b/src/main/kotlin/app/hashers/RepoHasher.kt index 19dc2e18..8d416622 100644 --- a/src/main/kotlin/app/hashers/RepoHasher.kt +++ b/src/main/kotlin/app/hashers/RepoHasher.kt @@ -46,26 +46,39 @@ class RepoHasher(private val localRepo: LocalRepo, private val api: Api, // Get repo setup (commits, emails to hash) from server. getRepoFromServer() + // Common error handling for subscribers. + // Exceptions can't be thrown out of reactive chain. + val errors = mutableListOf() + val onError: (Throwable) -> Unit = { + e -> errors.add(e) + Logger.error("Error while hashing:", e) + } + // Hash by all plugins. val observable = CommitCrawler.getObservable(git, serverRepo) .publish() CommitHasher(localRepo, serverRepo, api, rehashes) - .updateFromObservable(observable) + .updateFromObservable(observable, onError) FactHasher(localRepo, serverRepo, api) - .updateFromObservable(observable) + .updateFromObservable(observable, onError) // Start and synchronously wait until all subscribers complete. observable.connect() // TODO(anatoly): CodeLongevity hash from observable. - CodeLongevity(localRepo, serverRepo, api, git).update() + try { + CodeLongevity(localRepo, serverRepo, api, git).update() + } + catch (e: Throwable) { + onError(e) + } // Confirm hashing completion. postRepoToServer() + println("Hashing $localRepo successfully finished.") } finally { closeGit(git) } - println("Hashing $localRepo successfully finished.") } private fun loadGit(path: String): Git { @@ -104,7 +117,7 @@ class RepoHasher(private val localRepo: LocalRepo, private val api: Api, } private fun fetchRehashesAndAuthors(git: Git): - Pair, HashMap> { + Pair, HashSet> { val head: RevCommit = RevWalk(git.repository) .parseCommit(git.repository.resolve(RepoHelper.MASTER_BRANCH)) @@ -112,21 +125,17 @@ class RepoHasher(private val localRepo: LocalRepo, private val api: Api, revWalk.markStart(head) val commitsRehashes = LinkedList() - val contributors = hashMapOf() + val authors = hashSetOf() var commit: RevCommit? = revWalk.next() while (commit != null) { commitsRehashes.add(DigestUtils.sha256Hex(commit.name)) - if (!contributors.containsKey(commit.authorIdent.emailAddress)) { - val author = Author(commit.authorIdent.name, - commit.authorIdent.emailAddress) - contributors.put(commit.authorIdent.emailAddress, author) - } + authors.add(commit.authorIdent.emailAddress) commit.disposeBody() commit = revWalk.next() } revWalk.dispose() - return Pair(commitsRehashes, contributors) + return Pair(commitsRehashes, authors) } } diff --git a/src/test/kotlin/test/tests/hashers/CommitHasherTest.kt b/src/test/kotlin/test/tests/hashers/CommitHasherTest.kt index 4405500d..00d9a01b 100644 --- a/src/test/kotlin/test/tests/hashers/CommitHasherTest.kt +++ b/src/test/kotlin/test/tests/hashers/CommitHasherTest.kt @@ -63,10 +63,15 @@ class CommitHasherTest : Spek({ given("repo with initial commit and no history") { repo.commits = listOf() + val errors = mutableListOf() val mockApi = MockApi(mockRepo = repo) val observable = CommitCrawler.getObservable(gitHasher, repo) CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) - .updateFromObservable(observable) + .updateFromObservable(observable, { e -> errors.add(e) }) + + it ("have no errors") { + assertEquals(0, errors.size) + } it("send added commits") { assertEquals(1, mockApi.receivedAddedCommits.size) @@ -80,10 +85,15 @@ class CommitHasherTest : Spek({ given("repo with initial commit") { repo.commits = listOf(getLastCommit(git)) + val errors = mutableListOf() val mockApi = MockApi(mockRepo = repo) val observable = CommitCrawler.getObservable(gitHasher, repo) CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) - .updateFromObservable(observable) + .updateFromObservable(observable, { e -> errors.add(e) }) + + it ("have no errors") { + assertEquals(0, errors.size) + } it("doesn't send added commits") { assertEquals(0, mockApi.receivedAddedCommits.size) @@ -97,13 +107,17 @@ class CommitHasherTest : Spek({ given("happy path: added one commit") { repo.commits = listOf(getLastCommit(git)) + val errors = mutableListOf() val mockApi = MockApi(mockRepo = repo) - val revCommit = git.commit().setMessage("Second commit.").call() val addedCommit = Commit(revCommit) val observable = CommitCrawler.getObservable(gitHasher, repo) CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) - .updateFromObservable(observable) + .updateFromObservable(observable, { e -> errors.add(e) }) + + it ("have no errors") { + assertEquals(0, errors.size) + } it("doesn't send deleted commits") { assertEquals(0, mockApi.receivedDeletedCommits.size) @@ -121,6 +135,7 @@ class CommitHasherTest : Spek({ given("happy path: added a few new commits") { repo.commits = listOf(getLastCommit(git)) + val errors = mutableListOf() val mockApi = MockApi(mockRepo = repo) val otherAuthorsNames = listOf("a", "b", "a") @@ -139,7 +154,11 @@ class CommitHasherTest : Spek({ } val observable = CommitCrawler.getObservable(gitHasher, repo) CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) - .updateFromObservable(observable) + .updateFromObservable(observable, { e -> errors.add(e) }) + + it ("have no errors") { + assertEquals(0, errors.size) + } it("posts five commits as added") { assertEquals(5, mockApi.receivedAddedCommits.size) @@ -185,6 +204,7 @@ class CommitHasherTest : Spek({ given("lost server") { repo.commits = listOf(getLastCommit(git)) + val errors = mutableListOf() val mockApi = MockApi(mockRepo = repo) // Add some commits. @@ -201,7 +221,11 @@ class CommitHasherTest : Spek({ val observable = CommitCrawler.getObservable(gitHasher, repo) CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) - .updateFromObservable(observable) + .updateFromObservable(observable, { e -> errors.add(e) }) + + it ("have no errors") { + assertEquals(0, errors.size) + } it("adds posts one commit as added and received commit is lost one") { assertEquals(1, mockApi.receivedAddedCommits.size) diff --git a/src/test/kotlin/test/tests/hashers/FactHasherTest.kt b/src/test/kotlin/test/tests/hashers/FactHasherTest.kt index ba0de54e..203c2c82 100644 --- a/src/test/kotlin/test/tests/hashers/FactHasherTest.kt +++ b/src/test/kotlin/test/tests/hashers/FactHasherTest.kt @@ -55,10 +55,12 @@ class FactHasherTest : Spek({ date = createDate(year = 2017, month = 1, day = 1, // Sunday. hour = 13, minute = 0, seconds = 0)) + val errors = mutableListOf() val observable = CommitCrawler.getObservable(testRepo.git, repo) FactHasher(localRepo, repo, mockApi) - .updateFromObservable(observable) + .updateFromObservable(observable, { e -> errors.add(e) }) + assertEquals(0, errors.size) assertEquals(2, facts.size) assertTrue(facts.contains(Fact(repo, FactCodes.COMMITS_DAY_TIME, 13, 1.0, author1))) @@ -79,10 +81,12 @@ class FactHasherTest : Spek({ date = createDate(year=2017, month = 1, day = 2, // Monday. hour = 13, minute = 0, seconds = 0)) + val errors = mutableListOf() val observable = CommitCrawler.getObservable(testRepo.git, repo) FactHasher(localRepo, repo, mockApi) - .updateFromObservable(observable) + .updateFromObservable(observable, { e -> errors.add(e) }) + assertEquals(0, errors.size) assertEquals(5, facts.size) assertTrue(facts.contains(Fact(repo, FactCodes.COMMITS_DAY_TIME, 18, 1.0, author2))) From 287b5f12ad2f2606fa9e5dcc58154c8f662adf6d Mon Sep 17 00:00:00 2001 From: Anatoly Kislov Date: Fri, 29 Sep 2017 17:02:45 +0300 Subject: [PATCH 2/3] chore: fix spelling --- src/test/kotlin/test/tests/hashers/CommitHasherTest.kt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/kotlin/test/tests/hashers/CommitHasherTest.kt b/src/test/kotlin/test/tests/hashers/CommitHasherTest.kt index 00d9a01b..c3b71403 100644 --- a/src/test/kotlin/test/tests/hashers/CommitHasherTest.kt +++ b/src/test/kotlin/test/tests/hashers/CommitHasherTest.kt @@ -69,7 +69,7 @@ class CommitHasherTest : Spek({ CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) .updateFromObservable(observable, { e -> errors.add(e) }) - it ("have no errors") { + it ("has no errors") { assertEquals(0, errors.size) } @@ -91,7 +91,7 @@ class CommitHasherTest : Spek({ CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) .updateFromObservable(observable, { e -> errors.add(e) }) - it ("have no errors") { + it ("has no errors") { assertEquals(0, errors.size) } @@ -115,7 +115,7 @@ class CommitHasherTest : Spek({ CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) .updateFromObservable(observable, { e -> errors.add(e) }) - it ("have no errors") { + it ("has no errors") { assertEquals(0, errors.size) } @@ -156,7 +156,7 @@ class CommitHasherTest : Spek({ CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) .updateFromObservable(observable, { e -> errors.add(e) }) - it ("have no errors") { + it ("has no errors") { assertEquals(0, errors.size) } @@ -223,7 +223,7 @@ class CommitHasherTest : Spek({ CommitHasher(localRepo, repo, mockApi, repo.commits.map {it.rehash}) .updateFromObservable(observable, { e -> errors.add(e) }) - it ("have no errors") { + it ("has no errors") { assertEquals(0, errors.size) } From 1725736be7086d59309e1bec710bc25c24736374 Mon Sep 17 00:00:00 2001 From: Anatoly Kislov Date: Fri, 29 Sep 2017 17:33:00 +0300 Subject: [PATCH 3/3] wip: hashing error processing --- src/main/kotlin/app/hashers/RepoHasher.kt | 9 ++++++++- src/main/kotlin/app/ui/UpdateRepoState.kt | 10 +++++++--- src/main/kotlin/app/utils/HashingException.kt | 6 ++++++ 3 files changed, 21 insertions(+), 4 deletions(-) create mode 100644 src/main/kotlin/app/utils/HashingException.kt diff --git a/src/main/kotlin/app/hashers/RepoHasher.kt b/src/main/kotlin/app/hashers/RepoHasher.kt index 8d416622..240f1cf0 100644 --- a/src/main/kotlin/app/hashers/RepoHasher.kt +++ b/src/main/kotlin/app/hashers/RepoHasher.kt @@ -6,9 +6,9 @@ package app.hashers import app.Logger import app.api.Api import app.config.Configurator -import app.model.Author import app.model.LocalRepo import app.model.Repo +import app.utils.HashingException import app.utils.RepoHelper import org.apache.commons.codec.digest.DigestUtils import org.eclipse.jgit.api.Git @@ -26,7 +26,9 @@ class RepoHasher(private val localRepo: LocalRepo, private val api: Api, if (!RepoHelper.isValidRepo(localRepo.path)) { throw IllegalArgumentException("Invalid repo $localRepo") } + } + fun update() { println("Hashing $localRepo...") val git = loadGit(localRepo.path) try { @@ -74,6 +76,11 @@ class RepoHasher(private val localRepo: LocalRepo, private val api: Api, // Confirm hashing completion. postRepoToServer() + + if (errors.isNotEmpty()) { + throw HashingException(errors) + } + println("Hashing $localRepo successfully finished.") } finally { diff --git a/src/main/kotlin/app/ui/UpdateRepoState.kt b/src/main/kotlin/app/ui/UpdateRepoState.kt index 58feeede..159da0c1 100644 --- a/src/main/kotlin/app/ui/UpdateRepoState.kt +++ b/src/main/kotlin/app/ui/UpdateRepoState.kt @@ -7,6 +7,7 @@ import app.hashers.RepoHasher import app.Logger import app.api.Api import app.config.Configurator +import app.utils.HashingException import app.utils.RequestException /** @@ -20,9 +21,12 @@ class UpdateRepoState constructor(private val context: Context, println("Hashing your git repositories.") for (repo in configurator.getLocalRepos()) { try { - RepoHasher(repo, api, configurator) - } catch (e: RequestException) { - Logger.error("Network error while hashing $repo", e) + RepoHasher(repo, api, configurator).update() + } catch (e: HashingException) { + Logger.error("During hashing ${e.errors.size} errors occurred:") + e.errors.forEach { error -> + Logger.error("", error) + } } catch (e: Exception) { Logger.error("Error while hashing $repo", e) } diff --git a/src/main/kotlin/app/utils/HashingException.kt b/src/main/kotlin/app/utils/HashingException.kt new file mode 100644 index 00000000..ea309a9f --- /dev/null +++ b/src/main/kotlin/app/utils/HashingException.kt @@ -0,0 +1,6 @@ +// Copyright 2017 Sourcerer Inc. All Rights Reserved. +// Author: Anatoly Kislov (anatoly@sourcerer.io) + +package app.utils + +class HashingException(val errors: List) : Exception()