Skip to content

[Java] Fix skip negotiate null ref #30574

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

Merged
merged 4 commits into from
Mar 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/SignalR/build.cmd
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
@ECHO OFF
SET RepoRoot=%~dp0..\..
%RepoRoot%\eng\build.cmd -buildJava -projects %~dp0**\*.*proj %*
%RepoRoot%\eng\build.cmd -nobuildnative -buildJava -projects %~dp0**\*.*proj %*
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ public HttpHubConnectionBuilder withHubProtocol(HubProtocol protocol) {

/**
* Indicates to the {@link HubConnection} that it should skip the negotiate process.
* Note: This option only works with the Websockets transport and the Azure SignalR Service require the negotiate step.
* Note: This option only works with the {@link TransportEnum#WEBSOCKETS} transport selected via {@link #withTransport(TransportEnum) withTransport},
* additionally the Azure SignalR Service requires the negotiate step so this will fail when using the Azure SignalR Service.
*
* @param skipNegotiate Boolean indicating if the {@link HubConnection} should skip the negotiate step.
* @return This instance of the HttpHubConnectionBuilder.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,16 @@ public Completable start() {
Transport transport = customTransport;
if (transport == null) {
Single<String> tokenProvider = negotiateResponse.getAccessToken() != null ? Single.just(negotiateResponse.getAccessToken()) : accessTokenProvider;
switch (negotiateResponse.getChosenTransport()) {
TransportEnum chosenTransport;
if (this.skipNegotiate) {
if (this.transportEnum != TransportEnum.WEBSOCKETS) {
throw new RuntimeException("Negotiation can only be skipped when using the WebSocket transport directly with '.withTransport(TransportEnum.WEBSOCKETS)' on the 'HubConnectionBuilder'.");
}
Copy link
Member

Choose a reason for hiding this comment

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

You must have removed this from #30482 after I approved it. Could it break any previously-working code?

Copy link
Member Author

@BrennanConroy BrennanConroy Mar 2, 2021

Choose a reason for hiding this comment

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

Yeah, if you didn't specify a transport and used skip negotiate, it would work because WebSockets will be used first. So adding that check would have broken people that had it working but didn't explicitly specify the transport.

chosenTransport = this.transportEnum;
} else {
chosenTransport = negotiateResponse.getChosenTransport();
}
switch (chosenTransport) {
case LONG_POLLING:
transport = new LongPollingTransport(localHeaders, httpClient, tokenProvider);
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2743,6 +2743,63 @@ public void noConnectionIdWhenSkippingNegotiate() {
assertNull(hubConnection.getConnectionId());
}

@Test
public void SkippingNegotiateDoesNotNegotiate() {
try (TestLogger logger = new TestLogger(WebSocketTransport.class.getName())) {
AtomicBoolean negotiateCalled = new AtomicBoolean(false);
TestHttpClient client = new TestHttpClient().on("POST", "http://example.com/negotiate?negotiateVersion=1",
(req) -> {
negotiateCalled.set(true);
return Single.just(new HttpResponse(200, "",
TestUtils.stringToByteBuffer("{\"connectionId\":\"bVOiRPG8-6YiJ6d7ZcTOVQ\",\""
+ "availableTransports\":[{\"transport\":\"WebSockets\",\"transferFormats\":[\"Text\",\"Binary\"]}]}")));
});

HubConnection hubConnection = HubConnectionBuilder
.create("http://example")
.withTransport(TransportEnum.WEBSOCKETS)
.shouldSkipNegotiate(true)
.withHttpClient(client)
.build();

try {
hubConnection.start().timeout(30, TimeUnit.SECONDS).blockingAwait();
} catch (Exception e) {
assertEquals("WebSockets isn't supported in testing currently.", e.getMessage());
}
assertEquals(HubConnectionState.DISCONNECTED, hubConnection.getConnectionState());
assertFalse(negotiateCalled.get());

logger.assertLog("Starting Websocket connection.");
}
}

@Test
public void ThrowsIfSkipNegotiationSetAndTransportIsNotWebSockets() {
AtomicBoolean negotiateCalled = new AtomicBoolean(false);
TestHttpClient client = new TestHttpClient().on("POST", "http://example.com/negotiate?negotiateVersion=1",
(req) -> {
negotiateCalled.set(true);
return Single.just(new HttpResponse(200, "",
TestUtils.stringToByteBuffer("{\"connectionId\":\"bVOiRPG8-6YiJ6d7ZcTOVQ\",\""
+ "availableTransports\":[{\"transport\":\"WebSockets\",\"transferFormats\":[\"Text\",\"Binary\"]}]}")));
});

HubConnection hubConnection = HubConnectionBuilder
.create("http://example")
.shouldSkipNegotiate(true)
.withHttpClient(client)
.build();

try {
hubConnection.start().timeout(30, TimeUnit.SECONDS).blockingAwait();
} catch (Exception e) {
assertEquals("Negotiation can only be skipped when using the WebSocket transport directly with '.withTransport(TransportEnum.WEBSOCKETS)' on the 'HubConnectionBuilder'.", e.getMessage());
}
assertEquals(HubConnectionState.DISCONNECTED, hubConnection.getConnectionState());
assertFalse(negotiateCalled.get());
}

@Test
public void connectionIdIsAvailableAfterStart() {
TestHttpClient client = new TestHttpClient().on("POST", "http://example.com/negotiate?negotiateVersion=1",
Expand Down