Skip to content
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

Exclude Room metadata when clearing DB #343

Merged

Conversation

jmarsican
Copy link
Contributor

@jmarsican jmarsican commented May 5, 2020

  • Provide Room metadata table name and method to exclude it from DB clearing process
  • Implement test case: doesNotDeleteRoomMetadata
  • Replace deprecated method: InstrumentationRegistry.getTargetContext()

Room has become the most popular ORM library and it is officially supported by Google Android team. This library creates and populates automatically a table with important database metadata: identity hash of our implemented database schema. If this metadata is deleted it can cause unexpected behavior or even crashes (IllegalStateException).
For more info see:
Understanding migrations with Room
How does Room verify data integrity
Few people is aware of this, so it is good to provide built in logic to allow easy understand and usage of this rule when using Room

@Sloy
Copy link
Member

Sloy commented May 5, 2020

Hi @jmarsican! Thank you for your first contribution :)

When I saw your PR I was sure we were already excluding Room metadata, because we're using Room in production and we encountered the issues you mentioned. So I checked and, we're excluding it from our app! 😱 I really thought Barista was doing it by default, but nope.

So this change is more than welcome 😄

About your proposal, I would go even further. You're exposing a public function excludeRoomMetadataTable() that users can invoke. Nobody should want to delete the metadata file because it leads to issues, so let's make Barista smart by default. What do you think about always skipping that table automatically instead?

@jmarsican
Copy link
Contributor Author

Hey @Sloy I'm glad this a useful contribution. Thank you for your feedback and suggestions. I've just applied your proposal and pushed new commit.

@jmarsican
Copy link
Contributor Author

@Sloy please let me know if you want me to apply something else ;)

Copy link
Member

@Sloy Sloy left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! The updated PR looks perfect, thank you very much :)

@Sloy Sloy requested a review from a team May 7, 2020 07:05
@Sloy Sloy merged commit 7e7df53 into AdevintaSpain:master May 7, 2020
@rocboronat
Copy link
Member

Cool! 👏 Thank you so much, @jmarsican!

@Sloy
Copy link
Member

Sloy commented May 7, 2020

You should have this feature now available in the 3.5.0 release 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants