Skip to content

Support non exception errors from fetcher #123

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.

Sign up for GitHub

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

Merged
eyalgu merged 25 commits into from
Apr 29, 2020
Merged

Support non exception errors from fetcher #123

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1c0da31
Support non exception errors from fetcher
eyalgu Feb 29, 2020
0075ada
revert parital changes to store builder to reduce noise
eyalgu Feb 29, 2020
2fe7b7e
finish off diff
eyalgu Mar 11, 2020
9e3801e
Merge remote-tracking branch 'origin/master' into errors
eyalgu Mar 11, 2020
d886e41
Allow to create a FetcherResult.Error without a Throwable. Add tests
eyalgu Mar 12, 2020
47463e6
Add missing funcion and more tests
eyalgu Mar 16, 2020
7279bdf
lint
eyalgu Mar 16, 2020
cae8faa
unflake RxFlowableStoreTest
eyalgu Mar 16, 2020
cea6e71
try to rename FakeFetcher to FakeRxFetcher to (maybe) solve missing c…
eyalgu Mar 16, 2020
2e1b012
Merge remote-tracking branch 'origin/master' into errors
eyalgu Mar 17, 2020
7bd21db
move SourceOfTruth out of impl package
eyalgu Mar 17, 2020
44a5e15
Merge remote-tracking branch 'origin/master' into errors
eyalgu Mar 18, 2020
03768e6
Rename accidental change of RxStoreBuilder.fromMaybe back to formSingle
eyalgu Mar 18, 2020
81548bb
Introduce Fetcher from #139
eyalgu Apr 21, 2020
3981d37
fix Rx artifact
eyalgu Apr 22, 2020
fb7c64d
Merge remote-tracking branch 'origin/master' into errors
eyalgu Apr 22, 2020
738ed50
delete legacy presistor factory
eyalgu Apr 22, 2020
1527bec
fix api file
eyalgu Apr 22, 2020
cb50d9c
move fetcher to be a typealias
eyalgu Apr 22, 2020
ed9d87f
code review comments + clean up documentation
eyalgu Apr 23, 2020
cff04b4
code review comments
eyalgu Apr 27, 2020
3251fbb
Update store/src/main/java/com/dropbox/android/external/store4/Fetche…
eyalgu Apr 28, 2020
5a2df2e
Merge remote-tracking branch 'origin/master' into errors
eyalgu Apr 29, 2020
a0edfcf
Revert "Update sample app's build.gradle to refer to the externally r…
eyalgu Apr 29, 2020
e883e5d
update releasing.md
eyalgu Apr 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ buildscript {
}

ext.versions = [
androidGradlePlugin : '4.0.0-alpha09',
androidGradlePlugin : '3.6.1',
kotlin : '1.3.61',
dokkaGradlePlugin : '0.10.0',
ktlintGradle : '9.1.1',
Expand Down
2 changes: 2 additions & 0 deletions store/src/main/java/com/dropbox/android/external/store4/StoreBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ interface StoreBuilder<Key : Any, Output : Any> {
*/
fun disableCache(): StoreBuilder<Key, Output>

fun<UnwrappedOutput: Any> fetcherTransformer(fetcherTransformer: (Output) -> FetcherResult<UnwrappedOutput>)
Copy link
Collaborator

@yigit yigit Feb 29, 2020

Choose a reason for hiding this comment

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

Would store response work here? Or is it too generic?

Copy link
Contributor Author

@eyalgu eyalgu Mar 1, 2020

Choose a reason for hiding this comment

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

Store response has an origin, which I don't want to let users override.


/**
* Connects a (non-[Flow]) source of truth that is accessible via [reader], [writer],
* [delete], and [deleteAll].
Expand Down
5 changes: 5 additions & 0 deletions store/src/main/java/com/dropbox/android/external/store4/StoreResponse.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,8 @@ enum class ResponseOrigin {
*/
Fetcher
}

sealed class FetcherResult<T> {
data class Data<T>(val value: T): FetcherResult<T>()
data class Error<T>(val error: Throwable): FetcherResult<T>()
}
17 changes: 12 additions & 5 deletions store/src/main/java/com/dropbox/android/external/store4/impl/FetcherController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.dropbox.android.external.store4.impl

import com.dropbox.android.external.store4.FetcherResult
import com.dropbox.android.external.store4.ResponseOrigin
import com.dropbox.android.external.store4.StoreResponse
import com.dropbox.flow.multicast.Multicaster
Expand Down Expand Up @@ -45,7 +46,7 @@ internal class FetcherController<Key, Input, Output>(
/**
* The function that provides the actualy fetcher flow when needed
*/
private val realFetcher: (Key) -> Flow<Input>,
private val realFetcher: (Key) -> Flow<FetcherResult<Input>>,
/**
* [SourceOfTruth] to send the data each time fetcher dispatches a value. Can be `null` if
* no [SourceOfTruth] is available.
Expand All @@ -64,10 +65,16 @@ internal class FetcherController<Key, Input, Output>(
scope = scope,
bufferSize = 0,
source = flow { emitAll(realFetcher(key)) }.map {
StoreResponse.Data(
it,
origin = ResponseOrigin.Fetcher
) as StoreResponse<Input>
when (it) {
is FetcherResult.Data -> StoreResponse.Data(
it.value,
origin = ResponseOrigin.Fetcher
) as StoreResponse<Input>
is FetcherResult.Error -> StoreResponse.Error(
it.error,
origin = ResponseOrigin.Fetcher
)
}
}.catch {
Copy link
Collaborator

@yigit yigit Feb 29, 2020

Choose a reason for hiding this comment

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

I think in this new world, it should crash the whole flow and hopefully the app

Copy link
Contributor Author

@eyalgu eyalgu Mar 1, 2020

Choose a reason for hiding this comment

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

This change is only intended to allow the use of a wider variety of fetchers. Why do you think we should change the behavior of Store? In general we don't propagate exceptions but concert then to errors, why would this change affect this?

Copy link
Collaborator

@yigit yigit Mar 22, 2020

Choose a reason for hiding this comment

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

because in the previous API, they can only return values hence we have to catch exceptions for errors.

In this new world, they get the ability to return errors so any exception can be considered exceptional. FYI this is also what paging does but we are considering options to maybe catch IO Exceptions by default to be more pragmatic.

A reason to opt into this new API would be to catch errors like NPE's and if we eat them, it would probably make people unhappy.

emit(StoreResponse.Error(it, origin = ResponseOrigin.Fetcher))
},
Expand Down
3 changes: 2 additions & 1 deletion store/src/main/java/com/dropbox/android/external/store4/impl/RealStore.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.dropbox.android.external.store4.impl
import com.dropbox.android.external.cache4.Cache
import com.dropbox.android.external.store4.CacheType
import com.dropbox.android.external.store4.ExperimentalStoreApi
import com.dropbox.android.external.store4.FetcherResult
import com.dropbox.android.external.store4.MemoryPolicy
import com.dropbox.android.external.store4.ResponseOrigin
import com.dropbox.android.external.store4.Store
Expand All @@ -41,7 +42,7 @@ import kotlinx.coroutines.flow.withIndex
@FlowPreview
internal class RealStore<Key : Any, Input : Any, Output : Any>(
scope: CoroutineScope,
fetcher: (Key) -> Flow<Input>,
fetcher: (Key) -> Flow<FetcherResult<Input>>,
sourceOfTruth: SourceOfTruth<Key, Input, Output>? = null,
private val memoryPolicy: MemoryPolicy?
) : Store<Key, Output> {
Expand Down