square / sqlbrite

A lightweight wrapper around SQLiteOpenHelper which introduces reactive stream semantics to SQL operations.
https://square.github.io/sqlbrite/3.x/sqlbrite/
Apache License 2.0
4.57k stars 416 forks source link

Expose BriteDatabase#getWriteableDatabase #161

Closed burntcookie90 closed 7 years ago

burntcookie90 commented 7 years ago

Expose the BriteDatabase's helper db via getWriteableDatabase.

Fixes #159

burntcookie90 commented 7 years ago

I thought about exposing getReadableDatabase also, since it's right there and maybe it could be useful? Or just leave it be.

JakeWharton commented 7 years ago

I don't see a reason not to also expose readable.

JakeWharton commented 7 years ago

Ok I have one last concern: we're exposing a cached instance that is never closed (and never should be) to the user. The documentation of this method explicitly says that you need to close the returned instance when you're done with it, but doing so will poison the instance we're using internally. I think if we're going to expose these, we need to make them pass-through factories to the underlying database and change the existing methods which cache the instance to be for internal use only.

AlecKazakova commented 7 years ago

What do you mean by "pass-through factories to the underlying database"

If you still get a SQLiteDatabase back at the end I'm okay with that

JakeWharton commented 7 years ago
public SQliteDatabase getWriteableDatabase() {
  return helper.getWritableDatabase();
}
burntcookie90 commented 7 years ago

That was my original (having not looked at the source) plan

JakeWharton commented 7 years ago

Yes let's do that instead. So rename the existing methods that do caching to something else and then expose these pass-through versions publicly.

On Thu, Dec 15, 2016 at 11:10 AM Vishnu Rajeevan notifications@github.com wrote:

That was my original (having not looked at the source) plan

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/square/sqlbrite/pull/161#issuecomment-267367557, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEESkIR2_lFJ_0Mb8yhh185MozP3COks5rIWaAgaJpZM4LM-87 .

AlecKazakova commented 7 years ago

That still suffers from the same problem though... SQLiteOpenHelper does it's own caching of a SQLiteDatabase instance and so closing the returned database will close our cached one.

We probably shouldn't have our own cached version at all - since SQLiteOpenHelper does it's own caching as long as the user never closes a SQLiteDatabase given to them the behavior is exactly the same, it uses a single cached version.

However, if the user does close a database, by calling getWritableDatabase or getReadableDatabase we're protecting ourselves

JakeWharton commented 7 years ago

Got it. That sounds good.

On Thu, Dec 15, 2016 at 11:15 AM Alec Strong notifications@github.com wrote:

That still suffers from the same problem though... SQLiteOpenHelper does it's own caching of a SQLiteDatabase instance and so closing the returned database will close our cached one.

We probably shouldn't have our own cached version at all - since SQLiteOpenHelper does it's own caching as long as the user never closes a SQLiteDatabase given to them the behavior is exactly the same, it uses a single cached version.

However, if the user does close a database, by calling getWritableDatabase or getReadableDatabase we're protecting ourselves

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/square/sqlbrite/pull/161#issuecomment-267369115, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEQxmQVNOgxNHCFQWu9CtTbt-Eyyhks5rIWe3gaJpZM4LM-87 .

burntcookie90 commented 7 years ago

Okie dokes, I'll take a whack at it this evening.

AlecKazakova commented 7 years ago

Siiiiiiiiiiick looks good :+1:

JakeWharton commented 7 years ago

I'll release later tonight or tomorrow.

burntcookie90 commented 7 years ago

Cool thanks