stream-chat-android icon indicating copy to clipboard operation
stream-chat-android copied to clipboard

[ISSUE-4109] Using Repositories implementations instead of facade

Open leandroBorgesFerreira opened this issue 2 years ago • 4 comments

🎯 Goal

Using implementation of repositories instead of RepositoryFacade.

🧪 Testing

Test the persistence of the SDK

☑️Contributor Checklist

General

  • [x] I have signed the Stream CLA (required)
  • [x] Assigned a person / code owner group (required)
  • [ ] Thread with the PR link started in a respective Slack channel (#android-chat-core or #android-chat-ui) (required)
  • [x] PR targets the develop branch
  • [x] PR is linked to the GitHub issue it resolves

Code & documentation

  • ~[ ] Changelog is updated with client-facing changes~
  • ~[ ] New code is covered by unit tests~
  • ~[ ] Comparison screenshots added for visual changes~
  • ~[ ] Affected documentation updated (KDocs, docusaurus, tutorial)~

☑️Reviewer Checklist

  • [ ] UI Components sample runs & works
  • [ ] Compose sample runs & works
  • [ ] UI Changes correct (before & after images)
  • [ ] Bugs validated (bugfixes)
  • [ ] New feature tested and works
  • [ ] Release notes and docs clearly describe changes
  • [ ] All code we touched has new or updated KDocs

🎉 GIF

giphy (1) A facade

leandroBorgesFerreira avatar Sep 01 '22 19:09 leandroBorgesFerreira

Hi @JcMinarro.

One problem that we have with the facade is that we use some overrides in the original implementations that cause duplication when using the interfaces. For example:

userRepository.insertUsers(messageToBeDeleted.users())
messageRepository.insertMessage(messageToBeDeleted, true)

This code would call insert user twice because of the implementation of insertMessage of RepositoryFacade:

override suspend fun insertMessage(message: Message, cache: Boolean) {
        insertUsers(message.users())
        messageRepository.insertMessage(message, cache)
}

Using RepositoryFacade brings this problem with it. Another solution, instead of using the implementations inside the facade would be to erase this method and make sure the other classes are not having this problem (pehaps clean all override), but it is a bigger change.

What do you think?

leandroBorgesFerreira avatar Sep 02 '22 13:09 leandroBorgesFerreira

Hi @JcMinarro.

One problem that we have with the facade is that we use some overrides in the original implementations that cause duplication when using the interfaces. For example:

userRepository.insertUsers(messageToBeDeleted.users())
messageRepository.insertMessage(messageToBeDeleted, true)

This code would call insert user twice because of the implementation of insertMessage of RepositoryFacade:

override suspend fun insertMessage(message: Message, cache: Boolean) {
        insertUsers(message.users())
        messageRepository.insertMessage(message, cache)
}

Using RepositoryFacade brings this problem with it. Another solution, instead of using the implementations inside the facade would be to erase this method and make sure the other classes are not having this problem (pehaps clean all override), but it is a bigger change.

What do you think?

Just that is the case. Whenever a channel/message is added, we need to add all related users with this entity. If we forgot to insert them in some place, our SDK could crash when the entity is recovered (And indeed, it was crashing in the past until we implemented the facade and handled this case)

If someone inserts twice the same user, it doesn't create a duplicate. The code you posted is totally fine.

JcMinarro avatar Sep 02 '22 14:09 JcMinarro

Hey @JcMinarro 👋

The main issue is that we're doing unnecessary IO operations, at least 2 times more than it actually requires 🤷‍♂️ So I suppose by duplicate @leandroBorgesFerreira meant not the entities but IO operations.

kanat avatar Sep 07 '22 06:09 kanat

Hey @JcMinarro wave

The main issue is that we're doing unnecessary IO operations, at least 2 times more than it actually requires man_shrugging So I suppose by duplicate @leandroBorgesFerreira meant not the entities but IO operations.

Those unnecessary IO operations can be avoided by removing the insertUsers() call from outside of the repositoryFacade invocation, because they are not needed. But we can't allow other users to insert corrupt data to our DB

JcMinarro avatar Sep 14 '22 08:09 JcMinarro