v2.0 API discussion points

[tomblench]

Issue to track discussion points where we don't yet have a clear answer.

We should strike these off and/or open separate issues as and when we resolve what the next steps are for each.

• should DocumentStore query/database be methods instead of fields we have resolved that the will be methods - this has been merged
• naming proposal for Database methods (create, delete) instead of createDocumentFromRevision we have agreed on a set of names which are based on CRUD - this has been merged
• Database is in documentstore package, but Query isn’t but they are peers we have decided to keep this as-is
• double check API classes and decide if anything should not be public done, see https://github.com/cloudant/sync-android/issues/413#issuecomment-275451057
• argument checks currently mostly throw IllegalArgumentException except checkNotNull which throws a NullPointerException should we always throw an IllegalArgumentException? Perhaps we should consider a checked Exception type? done, see https://github.com/cloudant/sync-android/issues/413#issuecomment-275680032

[rhyshort]

With my arbitrary comparison

documentStore.database.create(revision);
documentStore.query.find(selector);

documentStore.database().create(revision);
documentStore.query().find(selector);

And after seeing our Mocks removed in #412, I think having database and query as methods (without the get prefix) is a nice compromise and allows us to do mocking without doing potential unsafe things to final fields via reflection.

So my vote is for .database() and .query().

[ricellis]

Should discuss whether these should be public:

• [x] com.cloudant.sync.documentstore.DocumentBodyFactory#EMPTY -> should be DocumentBody not DocumentBodyImpl - PR #492
• [x] com.cloudant.sync.documentstore.Changes#Changes (marked API private), but is in the public package - PR #495
• [x] com.cloudant.sync.documentstore.Changes#getResults returns a list of InternalDocumentRevision - PR #495
• [x] Naming of Id vs ID in many places - the Java convention is to use Id even though it isn't consistently applied in the JVM.
• [x] com.cloudant.sync.documentstore.DocumentRevision#isCurrent - I think this is probably needed, but might be worth a conversation. On investigation/discussion this is not needed and should be made internal - PR #496
• [x] com.cloudant.sync.documentstore.encryption package I think only KeyProvider and SimpleKeyProvider need be public? - having looked at this again it appears that all these encryption classes are OK as API.
• [x] com.cloudant.sync.query.IndexType#enumValue is this needed for case insensitivity? All enum types have a valueOf(String name) method built-in. - PR #494
• [x] com.cloudant.sync.replication.IntervalTimerReplicationPolicyManager should this move into JavaSE as it says it is not suitable for use on Android? - PR #493
• [x] com.cloudant.sync.replication.ReplicatorBuilder#withId if this is only needed internally by replication policies it should probably not be exposed on the public builder. See https://github.com/cloudant/sync-android/issues/413#issuecomment-276095152 and PR #498

[ricellis]

Added branch to change the NPE to IllegalArgumentException https://github.com/cloudant/sync-android/tree/413-npe-iae

[ricellis]

Other changes in branch https://github.com/cloudant/sync-android/tree/413-api-cleanup

[brynh]

I think I agree with most of the points except that com.cloudant.sync.replication.ReplicatorBuilder#withId is used in the setup of a replication policy by a user, so I think we need to keep it in the public builder. Its use is described in the replication policies guide

[ricellis]

@brynh thanks for the feedback. Regarding ReplicatorBuilder.withId() after further consideration I think it makes sense to leave it in place, since the alternative (for example passing the Replicator instance itself into the listener) requires exposing the Replicator which is much harder to do statically, and that makes the comparisons more difficult.

When looking at it however, I did notice a couple of other API issues in the replication policies:

• [x] The ReplicationPolicyManager.ReplicationListener is public, but only because the EventBus needs access to it. I think it should move into the internal package.
• [x] There are two nearly identical interfaces ReplicationPolicyManager.ReplicationsCompletedListener and ReplicationService.ReplicationCompleteListener - I think the latter could be deleted and just use the former interface.

wdyt?

[brynh]

@ricellis Yes, I agree that your suggestions make sense. Maybe as well to move SimpleReplicationCompleteListener from ReplicationService to ReplicationPolicyManager as well.