Annotate BatchStatement, Statement, SimpleStatement methods with CheckReturnValue

[akhaku]

Since the driver's default implementation is for
BatchStatement#add* methods to be immutable, we should annotate those methods with @CheckReturnValue

[akhaku]

Looks like I need to make changes to revapi.json for the build to be happy. Holding off on that until I get a 👍 or 👎 on making this change in general. The only reason I can think of for not adding these annotations is because, from the docs:

The driver's built-in implementation is immutable, and returns a new instance from this method. However custom implementations may choose to be mutable and return the same instance.

however similar documentation exists on methods like Statement#setExecutionProfileName but those methods are already annotated with @CheckReturnValue

Hm also it probably makes sense to add this on SimpleStatement#setQuery etc too... I'll wait to hear back on this first though.

[absurdfarce]

Thanks for the PR @akhaku!

After thinking about this for just a bit my inclination is to continue down the path you outline here. I'm not completely convinced about the idea of annotating the interfaces rather than the concrete implementations (i.e. the things which are actually immutable). That said, as you point out without this change there is a difference between the semantics of BatchStatement and Statement... and that doesn't seem right. The benefit of bringing those two interfaces into alignment seems like a strong enough argument on it's own to move forward with this PR; we can perhaps debate afterwards whether we want the interface methods to be so annotated, but whatever path we choose we should be consistent about it across the interfaces.

Let me know if you need anything re: getting the revapi checks to pass.

Also, would you mind adding the annotation to the setBatchType() and setKeyspace() methods on BatchStatement as well? They seem to be in a very similar situation.

Thanks again!

[adutra]

My 2c here is that you should add the missing annotation to all methods, and create exceptions in revapi.json.

I wouldn't care much about revapi here because even if it's indeed a breaking change stricto sensu, the worst that could happen is that this would break the build for people that have a strict static analyzer in place. But even if that happens, this is probably a good thing since it denotes a misusage of the driver API.

[adutra]

Hmm I might take that back sorry – at least partially :-)

I remember now: for SimpleStatement, BoundStatement and BatchStatement, the driver provides 2 implementations: an immutable one (declared in an internal package), and a mutable builder (declared in an API package).

The reason why we didn't annotate those interfaces with @CheckReturnValue is because of the builders, since they return the same instance.

I wouldn't mind adding the annotations though. It would make the usage of the immutable impls much easier for users; as for the builders, indeed users would need to recapture the returned object to make their IDEs happy, but this isn't incorrect either.

[akhaku]

Thank you both for the feedback and comments! I've been a little busy lately so haven't had time to get back to this just yet. I'll go ahead and add the annotations to setBatchType() and setKeyspace() too.
Good point about the builders, I didn't realize they actually implement their statement types too. But yeah it's certainly safer to keep/add the missing annotations, makes it a little more difficult for a user to mess this up.

[akhaku]

just a rebase

[absurdfarce]

Hey @akhaku, nice work here!

It seems like this addresses the outstanding issue with BatchStatement but this does now mean we have different semantics around the setters for that interface vs. the SimpleStatement interface. We should probably align those two so that the setters in SimpleStatement are also annotated in the same way.

I'm not sure I see how the builders are a problem in this case. BatchStatement.builder() returns an instance of BatchStatementBuilder; that class is mutable, but it doesn't implement the BatchStatement interface so there's no overlap there. BatchStatementBuilder.build() will produce an instance of BatchStatement but we re-use DefaultBatchStatement for that case. So it seems like the existing types would be okay if we annotate the interface.

It is true that this would force any custom implementations of the interface (which may or may not be immutable) to check return values for these methods. While this is somewhat annoying I'm not sure it's terribly onerous... and the benefits for the (much more common) default case seem to make it worthwhile.

[akhaku]

Just a rebase.
Regarding builders: gotcha, I was just commenting that I didn't realize the implementations returned by the builders are mutable by default. But I also didn't realize that they don't implement the BatchStatement interface, thanks.

Anyway, let me know if there's anything else I need to do here, otherwise please merge when ready!

[akhaku]

Thanks for the approval! Rebased to fix the merge conflict, please merge when ready.

[lukasz-antoniak]

Shall we also add @CheckReturnValue to Statement#setNowInSeconds(int)? All Implementations are immutable and create new object.

@NonNull
@SuppressWarnings("unchecked")
default SelfT setNowInSeconds(int nowInSeconds) {
  return (SelfT) this;
}

[akhaku]

Sure, done!

[tolbertam]

+1, i think this is good to merge as is, and 4.19.0 seems like a good time to do it!

I think @absurdfarce did still have a remaining point with regards to:

but this does now mean we have different semantics around the setters for that interface vs. the SimpleStatement interface. We should probably align those two so that the setters in SimpleStatement are also annotated in the same way.

For the same reason is there any reason why we wouldn't SimpleStatement: setQuery, setKeyspace, setPositionalValues, setNamedValuesWithIds, setNamedValues methods? It looks like we would want the annotation there for the same reason.

If you'd like to add those here that would be cool, but I also think we should be able to accept this PR as is as. I don't mind following up with a PR handling the SimpleStatement APIs if you are cool with it!

[absurdfarce]

I agree with @tolbertam that we're there for BatchStatement and that 4.19.0 seems like a good place to add this functionality. But the resulting disconnect between the APIs for BatchStatement and SimpleStatement continue to trouble me; I'm not sure we want to release 4.19.0 with that disconnect in place. We could file a ticket to fix that later but that seems less than ideal; I'm not sure when that would actually happen and this release would go out with the disconnect in place.

@akhaku are you able to include that change here?

[akhaku]

Fair enough, yeah updated with the suggested changes to SimpleStatement too 👍 and rebased as well

[akhaku]

Fixed a couple missed revapi changes and updated commit message with a more up-to-date description. Once I get that second +1 I'll update the commit message with the reviewers to get it ready to merge.

[absurdfarce]

I've got a Jenkins run for your latest update running right now @akhaku. Assuming there aren't any regressions there (and I don't expect there will be) your second +1 will be en route shortly.

[absurdfarce]

Okay, Jenkins run completed. Only failure was ContinuousPagingIT and it's known to have some issues (JAVA-2073). @akhaku you have your second +1 so if you can make the changes to the commit message we're all set here!

[akhaku]

Thanks, commit message updated, ready to merge

[absurdfarce]

Many thanks @akhaku!

#ThisIsHappening!