dpg, upgrade webpubsub to 2022-11 by haolingdong-msft · Pull Request #32125 · Azure/azure-sdk-for-java

haolingdong-msft · 2022-11-14T06:25:33Z

Description

Fix #31975.
azure-rest-api-spec: Azure/azure-rest-api-specs#20762

Regen and update source code. Highlights:
- Regen based on new API version.
- The Impl classes under 'impl' folder are all generated.
- Add removeConnectionFromAllGroupsWithResponse interface.
- Support query group for /api/hubs/{hub}/:generateToken.
- use empty string "" as content type to pass non-null check in impl classes, I have checked in LIVE test that the content-type passed in through RequestOptions appears in the real HTTP request.
Add related tests.
Add samples on filters.
Update CHANGELOG, readme.md and pom.xml.
Existing bug fix on WebPubSubServiceClient.getClientAccessToken()

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

…2-11

azure-sdk · 2022-11-14T06:31:17Z

API change check

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

azure-messaging-webpubsub

...messaging-webpubsub/src/main/java/com/azure/messaging/webpubsub/WebPubSubServiceVersion.java

haolingdong-msft · 2022-11-16T07:16:35Z

...ing-webpubsub/src/main/java/com/azure/messaging/webpubsub/implementation/WebPubSubsImpl.java

        if (hub == null) {
            return Mono.error(new IllegalArgumentException("Parameter hub is required and cannot be null."));
        }
        if (connectionId == null) {
            return Mono.error(new IllegalArgumentException("Parameter connectionId is required and cannot be null."));
        }
+        if (contentType == null) {


@srnagar Do we need this kind of validation on client side?

contentType is an explicit param in the method, so, checking for null is okay for explicit params.

ok, it's fine, the reason I ask this is that: I use empty string "" as content type to pass this non-null check (I have checked in LIVE test that the content-type passed in through RequestOptions appears in the real HTTP request). So would like to make sure if we really need this validation.

vicancy

Thanks for your efforts! The PR looks great! Added some little comments.

...ure-messaging-webpubsub/src/test/java/com/azure/messaging/webpubsub/TokenGenerationTest.java

...aging-webpubsub/src/test/java/com/azure/messaging/webpubsub/WebPubSubServiceClientTests.java

vicancy · 2022-12-16T07:52:42Z

...aging-webpubsub/src/test/java/com/azure/messaging/webpubsub/WebPubSubServiceClientTests.java

+        assertResponse(client.sendToAllWithResponse(
+            BinaryData.fromString("Hello World - Broadcast test!"),
+            new RequestOptions().setHeader("Content-Type", "text/plain")
+                .addQueryParam("filter", "userId ne 'user1'")), 202);


How about adding a ClientConnectionFilter helper class, as similar to what azure search does with its SearchFilter

A similar approach in c# is here https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/webpubsub/Azure.Messaging.WebPubSub/src/ClientConnectionFilter.cs

sdk/webpubsub/azure-messaging-webpubsub/CHANGELOG.md

sdk/webpubsub/azure-messaging-webpubsub/swagger/README.md

…2-11

sdk/webpubsub/azure-messaging-webpubsub/CHANGELOG.md

sdk/webpubsub/azure-messaging-webpubsub/README.md

...g-webpubsub/src/samples/java/com/azure/messaging/webpubsub/BroadcastingWithFilterSample.java

srnagar · 2023-01-04T17:40:50Z

sdk/webpubsub/azure-messaging-webpubsub/CHANGELOG.md

-### Features Added
-
-### Breaking Changes
+## 1.2.0 (Unreleased)


Please update the release date too.

sure, I will update before release. Plan to release next week (after core release finished)

vicancy · 2023-01-05T03:16:15Z

...aging-webpubsub/src/main/java/com/azure/messaging/webpubsub/WebPubSubServiceAsyncClient.java

                requestOptions.addQueryParam("role", options.getRoles().stream().collect(Collectors.joining(",")));
            }
+            if (CoreUtils.isNullOrEmpty(options.getGroups())) {
+                requestOptions.addQueryParam("group", options.getGroups().stream().collect(Collectors.joining(",")));


question: what if group name contains ,, will it be escaped?

sdk/webpubsub/azure-messaging-webpubsub/README.md

...ging-webpubsub/src/main/java/com/azure/messaging/webpubsub/implementation/WebPubSubUtil.java

...-messaging-webpubsub/src/main/java/com/azure/messaging/webpubsub/WebPubSubServiceClient.java

...ging-webpubsub/src/main/java/com/azure/messaging/webpubsub/implementation/WebPubSubUtil.java

haolingdong-msft · 2023-01-05T09:31:19Z

...ure-messaging-webpubsub/src/test/java/com/azure/messaging/webpubsub/TokenGenerationTest.java

+
+        List<String> expectedRoles = Arrays.asList("a", "b");
+        // test for special char case
+        List<String> expectedGroups = Arrays.asList("a&b", "c");


Also tested when name contains &

This test needs to use real webpubsub endpoint to generate client token in LIVE/RECORD mode and use recorded json in PLAYBACK mode. So I make TokenGenerationTest extends TestBase. But this will cause the existing parameterized tokenGeneration tests all needs playback file, otherwise it will have below error. Please let me know if you have any concern or suggestion.
java.lang.RuntimeException: Missing both new and old playback files

After the changes, does it test the key credential mode, or the AAD credential mode?

You can always skip PLAYBACK, if you only want to have LIVE tests.

AAD credential mode. skipping PLAYBACK still needs to extends TestBase?

srnagar · 2023-01-05T17:46:08Z

...-messaging-webpubsub/src/main/java/com/azure/messaging/webpubsub/WebPubSubServiceClient.java

                requestOptions.addQueryParam("minutesToExpire", String.valueOf(options.getExpiresAfter().toMinutes()));
            }
-            if (CoreUtils.isNullOrEmpty(options.getRoles())) {
+            if (!CoreUtils.isNullOrEmpty(options.getRoles())) {


Thanks for fixing this. Can you also add some tests to ensure these cases are covered?

Tests are added. In the test, I tested generate token from Azure AD token credential, and use the existing test data which includes setting roles and groups, and I also add data on special characters like & and ,. /cc @vicancy

azure-sdk-for-java/sdk/webpubsub/azure-messaging-webpubsub/src/test/java/com/azure/messaging/webpubsub/TokenGenerationTest.java

Line 62 in 3d9be36

public void testTokenGenerationFromAadCredential(GetClientAccessTokenOptions tokenOptions, String connectionString,

weidongxu-microsoft · 2023-01-06T03:21:15Z

...ure-messaging-webpubsub/src/test/java/com/azure/messaging/webpubsub/TokenGenerationTest.java

-    public void testTokenGenerationFromAadCredential() throws ParseException {
+    @ParameterizedTest
+    @MethodSource("getTokenOptions")
+    @DoNotRecord(skipInPlayback = true)


remember to delete the json with token.

weidongxu-microsoft

LGTM

good job on the fix.

sdk/webpubsub/azure-messaging-webpubsub/CHANGELOG.md

Co-authored-by: Weidong Xu <[email protected]>

vicancy

Awesome work!

haolingdong-msft added 4 commits November 14, 2022 14:01

regen and update source code

582a155

update tests to cover new features

fe08c54

update session-records

82ec95b

Merge remote-tracking branch 'remote/main' into webpubsub-upgrade-202…

2cdc5ae

…2-11

haolingdong-msft added 2 commits November 14, 2022 14:34

update test case

d6bb162

Fix checkstyle error

2c7a93f

srnagar reviewed Nov 15, 2022

View reviewed changes

...messaging-webpubsub/src/main/java/com/azure/messaging/webpubsub/WebPubSubServiceVersion.java Show resolved Hide resolved

haolingdong-msft added 3 commits November 16, 2022 10:59

update serviceVersion

ac09258

update according to comments

233810c

Add CHANGELOG

7665d36

haolingdong-msft commented Nov 16, 2022

View reviewed changes

Merge branch 'main' into webpubsub-upgrade-2022-11

eff6695

haolingdong-msft requested a review from vicancy December 16, 2022 07:26

update cliam

bbd43d8

vicancy reviewed Dec 16, 2022

View reviewed changes

update group claim

374ffff

weidongxu-microsoft reviewed Dec 19, 2022

View reviewed changes

sdk/webpubsub/azure-messaging-webpubsub/swagger/README.md Outdated Show resolved Hide resolved

haolingdong-msft mentioned this pull request Dec 19, 2022

Add e2e test for WebPubSub #32643

Closed

2 tasks

haolingdong-msft added 8 commits December 19, 2022 17:43

add sample on filters

5fe90e7

fix the wrong version value for 2021-10-01

7c9e24c

update CHANGELOG

497c56d

update pom and version_client

76b3857

update pom.xml

e8baf1e

Merge remote-tracking branch 'remote/main' into webpubsub-upgrade-202…

81ffefe

…2-11

update CHANGELOG

fb3689a

Fix spotbugs error

33448fa

vicancy reviewed Dec 20, 2022

View reviewed changes

use codesnippet and update sample

6f4c306

haolingdong-msft marked this pull request as ready for review December 20, 2022 07:13

weidongxu-microsoft reviewed Jan 3, 2023

View reviewed changes

...g-webpubsub/src/samples/java/com/azure/messaging/webpubsub/BroadcastingWithFilterSample.java Outdated Show resolved Hide resolved

update samples according to comments

0b8cb73

weidongxu-microsoft approved these changes Jan 3, 2023

View reviewed changes

Remove hub param from removeConnectionFromAllGroupsWithResponse API

dfdda8e

srnagar approved these changes Jan 4, 2023

View reviewed changes

srnagar reviewed Jan 4, 2023

View reviewed changes

vicancy reviewed Jan 5, 2023

View reviewed changes

haolingdong-msft added 5 commits January 5, 2023 14:56

update according to comments

c60fc7a

Fix existing bug on generate client token using Aad token

40d174d

Add test

b88b079

Fix checkstyle error

9e50e14

update CHANGELOG

4b49bd5

haolingdong-msft commented Jan 5, 2023

View reviewed changes

...ging-webpubsub/src/main/java/com/azure/messaging/webpubsub/implementation/WebPubSubUtil.java Show resolved Hide resolved

...-messaging-webpubsub/src/main/java/com/azure/messaging/webpubsub/WebPubSubServiceClient.java Show resolved Hide resolved

Fix spotbugs error

508cfe2

weidongxu-microsoft reviewed Jan 5, 2023

View reviewed changes

...ging-webpubsub/src/main/java/com/azure/messaging/webpubsub/implementation/WebPubSubUtil.java Outdated Show resolved Hide resolved

haolingdong-msft commented Jan 5, 2023

View reviewed changes

haolingdong-msft added 2 commits January 5, 2023 17:43

update tests

a206209

Update according to comments

11d5d4e

haolingdong-msft force-pushed the webpubsub-upgrade-2022-11 branch from 9fd130c to 11d5d4e Compare January 5, 2023 09:51

srnagar approved these changes Jan 5, 2023

View reviewed changes

Use parameterized tests

01f6b3c

weidongxu-microsoft reviewed Jan 6, 2023

View reviewed changes

haolingdong-msft added 2 commits January 6, 2023 11:31

Remove recording json for skipInPlayback test

8aa9442

remove unused imports

b1647a7

weidongxu-microsoft approved these changes Jan 6, 2023

View reviewed changes

weidongxu-microsoft reviewed Jan 6, 2023

View reviewed changes

sdk/webpubsub/azure-messaging-webpubsub/CHANGELOG.md Outdated Show resolved Hide resolved

Update sdk/webpubsub/azure-messaging-webpubsub/CHANGELOG.md

3d9be36

Co-authored-by: Weidong Xu <[email protected]>

vicancy approved these changes Jan 6, 2023

View reviewed changes

haolingdong-msft merged commit 4876ed3 into Azure:main Jan 6, 2023

Conversation

haolingdong-msft commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

All SDK Contribution checklist:

Uh oh!

azure-sdk commented Nov 14, 2022

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!

vicancy left a comment

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haolingdong-msft Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haolingdong-msft Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weidongxu-microsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vicancy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

haolingdong-msft commented Nov 14, 2022 •

edited

Loading

haolingdong-msft Jan 6, 2023 •

edited

Loading

haolingdong-msft Jan 6, 2023 •

edited

Loading