zendesk / android-db-commons

Some common utilities for ContentProvider/ContentResolver/Cursor and other db-related android stuff
Apache License 2.0
222 stars 28 forks source link

Initial implementation of converting to ContentProviderOperation #24

Closed henieek closed 10 years ago

henieek commented 10 years ago

ATTENTION: This PR is only an overview, it's not meant to be merged to master yet.

Hello,

This pull-request contains initial, straightforward implementation of converting db-commons' ProviderActions to ContentProviderOperations. The biggest advantage of this mechanism is that we are able to pass ProviderActions as arguments, so receiver can use them both in atomic ContentProvider actions as well as in the applyBatch.

Missing things:

assert query should be very easy in implementation. I got a couple of ideas to implement back-refs:

henieek commented 10 years ago

Or just smth like this:

ProviderAction.insert(SOME_URI)
  .value("bleh", 2)
  .value("blah", "huh")
  .withValueBackReference("ref_id", 5)
  .toContentProviderOperations();

From .withValueBackReference we receive a reference to an interface that does not expose perform() method, so after invoking it we can use it only for applyBatch purposes.

chalup commented 10 years ago

I never really liked the way you have to specify the index of the operation for the backref. I think the better syntax would be:

Insert someObj  = ProviderAction.insert(SOME_URI).values(/*...*/);
BatchInsert otherObj = ProviderAction.insert(OTHER_URI).values(/*...*/).withValueBackReference("some_id", someObj);

ProviderAction
  .batch(someObj, otherObj)
  .batch(/* more operations */)
  .perform(crudHandler);

Comments?

henieek commented 10 years ago

@chalup updated and cleaned. Check ConvertToContentProviderOperationTest.java for usage/API. If it's okay I'll add JavaDocs so we can merge it for further release.

henieek commented 10 years ago

Crap, one thing, how do we want to deal with such situation?

Insert insert = ProviderAction.insert(uri);
Batcher.begin()
  .append(insert)
  .append(insert)
  .appendWithBackRefs(someUpdate).forPrevious(insert);
// ...

ContentProvider doesn't guarantee that the id returned for same ContentProviderOperation will be same. (it's unlikely even, e.g. autoincrement). What we can do is:

chalup commented 10 years ago

Regarding using the same insert twice: it's über edge case, I'd throw an exception.

Regarding the API: I think we can do better. I do not like the batch() call to finish the back references definition.

I'm also not sure how the back references work with Update/Delete operations.

chalup commented 10 years ago

One more thing: how about changing append(X).appendWithBackRef(Y).forPrevious(X, X_ID) to append(X).append(Y).withValueBackRef(X, X_ID)? IMO clearer API.

henieek commented 10 years ago

Agreed! Now it's noticeably better.

In addition I did some minor git commits cleanup and:

Batcher.begin()
 .append(firstInsert)
 .append(secondInsert)
 .append(smallInsert1, smallInsert2, smallInsert3)
     .withValueBackReference(firstInsert, "blah")
     .withValueBackReference(secondInsert, "blah3")
 .operations()

(specified in test added in c98c850)

chalup commented 10 years ago

Seal of approval