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

Add RequestOptions & handling in RestProxy #22334

Merged
merged 11 commits into from
Jun 24, 2021
Merged

Add RequestOptions & handling in RestProxy #22334

merged 11 commits into from
Jun 24, 2021

Conversation

jianghaolu
Copy link
Contributor

No description provided.

@ghost ghost added the Azure.Core azure-core label Jun 16, 2021
@jianghaolu jianghaolu mentioned this pull request Jun 16, 2021
Closed
@jianghaolu jianghaolu mentioned this pull request Jun 21, 2021
Merged
srnagar
srnagar reviewed Jun 21, 2021
View reviewed changes
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/ResponseStatusOption.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated
* @param requestCallback the request callback
* @return the modified RequestOptions object
*/
public RequestOptions addRequestCallback(Consumer<HttpRequest> requestCallback) {
Copy link
Member

Choose a reason for hiding this comment

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

requestCallback may not convey the purpose of the callback. Other options:

  • addRequestModifier
  • addRequestProcessor
  • addRequestHandler
  • addRequestTransformation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sticking with requestCallback for now and may revisit in the future

alzimmermsft
alzimmermsft reviewed Jun 22, 2021
View reviewed changes
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/RequestOptions.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/rest/RestProxy.java
@@ -286,7 +295,11 @@ private HttpRequest configRequest(final HttpRequest request, final SwaggerMethod
}
}

if (isJson) {
if (bodyContentObject instanceof BinaryData) {
Copy link
Member

Choose a reason for hiding this comment

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

Once it is merge this should use RequestContent. Right now, this is creating another copy of the underlying BinaryData data.

sdk/core/azure-core/src/main/java/com/azure/core/http/rest/RestProxy.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/rest/RestProxy.java Outdated Show resolved Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/http/rest/SwaggerMethodParser.java
* @return the request options
*/
public RequestOptions setRequestOptions(Object[] swaggerMethodArguments) {
return CoreUtils.findFirstOfType(swaggerMethodArguments, RequestOptions.class);
Copy link
Member

Choose a reason for hiding this comment

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

This is an ancillary performance question, does the Java proxy interface pass the Object[] in a consistent order? I'm wondering if there could be an optimization for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AutoRest generated code does, but I'm not sure if azure-core could make the assumption that all code follows the same order.

However, a CoreUtils.findLastOfType() might slightly improve the perf.

@jianghaolu jianghaolu force-pushed the restproxy-requestoptions branch from 90c12ce to 0620138 Compare June 23, 2021 21:05
@jianghaolu jianghaolu marked this pull request as ready for review June 23, 2021 21:06
@jianghaolu jianghaolu merged commit d4f0b3c into Azure:feature/jianghao/requestoptions Jun 24, 2021
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-java that referenced this pull request Feb 28, 2023
@mozansaka
CodeGen from PR 22124 in Azure/azure-rest-api-specs
e516d1b 
[Hub Generated] Review request for Microsoft.DBforPostgreSQL to add version stable/2022-11-08 (Azure#22124)

* Adds base for updating Microsoft.DBforPostgreSQL from version preview/2020-10-05-privatepreview to version 2022-11-08

* Updates readme

* Updates API version in new specs and examples

* empty commit (Azure#21932)

Co-authored-by: Ozan Saka <[email protected]>

* 2022 11 08 (Azure#22025)

* Add private endpoint con. and private link endpoints

* Add private link list example

* Cluster related endpoint and exampled changes

* Cluster changes and configuration updates

* Long running operation header additions

* Change server group to cluster

* Add systemData to examples, fix private endpoint con. property

* Fix private link resource name parameter

* Fix INVALID_TYPE boolean given as strings

* Update approve private endp. con. request body

* Fix OBJECT_ADDITIONAL_PROPERTIES and READONLY_PROPERTY_NOT_ALLOWED_IN_REQUEST

* Fix INVALID_TYPE

* Fix ModelValidation Errors

* Fix lint errors

* Fix lint errors

* Remove location from cluster examples

* Update example details, fix common types

* Update readme files, fix configuration example name

* Fix SDK generation by removing wrong pageable tags

* Update configuration response types, fix cluster name pattern

* Update configuration example names

* Readme changes and minor final state via fix

* Fix indentation on suppression items

* Update node count description, remove pec id

* Update cluster examples

* Style fix

* Update description

Co-authored-by: Ozan Saka <[email protected]>
Co-authored-by: Sena Gungor <[email protected]>

* Fix credscan password (Azure#22127)

Co-authored-by: Ozan Saka <[email protected]>

* Add required parameters (Azure#22244)

* Add 200 resp. code to put&patch, change boolean to enum (Azure#22290)

* Add 200 resp. code to put&patch, change boolean to enum

* Update 200 responses

* Update 200 response schemas, enum casing

* Update configuration put requests

* Remove role no-op case

* fix private endp con list error, remove 200 code from role example

* Add coord/node config list endpoints

* fix sdk errors

* add 200 resp for configuration put requests

* Update readme

* Fix arm feedbacks (Azure#22334)

* Fix arm feedbacks

* Add pointInTimeUTC

* Update passwords and readme file

* Update response codes to 201, address feedbacks (Azure#22386)

* Update response codes to 201

* Add server configuration get

* Update password

* Fix duplicate operation id

* Update configuration example files

* Update configuration example files

* Fix credscan issue (Azure#22389)

* Fix PutRequestResponseScheme error (Azure#22408)

* Fix PutRequestResponseScheme error

* Update readme file

* Update x-ms-mutability

* Update property description

* Fix server properties (Azure#22772)

* Fix server properties

---------

Co-authored-by: mozansaka <[email protected]>
Co-authored-by: Ozan Saka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alzimmermsft alzimmermsft alzimmermsft left review comments

@srnagar srnagar srnagar approved these changes

@JonathanGiles JonathanGiles Awaiting requested review from JonathanGiles

@anuchandy anuchandy Awaiting requested review from anuchandy

@hemanttanwar hemanttanwar Awaiting requested review from hemanttanwar

@lmolkova lmolkova Awaiting requested review from lmolkova

Assignees
No one assigned
Labels
Azure.Core azure-core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jianghaolu @alzimmermsft @srnagar