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.

Sign up for GitHub

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

Jump to bottom

LLC Paging #1061

Merged
merged 30 commits into from
Jul 1, 2021
Merged

LLC Paging #1061

merged 30 commits into from
Jul 1, 2021

Conversation

qwordy
Copy link
Member

@qwordy qwordy commented Jun 18, 2021 •
edited
Loading

Resolve #1034

Add paging support for low level client. Generated code example is in protocol-tests.paging. impl is internal implementation. Users will use client class to access service. We have two kinds of clients, sync and async client. Each paging method in swagger will generate 4 methods.

TODO

  • Allow only path parameters. (To be confirmed)
  • Java doc generation

@qwordy qwordy mentioned this pull request Jun 25, 2021
Closed
6 tasks
@qwordy
Copy link
Member Author

qwordy commented Jun 28, 2021

Generated code misses parameters. Fixing. In the following code, offset is missing.

    @ServiceMethod(returns = ReturnType.SINGLE)
    public Mono<PagedResponse<BinaryData>> getMultiplePagesWithOffsetSinglePageAsync(RequestOptions requestOptions) {
        final String accept = "application/json";
        return FluxUtil.withContext(
                        context ->
                                service.getMultiplePagesWithOffset(
                                        this.client.getHost(), offset, accept, requestOptions, context))

@qwordy
Copy link
Member Author

qwordy commented Jun 28, 2021

Fixed it by adding

--pipeline.modelerfour.flatten-models=false --pipeline.modelerfour.flatten-payloads=false --pipeline.modelerfour.group-parameters=false

@qwordy
Copy link
Member Author

qwordy commented Jun 29, 2021 •
edited
Loading

Basic test can pass now👍
A issue:
clientMethod.getMethodPageDetails().getNextLinkName() returns the name in Java class, e.g., odataNextLink. But the raw name in swagger and response is odata.nextLink.
Same issue applies to item name, too. getItemName() returns indexes. Raw name is values.

    @JsonProperty(value = "values")
    private List<Product> indexes;

@qwordy
Copy link
Member Author

qwordy commented Jun 29, 2021

The issue is fixed👍

@qwordy qwordy marked this pull request as ready for review June 29, 2021 14:15
weidongxu-microsoft
weidongxu-microsoft reviewed Jun 30, 2021
View reviewed changes
javagen/src/main/java/com/azure/autorest/template/MethodGroupTemplate.java Outdated
Comment on lines 99 to 107
classBlock.privateMethod("List<BinaryData> getValues(BinaryData binaryData, String path)", block -> {
block.line("Object obj = binaryData.toObject(Object.class);");
block.line("Object values = ((Map)obj).get(path);");
block.line("return (List<BinaryData>)(((List)values).stream().map(BinaryData::fromObject).collect(Collectors.toList()));");
});
classBlock.privateMethod("String getNextLink(BinaryData binaryData, String path)", block -> {
block.line("Object obj = binaryData.toObject(Object.class);");
block.line("return (String)((Map)obj).getOrDefault(path, null);");
});
Copy link
Member

Choose a reason for hiding this comment

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

You will need more code on check and protection on class type etc. in real code.

protocol-tests/src/main/java/fixtures/paging/implementation/PagingsImpl.java
* <p><strong>Response Body Schema</strong>
*
* <pre>{@code
* BinaryData
Copy link
Member

Choose a reason for hiding this comment

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

The schema is probably not correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing.

@qwordy qwordy merged commit 6c15cd0 into Azure:llc Jul 1, 2021
@qwordy qwordy mentioned this pull request Jul 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianghaolu jianghaolu jianghaolu approved these changes

@weidongxu-microsoft weidongxu-microsoft weidongxu-microsoft approved these changes

@ChenTanyi ChenTanyi Awaiting requested review from ChenTanyi

@srnagar srnagar Awaiting requested review from srnagar

@yungezz yungezz Awaiting requested review from yungezz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qwordy @jianghaolu @weidongxu-microsoft @actions-user