Allow custom operations in ClearDatabaseRule and avoid locale changes

[AhmedGamal92]

The purpose of this PR is to allow custom database operations to be passed from users of the library.

Also opening databases with NO_LOCALIZED_COLLATORS to avoid changing the db locale, which can cause exceptions like locked db while trying to execute the rule.

[Sloy]

Hello @AhmedGamal92! Thanks for contributing :)

I'm not sure I understood the issue you're mentioning about the locale and Barista rule. Maybe you could write a test reproducing this issue with Barista? That way we can understand your problem and make sure the fix is effective.

Thanks!

[AhmedGamal92]

Hello @Sloy, Thanks for checking :)

So with this PR I'm trying to do two issues

• First one is that the current implementation of the DatabaseOperations doesn't give the opportunity to customize how the operations are executed, so changed that to an interface so it can be overridden in case the default implementation is not valid for the case.
• Second one is about opening the database to clear using a locale, faced that in my project but I don't think I can write a test for that, the solution is to open the database ignoring locale as anyway the rule needs to clear the tables only nothing will be written so it should not update the locale, as the current way of opening the database will override the locale.

[rocboronat]

Hi @AhmedGamal92! One question... didn't you able to do that with the current approach? I mean, you could extend DatabaseOperations and that was it, right? Or maybe the class was internal so you didn't extend it?

I mean, I think we can do the same with less code 😄

And about the NO_LOCALIZED_COLLATORS thing, just to know, how can we reproduce the issue you had? Do you think we could write a test that reproduces that issue before fixing it? It's just to know how did it happen.

Thanks!

[AhmedGamal92]

Hi @rocboronat

didn't you able to do that with the current approach? I mean, you could extend DatabaseOperations and that was it, right? Or maybe the class was internal so you didn't extend it?

It's a final internal class so can't do that with current implementation. Maybe we can add default implementation for the interface as yes less code is better :D

And about the NO_LOCALIZED_COLLATORS thing, just to know, how can we reproduce the issue you had? Do you think we could write a test that reproduces that issue before fixing it? It's just to know how did it happen.

What happens is that I get db locked exceptions while trying to open the db to clear tables as opening the db without that flag will change the locale, and I don't think the clear database rule should do that, and the issue I had I think is some race condition where db is being created with adding locale while the rule tries to do the same.

[rocboronat]

And what about if we made it just not internal? @Sloy any preference about it?

Wow, about your issue, so strange. It would be nice to have a way to reproduce it 😢

[Sloy]

I guess we could make the class open so anyone could override whatever function they want, without having to implement all of them. We would keep it in the internal package, which indicates that breaking changes might occur if needed in the future, as it's not intended as a public API.

But I also would like to have some kind of reproduction of the issue, because right now I don't really understand it. We have never encountered any problems with our non-English locales, and I have never used the NO_LOCALIZED_COLLATORS flags.

Actually, the documentation for that flag is scary, especially the part were it says "You must be consistent when using this flag to use the setting the database was created with."

@AhmedGamal92 how can we reproduce your issue? Creating the database with a specific locale? Or inserting some special characters? Or setting some language in the device? If we know how to reproduce it manually maybe we can find a way to write a test for it.

[AhmedGamal92]

@Sloy @rocboronat
Thanks, I understand the point of being able to reproduce and write test, but unfortunately I was not able to do that, as I mentioned it looks like some race condition in my project but can not reproduce with tests or in smaller projects.

So for now I will update the PR to make the class open, and will let you know if I was able to reproduce that.

[Sloy]

Were you able to reproduce the issue in the sample @AhmedGamal92?

So sorry we went dark, reading the conversation again I think we can merge this PR as it is right now.

[AhmedGamal92]

Were you able to reproduce the issue in the sample @AhmedGamal92?

So sorry we went dark, reading the conversation again I think we can merge this PR as it is right now.

Hey @Sloy,
Yes as in the discussion I've changed the code here to just have an open class and functions to give the ability to override the default behaviour without needing to duplicate these classes.
And yes you can merge the PR 👍

[Sloy]

Thank you @AhmedGamal92! :)