Support non exception errors from fetcher #123

eyalgu · 2020-02-29T23:27:01Z

Non-working PR to show the proposed way of adding support for allowing the fetcher to communicate errors not via exceptions.

The main complication here is in the builder API. we already have 2 builders because we support a builder with and without source of truth (to change to generic type). If we went this way we would either need to support 4 builder or we will need to change the builder API to force providing the source of truth (and the new translator) in the builder's constructor

eyalgu · 2020-02-29T23:29:58Z

store/src/main/java/com/dropbox/android/external/store4/StoreBuilder.kt

     */
    fun disableCache(): StoreBuilder<Key, Output>

+    fun<UnwrappedOutput> fetcherTransformer(fetcherTransformer: (Output) -> FetcherResult<UnwrappedOutput>)


Ignore anything in this file aside from this line. Everything bellow is a mess that I didn't want to get into before we decide this is the way to go.

I think the only scalable way to add support to another generic type changing parameter (like source of truth) would be for change the builder API it only receive the source of truth in the constructor.

Aside from builder changes (and tests) all the other changes needed to support this change are in the PR (and are pretty minimal)

yigit

Just a quick look from the phone.

yigit · 2020-02-29T23:41:28Z

store/src/main/java/com/dropbox/android/external/store4/impl/FetcherController.kt

+                            origin = ResponseOrigin.Fetcher
+                        )
+                    }
                }.catch {


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

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?

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.

yigit · 2020-02-29T23:42:11Z

store/src/main/java/com/dropbox/android/external/store4/StoreBuilder.kt

     */
    fun disableCache(): StoreBuilder<Key, Output>

+    fun<UnwrappedOutput: Any> fetcherTransformer(fetcherTransformer: (Output) -> FetcherResult<UnwrappedOutput>)


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

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

eyalgu · 2020-03-11T06:15:42Z

Sorry this was open for so long. finally got around to finish it. Note that this would be a major API change for how we build store (but no change to Store's API itself).

eyalgu · 2020-03-11T06:18:05Z

the tl;dr of the api change is that you have to pass a source of truth when you create a builder rather than later:

        StoreBuilder
            .fromNonFlow<Pair<String, RedditConfig>, List<Post>, List<Post>>(
                fetcher = { (query, config) ->
                    provideRetrofit().fetchSubreddit(query, config.limit)
                        .data.children.map(::toPosts)
                },
                sourceOfTruth = SourceOfTruth.from(
                    reader = { (query, _) -> db.postDao().loadPosts(query) },
                    writer = { (query, _), posts -> db.postDao().insertPosts(query, posts) },
                    delete = { (query, _) -> db.postDao().clearFeedBySubredditName(query) },
                    deleteAll = db.postDao()::clearAllFeeds
                )
            )
            .build()

codecov · 2020-03-11T14:39:19Z

Codecov Report

❗ No coverage uploaded for pull request base (master@fc8da86). Click here to learn what that means.
The diff coverage is 64.92%.

@@            Coverage Diff            @@
##             master     #123   +/-   ##
=========================================
  Coverage          ?   83.04%           
  Complexity        ?      229           
=========================================
  Files             ?       48           
  Lines             ?      973           
  Branches          ?      145           
=========================================
  Hits              ?      808           
  Misses            ?      109           
  Partials          ?       56

Impacted Files	Coverage Δ	Complexity Δ
...2/src/main/kotlin/com/dropbox/store/rx2/RxStore.kt	`100.00% <ø> (ø)`	`0.00 <0.00> (?)`
.../dropbox/android/external/store4/impl/RealStore.kt	`90.90% <ø> (ø)`	`14.00 <0.00> (?)`
...d/external/store4/impl/SourceOfTruthWithBarrier.kt	`93.54% <ø> (ø)`	`6.00 <0.00> (?)`
...ain/kotlin/com/dropbox/store/rx2/RxStoreBuilder.kt	`16.66% <16.66%> (ø)`	`0.00 <0.00> (?)`
...m/dropbox/android/external/store4/SourceOfTruth.kt	`65.21% <65.21%> (ø)`	`0.00 <0.00> (?)`
...in/kotlin/com/dropbox/store/rx2/RxSourceOfTruth.kt	`70.00% <70.00%> (ø)`	`0.00 <0.00> (?)`
...om/dropbox/android/external/store4/StoreBuilder.kt	`75.00% <72.22%> (ø)`	`0.00 <0.00> (?)`
...m/dropbox/android/external/store4/StoreResponse.kt	`91.42% <87.50%> (ø)`	`12.00 <3.00> (?)`
.../android/external/store4/impl/FetcherController.kt	`100.00% <100.00%> (ø)`	`6.00 <0.00> (?)`
.../android/external/store4/impl/RealSourceOfTruth.kt	`66.66% <100.00%> (ø)`	`0.00 <0.00> (?)`
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc8da86...e883e5d. Read the comment docs.

codecov · 2020-03-11T14:39:20Z

Codecov Report

Merging #123 into master will decrease coverage by 0.70%.
The diff coverage is 60.00%.

@@             Coverage Diff              @@
##             master     #123      +/-   ##
============================================
- Coverage     84.19%   83.49%   -0.71%     
  Complexity      242      242              
============================================
  Files            45       45              
  Lines           949      957       +8     
  Branches        151      147       -4     
============================================
  Hits            799      799              
- Misses           95      104       +9     
+ Partials         55       54       -1

Impacted Files	Coverage Δ	Complexity Δ
.../android/external/store4/impl/FetcherController.kt	`100.00% <ø> (ø)`	`6.00 <0.00> (ø)`
.../dropbox/android/external/store4/impl/RealStore.kt	`90.90% <ø> (ø)`	`14.00 <0.00> (ø)`
...m/dropbox/android/external/store4/StoreResponse.kt	`95.00% <50.00%> (-5.00%)`	`13.00 <0.00> (ø)`
...om/dropbox/android/external/store4/StoreBuilder.kt	`59.61% <52.77%> (-6.59%)`	`0.00 <0.00> (ø)`
...pbox/android/external/store4/impl/SourceOfTruth.kt	`64.10% <65.21%> (+1.60%)`	`0.00 <0.00> (ø)`
...2/src/main/kotlin/com/dropbox/store/rx2/RxStore.kt	`72.41% <100.00%> (-5.37%)`	`0.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38baeb7...9e3801e. Read the comment docs.

eyalgu · 2020-03-11T14:42:11Z

(still need to add a couple of tests here, will add soon hopefully)

…odcov

eyalgu · 2020-03-18T19:13:12Z

@yigit @digitalbuddha this is now ready for review - it looks bigger than it is because of the changes to the builder's interface (and exposing sourceOfTruth through the interface)

yigit · 2020-03-22T01:14:18Z

Sorry i didn't have time to check this, planning to do it now.
i did a quick through and i have a question.

Does it make sense to also make the fetcher a class instead of a function?

That could help w/ two things:
a) we can have future changes there w/o going into this builder API issues
b) different ways to create a Fetcher would be isolated in the Fetcher itself, instead of leaking into the store builder API.

I just wanted to ask in case you've already evaluated that option.

yigit

i'm not rally super happy with this change. i think it creates a lot of API load.

I'll try to prototype the Fetcher as a class idea where we can make it something that always retturns value or error and then provide a wrapper that returns the value type which we will catch exceptions.
I would like to know if you've already tried that though.

yigit · 2020-03-22T23:28:03Z

app/src/main/java/com/dropbox/android/sample/Graph.kt

        val adapter = moshi.adapter<RedditConfig>(RedditConfig::class.java)
        return StoreBuilder
-            .fromNonFlow<Unit, RedditConfig> {
+            .fromNonFlow<Unit, RedditConfig, RedditConfig> (fetcher = {


this was something we tried to avoid in the new store builder. is it required now?

yigit · 2020-03-22T23:29:45Z

build.gradle


    ext.versions = [
-            androidGradlePlugin         : '4.0.0-beta01',
+            androidGradlePlugin         : '3.6.0',


not important but probably better to use 3.6.1 ?

store/src/main/java/com/dropbox/android/external/store4/SourceOfTruth.kt

yigit · 2020-03-22T23:37:49Z

store/src/main/java/com/dropbox/android/external/store4/SourceOfTruth.kt

+         * WARNING: Delete operation is not supported when using a legacy [com.dropbox.android.external.store4.Persister],
+         * please use another override
+         */
+        fun <Key, Output> fromLegacyPresister(


should we mark this deprecated?

or delete it, we're still in alpha

yigit · 2020-03-22T23:40:15Z

store/src/main/java/com/dropbox/android/external/store4/StoreBuilder.kt

+         */
+        fun <Key : Any, RawOutput : Any, Output : Any> fromNonFlow(
+            fetcher: suspend (key: Key) -> RawOutput,
+            fetcherTransformer: (RawOutput) -> FetcherResult<Output>


this is a bit weird., why cant it just provide a FetcherResponse? Name conflict?

yigit · 2020-03-22T23:41:21Z

store/src/main/java/com/dropbox/android/external/store4/StoreBuilder.kt

+        /**
+         * Creates a new [StoreBuilder] from a [Flow] fetcher.
+         *
+         * Use when creating a [Store] that fetches objects in an websocket-like multiple responses


websocket seems weird example as people may not be able to relate to it (i might be wrong). Maybe we can use firebase as an example instead?

yigit · 2020-03-22T23:43:53Z

store/src/main/java/com/dropbox/android/external/store4/impl/FetcherController.kt

+                            origin = ResponseOrigin.Fetcher
+                        )
+                    }
                }.catch {


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.

yigit · 2020-03-23T01:36:14Z

created #139 to make discussions easier. unlike this one, it is not a complete PR.

eyalgu · 2020-03-30T15:04:53Z

Sorry for dropping off here. It's been hard to find OSS time in the last few weeks with the shelter in place and all. #139 seems good to me. I put a comment around the fetcher's API.

I'll try to integrate those changes into this revision over the next week of so, but feel free to finish up #139 you feel like it and if I'm taking too long.

store/src/main/java/com/dropbox/android/external/store4/impl/FetcherController.kt

store/src/main/java/com/dropbox/android/external/store4/impl/RealSourceOfTruth.kt

yigit

i think overall is good, thank you. just small comments.

app/src/main/java/com/dropbox/android/sample/Graph.kt

store-rx2/src/main/kotlin/com/dropbox/store/rx2/RxSourceOfTruth.kt

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt

store/src/main/java/com/dropbox/android/external/store4/StoreResponse.kt

store/src/main/java/com/dropbox/android/external/store4/impl/FetcherController.kt

store/src/test/java/com/dropbox/android/external/store3/StoreTest.kt

store/src/test/java/com/dropbox/android/external/store4/impl/TransformingFetcherTest.kt

eyalgu · 2020-04-23T16:43:00Z

@yigit Thanks for all the great feedback. This should be ready to go now.

yigit

lgtm, just small nits.
thanks!

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt

store/src/main/java/com/dropbox/android/external/store4/SourceOfTruth.kt

yigit · 2020-04-27T00:00:58Z

store/src/main/java/com/dropbox/android/external/store4/SourceOfTruth.kt

+
+ */
+interface SourceOfTruth<Key, Input, Output> {
+    val defaultOrigin: ResponseOrigin


Suggested change

val defaultOrigin: ResponseOrigin

/**

* The data origin value which will be reported in [StoreResponse.origin]. For instance, if the SourceOfTruth keeps

* the data on disk, this value should be [ResponseOrigin.Persister].

*/

val defaultOrigin: ResponseOrigin

store/src/main/java/com/dropbox/android/external/store4/SourceOfTruth.kt

store/src/main/java/com/dropbox/android/external/store4/StoreResponse.kt

yigit · 2020-04-27T00:05:19Z

store/src/main/java/com/dropbox/android/external/store4/impl/RealSourceOfTruth.kt

+    override val defaultOrigin =
+        ResponseOrigin.Persister


Suggested change

override val defaultOrigin =

ResponseOrigin.Persister

override val defaultOrigin = ResponseOrigin.Persister

yigit · 2020-04-27T00:05:34Z

store/src/main/java/com/dropbox/android/external/store4/impl/RealSourceOfTruth.kt

+    override val defaultOrigin =
+        ResponseOrigin.Persister


Suggested change

override val defaultOrigin =

ResponseOrigin.Persister

override val defaultOrigin = ResponseOrigin.Persister

yigit

just a small nit, lgtm; Thanks!

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt

…r.kt Co-Authored-By: Yigit Boyar <[email protected]>

…eleased version of Store (#159)" This reverts commit fc8da86.

eyalgu · 2020-04-29T17:04:03Z

reverting #159 for now because it makes it impossible to update both the source and the sample in the same diff. Let's discuss bringing it back in an issue. there are pros and cons for it, but for now, while the API is unstable it's just to block us. FYI @DYJParker

DYJParker · 2020-04-29T17:09:11Z

Just tagging @digitalbuddha in this, since he drove the change.

reverting #159 for now because it makes it impossible to update both the source and the sample in the same diff. Let's discuss bringing it back in an issue. there are pros and cons for it, but for now, while the API is unstable it's just to block us. FYI @DYJParker

eyalgu · 2020-04-29T17:11:59Z

opened #161 to continue the discussion over #159

* Support non exception errors from fetcher * revert parital changes to store builder to reduce noise * finish off diff * Allow to create a FetcherResult.Error without a Throwable. Add tests * Add missing funcion and more tests * lint * unflake RxFlowableStoreTest * try to rename FakeFetcher to FakeRxFetcher to (maybe) solve missing codcov * move SourceOfTruth out of impl package * Rename accidental change of RxStoreBuilder.fromMaybe back to formSingle * Introduce Fetcher from MobileNativeFoundation#139 * fix Rx artifact * delete legacy presistor factory * fix api file * move fetcher to be a typealias * code review comments + clean up documentation * code review comments * Update store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt Co-Authored-By: Yigit Boyar <[email protected]> * Revert "Update sample app's build.gradle to refer to the externally released version of Store (MobileNativeFoundation#159)" This reverts commit fc8da86. * update releasing.md Co-authored-by: Yigit Boyar <[email protected]>

Support non exception errors from fetcher

1c0da31

eyalgu requested a review from yigit February 29, 2020 23:27

eyalgu commented Feb 29, 2020

View reviewed changes

eyalgu mentioned this pull request Feb 29, 2020

Handle fetchers that don't throw an exception #82

Closed

revert parital changes to store builder to reduce noise

0075ada

yigit reviewed Feb 29, 2020

View reviewed changes

finish off diff

2fe7b7e

eyalgu requested review from digitalbuddha and yigit March 11, 2020 06:13

Merge remote-tracking branch 'origin/master' into errors

9e3801e

eyalgu added 9 commits March 11, 2020 21:42

Allow to create a FetcherResult.Error without a Throwable. Add tests

d886e41

Add missing funcion and more tests

47463e6

lint

7279bdf

unflake RxFlowableStoreTest

cae8faa

try to rename FakeFetcher to FakeRxFetcher to (maybe) solve missing c…

cea6e71

…odcov

Merge remote-tracking branch 'origin/master' into errors

2e1b012

move SourceOfTruth out of impl package

7bd21db

Merge remote-tracking branch 'origin/master' into errors

44a5e15

Rename accidental change of RxStoreBuilder.fromMaybe back to formSingle

03768e6

yigit reviewed Mar 22, 2020

View reviewed changes

Introduce Fetcher from #139

81548bb

eyalgu commented Apr 21, 2020

View reviewed changes

store/src/main/java/com/dropbox/android/external/store4/impl/FetcherController.kt Outdated Show resolved Hide resolved

store/src/main/java/com/dropbox/android/external/store4/impl/RealSourceOfTruth.kt Show resolved Hide resolved

eyalgu added 5 commits April 21, 2020 21:47

fix Rx artifact

3981d37

Merge remote-tracking branch 'origin/master' into errors

fb7c64d

delete legacy presistor factory

738ed50

fix api file

1527bec

move fetcher to be a typealias

cb50d9c

yigit reviewed Apr 23, 2020

View reviewed changes

code review comments + clean up documentation

ed9d87f

eyalgu requested a review from yigit April 23, 2020 16:43

yigit reviewed Apr 27, 2020

View reviewed changes

code review comments

cff04b4

yigit previously approved these changes Apr 28, 2020

View reviewed changes

store/src/main/java/com/dropbox/android/external/store4/Fetcher.kt Outdated Show resolved Hide resolved

Update store/src/main/java/com/dropbox/android/external/store4/Fetche…

3251fbb

…r.kt Co-Authored-By: Yigit Boyar <[email protected]>

eyalgu dismissed yigit’s stale review via 3251fbb April 28, 2020 19:11

yigit previously approved these changes Apr 29, 2020

View reviewed changes

eyalgu added 2 commits April 29, 2020 09:54

Merge remote-tracking branch 'origin/master' into errors

5a2df2e

Revert "Update sample app's build.gradle to refer to the externally r…

a0edfcf

…eleased version of Store (#159)" This reverts commit fc8da86.

eyalgu dismissed yigit’s stale review via a0edfcf April 29, 2020 17:01

eyalgu mentioned this pull request Apr 29, 2020

When should we have the sample app point to released artifacts? #161

Closed

eyalgu linked an issue Apr 29, 2020 that may be closed by this pull request

When should we have the sample app point to released artifacts? #161

Closed

update releasing.md

e883e5d

digitalbuddha approved these changes Apr 29, 2020

View reviewed changes

eyalgu merged commit e71a4e9 into master Apr 29, 2020

eyalgu linked an issue Apr 29, 2020 that may be closed by this pull request

Handle fetchers that don't throw an exception #82

Closed

eyalgu deleted the errors branch April 29, 2020 18:49

	override val defaultOrigin =
	ResponseOrigin.Persister
	override val defaultOrigin = ResponseOrigin.Persister

Support non exception errors from fetcher #123

Support non exception errors from fetcher #123

Uh oh!

Conversation

eyalgu commented Feb 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yigit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalgu commented Mar 11, 2020

Uh oh!

eyalgu commented Mar 11, 2020

Uh oh!

codecov bot commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented Mar 11, 2020

Codecov Report

Uh oh!

eyalgu commented Mar 11, 2020

Uh oh!

eyalgu commented Mar 18, 2020

Uh oh!

yigit commented Mar 22, 2020

Uh oh!

yigit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yigit commented Mar 23, 2020

Uh oh!

eyalgu commented Mar 30, 2020

Uh oh!

Uh oh!

Uh oh!

yigit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eyalgu commented Apr 23, 2020

Uh oh!

yigit left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 11, 2020 •

edited

Loading

eyalgu commented Apr 29, 2020 •

edited

Loading