Final instances vs Incomplete instances vs Update-in-place

[bitwiseman]

#495 and #496 raise an interesting point. Also #584.

This project currently tends to treat instances of data objects as "final" - you request an Issue and the instance you requested never changes after that. That instance is a stable snapshot and can be used as a key in a HashSet or Map without fear of it changing.

We also have action and update methods on these objects. In general, these methods update the state of the server-side object the instance is based on, but do not change the data in the instance. You call close() or delete() or one of the set* methods and a request is sent to change the object on the server, but the data in the local instance doesn't update to match. This is consistent with the "final instances" model but has some problems. There's no batching of updates (each action is another query) and since the local instance is out of date, updates can end up overwriting each other as in #495. The only current workaround is send another request to get a fresh instance for each update. 😱

The Builder/Updater pattern added to some objects addresses this by batching updates together and then returning a new instance matching those updates. This solves the problem of stale data but results in verbose item.update().title('New Title').update() syntax for single field changes. It does partially unblock update-in-place as item = item.update().title('New Title').update().

Finally, some objects can be incomplete when first created - either due to their source (partial information provided as part of creating another object) or due to the target only updating on request (pull request mergability is only updated after the first time PR is queried after a new commit is pushed). For these cases, we currently update in place. Do we want to force users to get new instances instead?

There's no one right way to do this, but we should look at how to make this behavior consistent across all objects.

A couple random thought s off hand about this:

• set* method naming is misleading - best practice in my understanding is to make get* and set* methods local and lightweight. Alternative would be to have update* methods that actually send updates to the server.
• Create states for the objects and allow the parent objects and the GitHub object to create objects in those state. Rather than having updaters for objects, users would get a mutable instance. Possible states for data objects: immutable, immediate mutable, batch mutable. Not sure how the incomplete objects figure into this yet.

[bitwiseman]

A thought I had today. update* methods could return a new instance with the updated data, while update() would start a batch.

So:

GHLabel label = getLabel(myLabelName);
// two network calls, each returning a new instance
label = label.updateName("New Name").updateColor(newColor);

// one network call, returns new instance
label = label.update().name("New Name").color(newColor).apply();

// one network call, updates in place?  Maybe not. 
// I'd really rather have instances be effectively final so their hashCode and equality are stable. 
// Set the state of the connection to update in place.
// This changes all instances created with this connection to update in place. 
github.updateInPlace(true);
label = github.getRepository(myRepoName).getLabel(myLabelName);
label.update().name("New Name").color(newColor).apply();

[bitwiseman]

Current design for the set()/update() design doesn't currently allow "update in place" but it can do handle it:

So:

GHLabel label = getLabel(myLabelName);
// two network calls, each returning a new instance
label = label.set().name("New Name");
label = label.set().color(newColor);

// one network call, returns new instance
label = label.update().name("New Name").color(newColor).done();

Possible designs to handle "update in place"

Firstly, in all cases, the GitHubClient instance should be able to control "update in place" behavior for all objects it creates.

enum UpdateInPlace {
    // Lets objects and operations decide for themselves if they will update in place
    // NOTE: `hashCode()` and `equals()` are not guaranteed stable with this setting enabled
    // Individual classes classes or instances can override this.  
    ALLOWED, 
    // throw if update in place is attempted.
    // NOTE: `populate()` should still succeed.
    // `refresh()` does not update current instance, returns a new instance.
    // QUESTION: Should get `hashCode()` and `equals()` automatically call `populate()` to ensure they are stable? 
    NEVER, 
    // always update in place
    // NOTE: `hashCode()` and `equals()` are not guaranteed stable with this setting enabled 
    // `refresh()` updates current instance and returns it (but return can be ignored.
    ALWAYS
}

GitHub gitHub = new GitHubBuilder().updateInPlace(UpdateInPlace.ALWAYS).build();

There are then several ways individual classes and instances can choose to update in place or not.
Right now, different classes have different behaviors with not pattern. For the first phase of this change the defaults will remain unchanged, in a future version we'll move to one consistent behavior - likely inherited from the GitHub instance.

For the set()/update() methods we have already defaulted with UpdateInPlace.NEVER. We'll switch to default of ALLOWED but also not update in place unless requested.

Add withRefresh() method

GHLabel label = getLabel(myLabelName);
// two network calls, each updating in place.
// They also return the same instance but we can ignore it
label.set().withRefresh().name("New Name");
label.set().withRefresh().color(newColor);

// one network call, updates in place
// return the same instance but we can ignore it
label = label.update().withRefresh().name("New Name").color(newColor).done();

Add setThis() and updateThis() methods

GHLabel label = getLabel(myLabelName);
// two network calls, each updating in place.
// They also return the same instance but we can ignore it
label.setThis().name("New Name");
label.setThis().color(newColor);

// one network call, updates in place
// return the same instance but we can ignore it
label = label.updateThis().name("New Name").color(newColor).done();