Integrate Sync Stack on KV Keys #30162

g2vinay · 2022-07-27T19:21:42Z

Integrate Sync Stack on KV Keys SDK.

Polling and Paging APIs will be syncified later.

Cryptography Client being syncified in next PR.

Enabled Sync Rest Proxy by default.

g2vinay · 2022-07-27T19:23:07Z

...e-security-keyvault-keys/src/test/java/com/azure/security/keyvault/keys/AssertingClient.java

+/**
+ * An HTTP Client that helps in providing assertions for invoking http client implementations.
+ */
+public final class AssertingClient implements HttpClient {


Note: Temporarily added, will be removed before merging.

g2vinay · 2022-07-27T19:23:27Z

...keyvault-keys/src/test/java/com/azure/security/keyvault/keys/AssertingHttpClientBuilder.java

+/**
+ * HTTP client builder that helps in running sync and async assertion checks.
+ */
+public class AssertingHttpClientBuilder {


Temporarily added, will be removed before merging.

azure-sdk · 2022-07-27T19:29:30Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-security-keyvault-keys

kasobol-msft

LGTM.

...-security-keyvault-keys/src/main/java/com/azure/security/keyvault/keys/KeyClientBuilder.java

alzimmermsft · 2022-07-28T16:11:58Z

...re-security-keyvault-keys/src/main/java/com/azure/security/keyvault/keys/KeyAsyncClient.java

 * @see PagedFlux
 */
-@ServiceClient(builder = KeyClientBuilder.class, isAsync = true, serviceInterfaces = KeyService.class)
+@ServiceClient(builder = KeyClientBuilder.class, isAsync = true)


We should continue providing serviceInterfaces = <the interface RestProxy uses>

Can we use service interface that's in impl package here ?

We don't do that here

azure-sdk-for-java/sdk/anomalydetector/azure-ai-anomalydetector/src/main/java/com/azure/ai/anomalydetector/AnomalyDetectorClient.java

Line 35 in 639bff3

@ServiceClient(builder = AnomalyDetectorClientBuilder.class)

We should continue providing serviceInterfaces = <the interface RestProxy uses>

I think we need that in the <service> interface class.

...-security-keyvault-keys/src/main/java/com/azure/security/keyvault/keys/KeyClientBuilder.java

.../src/main/java/com/azure/security/keyvault/keys/implementation/KeyVaultErrorCodeStrings.java

...yvault-keys/src/main/java/com/azure/security/keyvault/keys/implementation/KeyClientImpl.java

samvaity · 2022-07-28T17:45:50Z

...re-security-keyvault-keys/src/main/java/com/azure/security/keyvault/keys/KeyAsyncClient.java

 * @see PagedFlux
 */
-@ServiceClient(builder = KeyClientBuilder.class, isAsync = true, serviceInterfaces = KeyService.class)
+@ServiceClient(builder = KeyClientBuilder.class, isAsync = true)


We should continue providing serviceInterfaces = <the interface RestProxy uses>

I think we need that in the <service> interface class.

...re-security-keyvault-keys/src/main/java/com/azure/security/keyvault/keys/KeyAsyncClient.java

...yvault-keys/src/main/java/com/azure/security/keyvault/keys/implementation/KeyClientImpl.java

.../src/main/java/com/azure/security/keyvault/keys/implementation/KeyVaultErrorCodeStrings.java

…stack

…nd importKeyWithResponseAsync().

…ved them from all async methods.

…Applied formatting fixes.

vcolin7 · 2022-08-03T21:58:18Z

...ecurity-keyvault-keys/src/test/java/com/azure/security/keyvault/keys/KeyAsyncClientTest.java

        }
    }

+    private HttpClient buildAsyncAssertingClient(HttpClient httpClient) {


We shouldn't skip these tests. We want to make sure things are still working well with the current implementation. Can we get away with not using AssertingHttpClientBuilder until polling and paging are supported?

vcolin7 · 2022-08-03T22:06:34Z

...ure-security-keyvault-keys/src/test/java/com/azure/security/keyvault/keys/KeyClientTest.java

+            serviceVersion));
+    }
+
+    private HttpClient buildSyncAssertingClient(HttpClient httpClient) {


We shouldn't skip these tests. We want to make sure things are still working well with the current implementation. Can we get away with not using AssertingHttpClientBuilder until polling and paging are supported?

vcolin7 · 2022-08-03T22:36:13Z

...ure-security-keyvault-keys/src/test/java/com/azure/security/keyvault/keys/KeyClientTest.java

-        HttpPipeline httpPipeline = getHttpPipeline(httpClient, testTenantId);
+        HttpPipeline httpPipeline = getHttpPipeline(buildSyncAssertingClient(httpClient == null
+            ? interceptorManager.getPlaybackClient() : httpClient), testTenantId);
        KeyAsyncClient asyncClient = spy(new KeyClientBuilder()


We can remove this instance of KeyAsyncClient as it won't be used any longer. We should then use the spy() and when() methods on the KeyClientImpl from below.

Adding comment to request changes.

vcolin7 · 2022-08-03T23:15:22Z

...ure-security-keyvault-keys/src/test/java/com/azure/security/keyvault/keys/KeyClientTest.java

-        HttpPipeline httpPipeline = getHttpPipeline(httpClient, testTenantId);
+        HttpPipeline httpPipeline = getHttpPipeline(buildSyncAssertingClient(httpClient == null
+            ? interceptorManager.getPlaybackClient() : httpClient), testTenantId);
        KeyAsyncClient asyncClient = spy(new KeyClientBuilder()


Adding comment to request changes.

…n a KeyClientImpl instance.

…tests.

update

dddde11

ghost added the KeyVault label Jul 27, 2022

g2vinay requested review from alzimmermsft, kasobol-msft and samvaity July 27, 2022 19:21

g2vinay commented Jul 27, 2022

View reviewed changes

kasobol-msft reviewed Jul 27, 2022

View reviewed changes

...-security-keyvault-keys/src/main/java/com/azure/security/keyvault/keys/KeyClientBuilder.java Outdated Show resolved Hide resolved

update

0d283aa

g2vinay requested a review from vcolin7 July 27, 2022 21:14

update

4b4a258

g2vinay marked this pull request as ready for review July 27, 2022 21:30

alzimmermsft reviewed Jul 28, 2022

View reviewed changes

samvaity reviewed Jul 28, 2022

View reviewed changes

g2vinay added 2 commits August 1, 2022 21:23

update

e87bce3

update

c26b338

kasobol-msft approved these changes Aug 2, 2022

View reviewed changes

kasobol-msft mentioned this pull request Aug 2, 2022

Feature request: Support synchronous Http. Azure/autorest.java#1598

Closed

vcolin7 added 5 commits August 2, 2022 14:31

Replaced mentions of KeyAsyncClient with KeyClientImpl

7ad8d1e

Merge remote-tracking branch 'upstream/main' into update-kv-key-sync-…

abac9a8

…stack

Added missing logging statements in createRsaKeyWithResponseAsync() a…

ca58af7

…nd importKeyWithResponseAsync().

Ensured context null-checks are present for all sync methods and remo…

7f4d6b6

…ved them from all async methods.

Renamed keyClient to implClient in KeyAsyncClient and KeyClient. …

e184e2f

…Applied formatting fixes.

vcolin7 reviewed Aug 3, 2022

View reviewed changes

vcolin7 requested changes Aug 3, 2022

View reviewed changes

vcolin7 added 2 commits August 3, 2022 16:28

Removed getLogger() and getDefaultPollingInterval() from KeyAsyncClient.

c509e6c

Updated tests to correctly use Mockito's spy() and when() functions o…

f43442d

…n a KeyClientImpl instance.

vcolin7 approved these changes Aug 3, 2022

View reviewed changes

Removed unused import from KeyAsyncClient.

0e659f0

Changed config to open implementation package to unnamed modules for …

f68a2cb

…tests.

vcolin7 merged commit 3bcf41a into Azure:main Aug 4, 2022

Integrate Sync Stack on KV Keys #30162

Integrate Sync Stack on KV Keys #30162

Uh oh!

Conversation

g2vinay commented Jul 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azure-sdk commented Jul 27, 2022

Uh oh!

kasobol-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants