-
Notifications
You must be signed in to change notification settings - Fork 2k
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
dpg, upgrade webpubsub to 2022-11 #32125
dpg, upgrade webpubsub to 2022-11 #32125
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. |
...messaging-webpubsub/src/main/java/com/azure/messaging/webpubsub/WebPubSubServiceVersion.java
Show resolved
Hide resolved
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srnagar Do we need this kind of validation on client side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contentType
is an explicit param in the method, so, checking for null
is okay for explicit params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your efforts! The PR looks great! Added some little comments.
...ure-messaging-webpubsub/src/test/java/com/azure/messaging/webpubsub/TokenGenerationTest.java
Outdated
Show resolved
Hide resolved
...aging-webpubsub/src/test/java/com/azure/messaging/webpubsub/WebPubSubServiceClientTests.java
Show resolved
Hide resolved
assertResponse(client.sendToAllWithResponse( | ||
BinaryData.fromString("Hello World - Broadcast test!"), | ||
new RequestOptions().setHeader("Content-Type", "text/plain") | ||
.addQueryParam("filter", "userId ne 'user1'")), 202); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
...g-webpubsub/src/samples/java/com/azure/messaging/webpubsub/BroadcastingWithFilterSample.java
Outdated
Show resolved
Hide resolved
...g-webpubsub/src/samples/java/com/azure/messaging/webpubsub/BroadcastingWithFilterSample.java
Outdated
Show resolved
Hide resolved
### Features Added | ||
|
||
### Breaking Changes | ||
## 1.2.0 (Unreleased) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the release date too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I will update before release. Plan to release next week (after core release finished)
@@ -67,6 +70,9 @@ public Mono<WebPubSubClientAccessToken> getClientAccessToken(GetClientAccessToke | |||
if (CoreUtils.isNullOrEmpty(options.getRoles())) { | |||
requestOptions.addQueryParam("role", options.getRoles().stream().collect(Collectors.joining(","))); | |||
} | |||
if (CoreUtils.isNullOrEmpty(options.getGroups())) { | |||
requestOptions.addQueryParam("group", options.getGroups().stream().collect(Collectors.joining(","))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what if group name contains ,
, will it be escaped?
...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
...ging-webpubsub/src/main/java/com/azure/messaging/webpubsub/implementation/WebPubSubUtil.java
Outdated
Show resolved
Hide resolved
|
||
List<String> expectedRoles = Arrays.asList("a", "b"); | ||
// test for special char case | ||
List<String> expectedGroups = Arrays.asList("a&b", "c"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also tested when name contains &
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AAD credential mode. skipping PLAYBACK still needs to extends TestBase
?
9fd130c
to
11d5d4e
Compare
@@ -62,13 +62,12 @@ public WebPubSubClientAccessToken getClientAccessToken(GetClientAccessTokenOptio | |||
if (options.getExpiresAfter() != null) { | |||
requestOptions.addQueryParam("minutesToExpire", String.valueOf(options.getExpiresAfter().toMinutes())); | |||
} | |||
if (CoreUtils.isNullOrEmpty(options.getRoles())) { | |||
if (!CoreUtils.isNullOrEmpty(options.getRoles())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this. Can you also add some tests to ensure these cases are covered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Line 62 in 3d9be36
public void testTokenGenerationFromAadCredential(GetClientAccessTokenOptions tokenOptions, String connectionString, |
public void testTokenGenerationFromAadCredential() throws ParseException { | ||
@ParameterizedTest | ||
@MethodSource("getTokenOptions") | ||
@DoNotRecord(skipInPlayback = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remember to delete the json with token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
good job on the fix.
Co-authored-by: Weidong Xu <weidxu@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!
* regen and update source code * update tests to cover new features * update session-records * update test case * Fix checkstyle error * update serviceVersion * update according to comments * Add CHANGELOG * update cliam * update group claim * add sample on filters * fix the wrong version value for 2021-10-01 * update CHANGELOG * update pom and version_client * update pom.xml * update CHANGELOG * Fix spotbugs error * use codesnippet and update sample * nit: remove @generated * update samples according to comments * Remove hub param from removeConnectionFromAllGroupsWithResponse API * update according to comments * Fix existing bug on generate client token using Aad token * Add test * Fix checkstyle error * update CHANGELOG * Fix spotbugs error * update tests * Update according to comments * Use parameterized tests * Remove recording json for skipInPlayback test * remove unused imports * Update sdk/webpubsub/azure-messaging-webpubsub/CHANGELOG.md Co-authored-by: Weidong Xu <weidxu@microsoft.com> Co-authored-by: Weidong Xu <weidxu@microsoft.com>
Description
Fix #31975.
azure-rest-api-spec: Azure/azure-rest-api-specs#20762
Impl
classes under 'impl' folder are all generated.removeConnectionFromAllGroupsWithResponse
interface.group
for/api/hubs/{hub}/:generateToken
.""
as content type to pass non-null check inimpl
classes, I have checked in LIVE test that the content-type passed in through RequestOptions appears in the real HTTP request.WebPubSubServiceClient.getClientAccessToken()
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines