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

Paged Flux abstractions #6465

Merged
merged 43 commits into from
Jan 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
424490c
PagedFluxCore as an abstraction for generic paging (not tied to proto…
anuchandy Nov 21, 2019
35f8814
Making PagedFluxCore abstract, letting user to define state type
anuchandy Nov 21, 2019
1eb7381
Making the Paged Flux type more flexiable
anuchandy Nov 24, 2019
d3a5d42
Adding byItem(token) and some cleanup.
anuchandy Nov 25, 2019
1dee7f5
Code cleanup and adding javadoc code-snippets for PagedFluxCore
anuchandy Nov 26, 2019
7fd7e41
Merge branch 'master' of github.com:azure/azure-sdk-for-java into pag…
anuchandy Nov 26, 2019
92029bd
Moving paging base types to util package, adding exclusions for pagin…
anuchandy Nov 26, 2019
946f8ae
Making the base Paged Flux completely abstract, exposing prefetch for…
anuchandy Dec 1, 2019
8c69889
Enabling a way to decorate a PagedFlux and have that decoreated type …
anuchandy Dec 11, 2019
1540c6c
Removed unused internal ctr
anuchandy Dec 11, 2019
dfa1f40
Update sdk/core/azure-core/src/main/java/com/azure/core/util/paging/C…
anuchandy Dec 11, 2019
b9fe5e9
cleaning up comments and simplifying the code
anuchandy Dec 12, 2019
317b7d8
Merge branch 'paging-base' of github.com:anuchandy/azure-sdk-for-java…
anuchandy Dec 12, 2019
6b82642
Update sdk/core/azure-core/src/main/java/com/azure/core/http/rest/Pag…
anuchandy Dec 12, 2019
3175061
Removing generic arg for contination option, adding sample for PagedF…
anuchandy Dec 12, 2019
81bcb74
Merge branch 'paging-base' of github.com:anuchandy/azure-sdk-for-java…
anuchandy Dec 12, 2019
87ae9f8
Merge branch 'master' of github.com:azure/azure-sdk-for-java into pag…
anuchandy Dec 13, 2019
b1df280
Simplifying ContinuablePagedFluxCore concat logic and updating the Pa…
anuchandy Dec 13, 2019
4670ab7
Adding getElements() to Page that return elements as iterable-stream,…
anuchandy Dec 13, 2019
0e48a11
Moving deprecated getItems() to REST Page
anuchandy Dec 13, 2019
4a67f98
Checking for null page items before creating IterableStream, Fixing t…
anuchandy Dec 14, 2019
9f77c80
Re-recording data lake test as we no longer make extra network call
anuchandy Dec 14, 2019
daff607
Handle the case where byPage(ct) can be invoked on a PagedFlux create…
anuchandy Dec 14, 2019
1607889
re-record datalake tests using PagedFlux/PagedIterable
anuchandy Dec 14, 2019
8e88204
Removing interface type PageRetrievalProvider
anuchandy Dec 15, 2019
759e88c
Adding preferred page size option
anuchandy Dec 15, 2019
33de1d9
javadoc updates for PagedFlux types
anuchandy Dec 15, 2019
5afcc84
javadoc update, use null instead of -1 for no page-size
anuchandy Jan 5, 2020
65942bb
fix merge conflcits
anuchandy Jan 5, 2020
879f7ca
Udating keyvault, storage-blob-batch and azure-core-test referenced v…
anuchandy Jan 5, 2020
ee078a8
Fixing checkstyles in textanalytics samples
anuchandy Jan 5, 2020
5363af6
Explicitly specify type in code-snippet
anuchandy Jan 6, 2020
f23ac6e
updating azure-core version of kevault-certificates, keys, secrets, s…
anuchandy Jan 6, 2020
d78a70b
Adding revapi supression for new base ContinuablePagedFluxCore
anuchandy Jan 6, 2020
8c5372f
sync with upstream
anuchandy Jan 14, 2020
79b1543
bump up azure-core versions of client-libs that uses new page types
anuchandy Jan 15, 2020
d2e764f
Merge branch 'master' of github.com:azure/azure-sdk-for-java into pag…
anuchandy Jan 15, 2020
dfe68d4
Merge branch 'master' of github.com:azure/azure-sdk-for-java into pag…
anuchandy Jan 15, 2020
f01f1c9
javadoc improvments
anuchandy Jan 15, 2020
ac3a8f4
Page::getElements() should not return null
anuchandy Jan 17, 2020
b08ec16
PagedFlux javadoc update
anuchandy Jan 17, 2020
43e83e4
Adding IterableStream::of(Iterable) factory method
anuchandy Jan 17, 2020
44fcc6d
Merge branch 'paging-base' of github.com:anuchandy/azure-sdk-for-java…
anuchandy Jan 17, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@
"exampleUseChainInNewApi": ".*com\\.azure\\.messaging\\.eventhubs\\.checkpointstore\\.blob\\..*",
"justification": "azure-messaging-eventhubs and azure-storage-blob are used in the Event Hubs checkpoint store."
},
{
"regex": true,
"code": "java\\.missing\\.(oldSuperType|newSuperType)",
"new": "class com\\.azure\\.core\\.http\\.rest\\.((PagedFlux<T extends java\\.lang\\.Object>)|(PagedFluxBase<T extends java\\.lang\\.Object.*>)).*",
"justification": "azure.core 1.3.0 will introduce a new base type for GAed PagedFlux. Since the base type is not in azure.core <= 1.2.0, revapi is reporting it. This entry should be removed after 1.3.0 release."
},
{
"regex": true,
"code": "java\\.missing\\.(oldClass|newClass)",
Expand All @@ -93,4 +99,4 @@
}
]
}
]
]
Original file line number Diff line number Diff line change
Expand Up @@ -707,4 +707,14 @@
<Bug pattern="CN_IMPLEMENTS_CLONE_BUT_NOT_CLONEABLE"/>
</Match>

<Match>
<Or>
<Class name="com.azure.core.util.paging.PagedFluxCoreJavaDocCodeSnippets"/>
</Or>
<Bug pattern="NP_LOAD_OF_KNOWN_NULL_VALUE,
SIC_INNER_SHOULD_BE_STATIC_ANON,
UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS,
NP_BOOLEAN_RETURN_NULL"/>
</Match>

</FindBugsFilter>
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.
package com.azure.data.appconfiguration.implementation;

import com.azure.core.util.IterableStream;
import com.azure.data.appconfiguration.models.ConfigurationSetting;
import com.azure.core.http.rest.Page;
import com.fasterxml.jackson.annotation.JsonProperty;
Expand Down Expand Up @@ -30,12 +31,12 @@ public String getContinuationToken() {
}

/**
* Gets the list of {@link ConfigurationSetting ConfigurationSettings} on this page.
* Gets the iterable stream of {@link ConfigurationSetting ConfigurationSettings} on this page.
*
* @return The list of {@link ConfigurationSetting ConfigurationSettings}.
* @return The iterable stream of {@link ConfigurationSetting ConfigurationSettings}.
*/
@Override
public List<ConfigurationSetting> getItems() {
return items;
public IterableStream<ConfigurationSetting> getElements() {
return IterableStream.of(items);
}
}
2 changes: 1 addition & 1 deletion sdk/core/azure-core-test/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-core</artifactId>
<version>1.2.0</version> <!-- {x-version-update;com.azure:azure-core;dependency} -->
<version>1.3.0-beta.1</version> <!-- {x-version-update;unreleased_com.azure:azure-core;dependency} -->
</dependency>

<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

Expand Down Expand Up @@ -36,6 +35,7 @@
import com.azure.core.test.http.MockHttpResponse;
import com.azure.core.test.http.NoOpHttpClient;
import com.azure.core.util.Base64Url;
import com.azure.core.util.IterableStream;
import com.fasterxml.jackson.annotation.JsonGetter;
import com.fasterxml.jackson.annotation.JsonProperty;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -468,8 +468,8 @@ static class KeyValuePage implements Page<KeyValue> {
}

@Override
public List<KeyValue> getItems() {
return items;
public IterableStream<KeyValue> getElements() {
return new IterableStream<KeyValue>(items);
}

@Override
Expand All @@ -488,8 +488,8 @@ static class ConformingPage<T> implements Page<T> {
}

@Override
public List<T> getItems() {
return items;
public IterableStream<T> getElements() {
return IterableStream.of(items);
}

@Override
Expand Down Expand Up @@ -611,9 +611,8 @@ public void service2getPageSerializes() {
StepVerifier.create(createService(Service2.class).getPageAsyncSerializes(page))
.assertNext(response -> {
assertEquals(page.getContinuationToken(), response.getContinuationToken());
assertNull(response.getItems());
assertEquals(0, response.getItems().size());
})
.verifyComplete();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,31 @@

package com.azure.core.http.rest;

import com.azure.core.util.IterableStream;
import com.azure.core.util.paging.ContinuablePage;

import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;

/**
* Represents a paginated REST response from the service.
*
* @param <T> Type of the listed objects in that response.
* @param <T> Type of items in the page response.
*/
public interface Page<T> {

public interface Page<T> extends ContinuablePage<String, T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A ContinuablePage is a special type of Page and ContinuablePage would extend from Page.
Ideally, we could have changed the name of this interface to ContinuablePage and the base interface could have been just Page with one method getElements()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here:

  1. We need a base type to represent the general page contract, we don’t want that contract to be in http package since non-http based client libs might want to use it. A genuine concern from cosmos team was - inheritance hierarchy of TCP based cosmos type containing core types from http package, the Page type is in http package.
  2. Ideally we could merge Page and ContinuablePage and call it Page then move it out of http package but that is a breaking change.

So we’ve above two constraints - hence the type ContinuablePage and inheritance hierarchy as proposed in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anuchandy Can you please be sure to update #6610 with the final set of changes we would make when a breaking-change release is possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because we have a released version already, we'll have to keep this hierarchy.

/**
* Gets a list of items returned from the service.
* Get list of elements in the page.
*
* @return A list of items from the service.
*/
List<T> getItems();

/**
* Gets a link to the next page, or {@code null} if there are no more results.
* @return the page elements
*
* @return A link to the next page, or {@code null} if there are no more results.
* @deprecated use {@link this#getElements()}.
*/
String getContinuationToken();
@Deprecated
default List<T> getItems() {
IterableStream<T> iterableStream = this.getElements();
return iterableStream == null
? new ArrayList<>()
: this.getElements().stream().collect(Collectors.toList());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,24 @@
import com.azure.core.http.HttpRequest;

import java.util.stream.Collectors;

import com.azure.core.util.paging.PageRetriever;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;

import java.util.function.Function;
import java.util.function.Supplier;

/**
* This class is a flux that can operate on a {@link PagedResponse} and also provides the ability to operate on
* individual items. When processing the response by page, each response will contain the items in the page as well as
* the request details like status code and headers.
* This type is a Flux that provides the ability to operate on paginated REST responses of type {@link PagedResponse}
* and individual items in such pages. When processing the response by page, each response will contain the items
* in the page as well as the REST response details like status code and headers.
*
* <p>To process one item at a time, simply subscribe to this flux as shown below </p>
* <p><strong>Code sample</strong></p>
* {@codesnippet com.azure.core.http.rest.pagedflux.items}
*
* <p>To process one page at a time, use {@link #byPage} method as shown below </p>
* <p>To process one page at a time, use {@link #byPage()} method as shown below </p>
* <p><strong>Code sample</strong></p>
* {@codesnippet com.azure.core.http.rest.pagedflux.pages}
*
Expand All @@ -37,24 +39,23 @@
* @see Flux
*/
public class PagedFlux<T> extends PagedFluxBase<T, PagedResponse<T>> {

/**
* Creates an instance of {@link PagedFlux} that consists of only a single page of results. The only argument to
* this constructor therefore is a supplier that fetches the first (and known-only) page of {@code T}.
* Creates an instance of {@link PagedFlux} that consists of only a single page.
* This constructor takes a {@code Supplier} that return the single page of {@code T}.
*
* <p><strong>Code sample</strong></p>
* {@codesnippet com.azure.core.http.rest.pagedflux.singlepage.instantiation}
*
* @param firstPageRetriever Supplier that retrieves the first page.
*/
public PagedFlux(Supplier<Mono<PagedResponse<T>>> firstPageRetriever) {
super(firstPageRetriever);
this(firstPageRetriever, token -> Mono.empty());
}

/**
* Creates an instance of {@link PagedFlux}. The constructor takes in two arguments. The first argument is a
* supplier that fetches the first page of {@code T}. The second argument is a function that fetches subsequent
* pages of {@code T}
* Creates an instance of {@link PagedFlux}. The constructor takes a {@code Supplier} and
* {@code Function}. The {@code Supplier} returns the first page of {@code T},
* the {@code Function} retrieves subsequent pages of {@code T}.
*
* <p><strong>Code sample</strong></p>
* {@codesnippet com.azure.core.http.rest.pagedflux.instantiation}
Expand All @@ -64,21 +65,65 @@ public PagedFlux(Supplier<Mono<PagedResponse<T>>> firstPageRetriever) {
*/
public PagedFlux(Supplier<Mono<PagedResponse<T>>> firstPageRetriever,
Function<String, Mono<PagedResponse<T>>> nextPageRetriever) {
super(firstPageRetriever, nextPageRetriever);
this(() -> (continuationToken, pageSize) -> continuationToken == null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pageSize is not used here. Should we also support having an overload that doesn't require pageSize? This is also the most common case in all the services so far (except Cosmos maybe).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storage (e.g. in blob list) and many of the ARM services supports client driven pagination. Agree that a version of PageRetriever without pageSize can simplify implementation of client APIs that does not support page-size, but it brings a new type that is not used by customers but only by SDK implementors. I was thinking it is ok to keep the bar high for SDK implementor if that save us from adding a new type (and provide the type when customer ask for it)

? firstPageRetriever.get().flux()
: nextPageRetriever.apply(continuationToken).flux(), true);
}

/**
* Create PagedFlux backed by Page Retriever Function Supplier.
*
* @param provider the Page Retrieval Provider
* @param ignored param is ignored, exists in signature only to avoid conflict with first ctr
*/
private PagedFlux(Supplier<PageRetriever<String, PagedResponse<T>>> provider, boolean ignored) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too bad that we have to use a factory to create this. I think it makes it less discoverable since you can do new Paged(..., and if you want to use the cooler PageRetriever, figure out that instead of new, use .create

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish Java has a way to work around type erasure which is causing this pain. As pointed in the code comments - proposal is to update the current ctr taking Mono Supplier with PageRetriever Supplier, but we've to wait for major version bump up for doing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will unfortunately need to live with this inconsistency until we can do a breaking change release. The only positive is that we don't expect end users to be dealing with this API, so the only people encountering this will be client library developers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Maybe this can be tracked in a github issue with appropriate labels so we don't forget to make this change when working on a major version release.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this work item here: #6610 (comment)

super(provider, ignored);
}

/**
* Creates an instance of {@link PagedFlux} backed by a Page Retriever Supplier (provider).
* When invoked provider should return {@link PageRetriever}. The provider will be called for each
* Subscription to the PagedFlux instance. The Page Retriever can get called multiple times in serial
* fashion, each time after the completion of the Flux returned from the previous invocation.
* The final completion signal will be send to the Subscriber when the last Page emitted by the Flux
* returned by Page Retriever has {@code null} continuation token.
*
* The provider is useful mainly in two scenarios:
* <ul>
* <li> To manage state across multiple call to Page Retrieval within the same Subscription.
* <li> To decorate a PagedFlux to produce new PagedFlux.
* </ul>
*
* <p><strong>Decoration sample</strong></p>
* {@codesnippet com.azure.core.http.rest.pagedflux.create.decoration}
*
* @param provider the Page Retrieval Provider
* @param <T> The type of items in a {@link PagedResponse}
* @return PagedFlux backed by the Page Retriever Function Supplier
*/
public static <T> PagedFlux<T> create(Supplier<PageRetriever<String, PagedResponse<T>>> provider) {
return new PagedFlux<>(provider, true);
}

/**
* Maps this PagedFlux instance of T to a PagedFlux instance of type S as per the provided mapper function.
* Maps this PagedFlux instance of T to a PagedFlux instance of type S as per the provided mapper
* function.
*
* @param mapper The mapper function to convert from type T to type S.
* @param <S> The mapped type.
* @return A PagedFlux of type S.
* @deprecated refer the decoration samples for {@link PagedFlux#create(Supplier)}.
*/
@Deprecated
public <S> PagedFlux<S> mapPage(Function<T, S> mapper) {
return new PagedFlux<S>(() -> getFirstPageRetriever().get()
.map(mapPagedResponse(mapper)),
continuationToken -> getNextPageRetriever().apply(continuationToken)
.map(mapPagedResponse(mapper)));
Supplier<PageRetriever<String, PagedResponse<S>>> provider = () -> (continuationToken, pageSize) -> {
Flux<PagedResponse<T>> flux = (continuationToken == null)
? byPage()
: byPage(continuationToken);
return flux
.map(mapPagedResponse(mapper));
};
return PagedFlux.create(provider);
}

private <S> Function<PagedResponse<T>, PagedResponse<S>> mapPagedResponse(Function<T, S> mapper) {
Expand Down
Loading