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.

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

Interested in adding "create/update/delete" behaviors to the store? #212

Open
lmckenzie opened this Issue May 17, 2017 · 14 comments

Comments

Projects
None yet
3 participants
@lmckenzie

lmckenzie commented May 17, 2017

I have been using the repository pattern in many Android apps and I really like it. I am very excited to find this library and I am considering updating my various implementations of the pattern to use this library. One issue or rather difference I ran into was the lack of "create/update/delete" behaviors. Is this something you would be interested in?

@digitalbuddha

This comment has been minimized.

Contributor

digitalbuddha commented May 17, 2017

Create/update should only happen through a value coming from your fetcher. As for delete there is already a clear functionality. Not sure what you are asking to add?

@lmckenzie

This comment has been minimized.

lmckenzie commented May 17, 2017

I was thinking about it a little differently than that. The behaviors of "create/update/delete" apply when the client modifies data and needs to update the backing data source. Create would tell the "creator" to create a new record with the data provided. Creator implementation could be a POST request to a web service for example. Allowing operations like:

MyData data = new MyData(...)
myStore.create(data)
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(this::showPosts, throwable -> {
                });

This same idea applies to update or delete.

@digitalbuddha

This comment has been minimized.

Contributor

digitalbuddha commented May 17, 2017

Oh I see what you mean. Thats a nice way to do it. Safe to assume create/update/delete would not cache or work offline or did you have some idea of how to queue requests?

@lmckenzie

This comment has been minimized.

lmckenzie commented May 17, 2017

I was thinking you would have the optional to update the cache immediately and then clean if there was a failure.

@lmckenzie

This comment has been minimized.

lmckenzie commented May 17, 2017

If offline the request would fail.

@digitalbuddha

This comment has been minimized.

Contributor

digitalbuddha commented May 17, 2017

How does updating cache work? If I make same post request again do I get cached response or does it hit server? I don't have a clear mental model of how a post could be cacheable

@lmckenzie

This comment has been minimized.

lmckenzie commented May 17, 2017

In the context of create/update/delete operations by cache I mean the same stores cache used for "get()". Let's say I have a Store for comments, I "create" a comment. The memory and/or disk cache is updated immediately with this new record. Then the creator attempts to create the record. When the creator finishes success/failure the cache is updated again.

The use case would be I post a comment and then rotate my device. This calls "get" again and if the new comment is not cached then it would appear to the user that the comment did not post.

@lmckenzie

This comment has been minimized.

lmckenzie commented May 17, 2017

Just a thought... As far as offline goes it could be another optional object that allows the implementer to define the behavior. If it is added to the store then any fetcher/creator/updater/deleter (not attached to the names) that has an error due to connectivity would call the offline-Inator to attempt to handle when connectivity is restored.

@digitalbuddha

This comment has been minimized.

Contributor

digitalbuddha commented May 17, 2017

Thank you for explaining. I believe that would be a reasonable implementation. As long as the eager update is configurable/optional I agree with your general flow. Most of our team is at io until next week. I'd love to hear others thoughts on how this should work prior to starting on implementation. Mind tabling for few days?

@digitalbuddha

This comment has been minimized.

Contributor

digitalbuddha commented May 17, 2017

For the offline stuff, I'd prefer to rely on job scheduler or some other abstraction external to the store. I think keeping queues of what to post will get messy. Particularly if a user wants multiple posts to be transactional.

@lmckenzie

This comment has been minimized.

lmckenzie commented May 17, 2017

I do not mind at all. I will be busy with I/O myself. I just wanted to get the conversation started.

@Exerosis

This comment has been minimized.

Exerosis commented Aug 19, 2017

Don't mean to be pushy, but any update here? This is a beautiful idea! I foresee some problems, but I'm still relatively new to Android so these might be obvious. Take this example, a user finishes typing up a comment and adds it, I call store.set(comments.add(comment)) or something like that. This immediately triggers my getRefreshing() and my layout is updated with the new comment. Then the poster/updater/creator either fails or makes the post and getRefreshing() is triggered again fixing the mistake or ensuring that the comment is posted. As much as it sounds great, it also makes it impossible to tell the state of the data returned by store.get(), it could be cached data that's about to fail to be posted or data freshly gathered from the server. Which would make it difficult to add a 'sending' mark on comments not yet posted. It also would mean each use of get() will require additional implementation to handle the potential for not yet posted comments. The obvious fix seems to be returning a cacher type with the data returned by get(), Cacher.POSTER, Cacher.FETCHER or something which seems very messy. The other slight issue is simply the waste of network usage caused by posting the entire block of comments back to the server with a single change, the desired effect would be posting a single comment and then updating the cache/disk instead which seems like it would make the problem highlighted above even worse :( A proposed fix would be a type of store that specializes in Lists, so instead of returning a single Parsed type a Collection would instead be returned, making it a little simpler to add to. That said, I'm sure your team will figure something out, there are certainly no more qualified individuals! Keep up the amazing work.

@digitalbuddha

This comment has been minimized.

Contributor

digitalbuddha commented Aug 19, 2017

Thank you for the kind words! The problem is that we don't need this feature internally and as a result don't currently have the time to allocate to implementing. For at least the foreseeable future this is not something we will be adding. We did make the store highly extensible and based on interfaces, if someone wants to make a Poster that goes along with Store we would be able to open up any additional apis that would be needed to make it update a Store.

@Exerosis

This comment has been minimized.

Exerosis commented Aug 19, 2017

Ah I understand, perhaps I will take a stab at it! Thanks for getting back to me so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment