Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CloudTableClient should use Iterator instead of Iterable. #55

Closed
wants to merge 1 commit into from

Conversation

christophercurrie
Copy link

The Problem

CloudTableClient#execute(TableQuery<?>, ..) overloads return Iterable, which implies a collection that can be iterated over multiple times. However, the return value is implemented as an Iterator that implements Iterable by returning this, which is generally regarded as a Bad Thing. More on this.

The temptation to do this is common given the seductive lure of being able to use your iterator in a foreach loop:

foreach (Result r : tableClient.execute(query, resolver)) {
 // handle results
}

But, because execute returns an Iterator under the hood, this breaks down if you try to do it twice:

Iterable<Result> result = tableClient.execute(query, resolver);

foreach (Result r : result) {
 // handle results
}

foreach (Result r : result) {
 // never reached
}

Iterable expects this to work, as does mountains of client code that consume Iterable instances. The fact that Iterator can't normally be used in this way is a warning and reminder to users that iterators are single pass only.

In general, methods that return a result that can only be iterated once should return Iterator, not Iterable. This pull request implements that policy for CloudTableClient, at the cost of breaking client code.

Alternative solutions

If breaking client code is not an option, the following alternatives can be explored:

Deprecate the old code, and add new methods

This option keeps the broken old behavior but deprecates it, so that end user expectations aren't changed. New methods that return Iterator could be added to indicate the single-pass semantics.

Keep the old interface, but make it support the correct semantics.

Iterable should return a new iterator for each call to iterator. There are some options for how to achieve this, such as (a) caching the results, or (b) issuing a new query to the service. Each choice has trade-offs, which is why dropping or deprecating the API should be preferred. But a correctly functioning API that has trade-offs is better than an incorrect one.

@ghost ghost assigned ghost and jeffreyjirwin May 4, 2012
@jeffreyjirwin
Copy link

Hi Christopher - thanks for the feedback. We've now addressed this in the updated source available here.

Thanks again, and please continue to let us know if you have other feedback!
-Jeff

@christophercurrie
Copy link
Author

Thanks, I see the change implemented in 0f84dbb, looks great. Thanks for addressing this quickly. I look forward to a new Maven release!

joostdenijs pushed a commit to joostdenijs/azure-sdk-for-java that referenced this pull request Jan 18, 2013
jianghaolu pushed a commit to jianghaolu/azure-sdk-for-java that referenced this pull request Apr 5, 2017
g2vinay pushed a commit to g2vinay/azure-sdk-for-java that referenced this pull request Mar 4, 2019
Java Storage Client Library 4.0.0
g2vinay pushed a commit to g2vinay/azure-sdk-for-java that referenced this pull request Mar 4, 2019
Fixed breaking changes and changelog for 5.1.0 release
g2vinay pushed a commit to g2vinay/azure-sdk-for-java that referenced this pull request May 30, 2019
navalev pushed a commit to navalev/azure-sdk-for-java that referenced this pull request Dec 24, 2019
- General refactoring and rearrangements for tests.
- Adding readJsonFileToList() to JsonApi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants