Skip to content

Commit 2359634

Browse files
BrennanConroyBrennanConroy
authored
C# and Java client negotiateVersion cleanup (#14196)
1 parent e361ca7 commit 2359634

File tree

9 files changed

+174
-91
lines changed

9 files changed

+174
-91
lines changed

‎src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.ConnectionLifecycle.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ public async Task SSEWaitsForResponseToStart()
359359
var httpHandler = new TestHttpMessageHandler();
360360

361361
var connectResponseTcs = new TaskCompletionSource<object>();
362-
httpHandler.OnGet("/?negotiateVersion=1&id=00000000-0000-0000-0000-000000000000", async (_, __) =>
362+
httpHandler.OnGet("/?id=00000000-0000-0000-0000-000000000000", async (_, __) =>
363363
{
364364
await connectResponseTcs.Task;
365365
return ResponseUtils.CreateResponse(HttpStatusCode.Accepted);

‎src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.Negotiate.cs

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ await WithConnectionAsync(
200200

201201
Assert.Equal("0rge0d00-0040-0030-0r00-000q00r00e00", connectionId);
202202
Assert.Equal("http://fakeuri.org/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[0].RequestUri.ToString());
203-
Assert.Equal("http://fakeuri.org/?negotiateVersion=1&id=different-id", testHttpHandler.ReceivedRequests[1].RequestUri.ToString());
203+
Assert.Equal("http://fakeuri.org/?id=different-id", testHttpHandler.ReceivedRequests[1].RequestUri.ToString());
204204
}
205205

206206
[Fact]
@@ -240,7 +240,7 @@ await WithConnectionAsync(
240240

241241
Assert.Equal("0rge0d00-0040-0030-0r00-000q00r00e00", connectionId);
242242
Assert.Equal("http://fakeuri.org/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[0].RequestUri.ToString());
243-
Assert.Equal("http://fakeuri.org/?negotiateVersion=1&id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[1].RequestUri.ToString());
243+
Assert.Equal("http://fakeuri.org/?id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[1].RequestUri.ToString());
244244
}
245245

246246
[Fact]
@@ -298,8 +298,8 @@ await WithConnectionAsync(
298298

299299
Assert.Equal("http://fakeuri.org/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[0].RequestUri.ToString());
300300
Assert.Equal("https://another.domain.url/chat/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[1].RequestUri.ToString());
301-
Assert.Equal("https://another.domain.url/chat?negotiateVersion=1&id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[2].RequestUri.ToString());
302-
Assert.Equal("https://another.domain.url/chat?negotiateVersion=1&id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[3].RequestUri.ToString());
301+
Assert.Equal("https://another.domain.url/chat?id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[2].RequestUri.ToString());
302+
Assert.Equal("https://another.domain.url/chat?id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[3].RequestUri.ToString());
303303
Assert.Equal(5, testHttpHandler.ReceivedRequests.Count);
304304
}
305305

@@ -402,11 +402,73 @@ await WithConnectionAsync(
402402
});
403403
}
404404

405+
Assert.Equal("http://fakeuri.org/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[0].RequestUri.ToString());
406+
Assert.Equal("https://another.domain.url/chat/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[1].RequestUri.ToString());
407+
Assert.Equal("https://another.domain.url/chat?id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[2].RequestUri.ToString());
408+
Assert.Equal("https://another.domain.url/chat?id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[3].RequestUri.ToString());
409+
// Delete request
410+
Assert.Equal(5, testHttpHandler.ReceivedRequests.Count);
411+
}
412+
413+
[Fact]
414+
public async Task NegotiateThatReturnsRedirectUrlDoesNotAddAnotherNegotiateVersionQueryString()
415+
{
416+
var testHttpHandler = new TestHttpMessageHandler(autoNegotiate: false);
417+
var negotiateCount = 0;
418+
testHttpHandler.OnNegotiate((request, cancellationToken) =>
419+
{
420+
negotiateCount++;
421+
if (negotiateCount == 1)
422+
{
423+
return ResponseUtils.CreateResponse(HttpStatusCode.OK,
424+
JsonConvert.SerializeObject(new
425+
{
426+
url = "https://another.domain.url/chat?negotiateVersion=1"
427+
}));
428+
}
429+
else
430+
{
431+
return ResponseUtils.CreateResponse(HttpStatusCode.OK,
432+
JsonConvert.SerializeObject(new
433+
{
434+
connectionId = "0rge0d00-0040-0030-0r00-000q00r00e00",
435+
availableTransports = new object[]
436+
{
437+
new
438+
{
439+
transport = "LongPolling",
440+
transferFormats = new[] { "Text" }
441+
},
442+
}
443+
}));
444+
}
445+
});
446+
447+
testHttpHandler.OnLongPoll((token) =>
448+
{
449+
var tcs = new TaskCompletionSource<HttpResponseMessage>(TaskCreationOptions.RunContinuationsAsynchronously);
450+
451+
token.Register(() => tcs.TrySetResult(ResponseUtils.CreateResponse(HttpStatusCode.NoContent)));
452+
453+
return tcs.Task;
454+
});
455+
456+
testHttpHandler.OnLongPollDelete((token) => ResponseUtils.CreateResponse(HttpStatusCode.Accepted));
457+
458+
using (var noErrorScope = new VerifyNoErrorsScope())
459+
{
460+
await WithConnectionAsync(
461+
CreateConnection(testHttpHandler, loggerFactory: noErrorScope.LoggerFactory),
462+
async (connection) =>
463+
{
464+
await connection.StartAsync().OrTimeout();
465+
});
466+
}
467+
405468
Assert.Equal("http://fakeuri.org/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[0].RequestUri.ToString());
406469
Assert.Equal("https://another.domain.url/chat/negotiate?negotiateVersion=1", testHttpHandler.ReceivedRequests[1].RequestUri.ToString());
407470
Assert.Equal("https://another.domain.url/chat?negotiateVersion=1&id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[2].RequestUri.ToString());
408471
Assert.Equal("https://another.domain.url/chat?negotiateVersion=1&id=0rge0d00-0040-0030-0r00-000q00r00e00", testHttpHandler.ReceivedRequests[3].RequestUri.ToString());
409-
// Delete request
410472
Assert.Equal(5, testHttpHandler.ReceivedRequests.Count);
411473
}
412474

‎src/SignalR/clients/csharp/Client/test/UnitTests/TestHttpMessageHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public static TestHttpMessageHandler CreateDefault()
120120
});
121121
testHttpMessageHandler.OnRequest((request, next, cancellationToken) =>
122122
{
123-
if (request.Method.Equals(HttpMethod.Delete) && request.RequestUri.PathAndQuery.Contains("&id="))
123+
if (request.Method.Equals(HttpMethod.Delete) && request.RequestUri.PathAndQuery.Contains("id="))
124124
{
125125
deleteCts.Cancel();
126126
return Task.FromResult(ResponseUtils.CreateResponse(HttpStatusCode.Accepted));

‎src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ public partial class HttpConnection : ConnectionContext, IConnectionInherentKeep
4242
private readonly HttpConnectionOptions _httpConnectionOptions;
4343
private ITransport _transport;
4444
private readonly ITransportFactory _transportFactory;
45-
private string _connectionToken;
4645
private string _connectionId;
4746
private readonly ConnectionLogScope _logScope;
4847
private readonly ILoggerFactory _loggerFactory;
@@ -343,7 +342,7 @@ private async Task SelectAndStartTransport(TransferFormat transferFormat, Cancel
343342
}
344343

345344
// This should only need to happen once
346-
var connectUrl = CreateConnectUrl(uri, _connectionToken);
345+
var connectUrl = CreateConnectUrl(uri, negotiationResponse.ConnectionToken);
347346

348347
// We're going to search for the transfer format as a string because we don't want to parse
349348
// all the transfer formats in the negotiation response, and we want to allow transfer formats
@@ -384,10 +383,10 @@ private async Task SelectAndStartTransport(TransferFormat transferFormat, Cancel
384383
if (negotiationResponse == null)
385384
{
386385
negotiationResponse = await GetNegotiationResponseAsync(uri, cancellationToken);
387-
connectUrl = CreateConnectUrl(uri, _connectionToken);
386+
connectUrl = CreateConnectUrl(uri, negotiationResponse.ConnectionToken);
388387
}
389388

390-
Log.StartingTransport(_logger, transportType, connectUrl);
389+
Log.StartingTransport(_logger, transportType, uri);
391390
await StartTransport(connectUrl, transportType, transferFormat, cancellationToken);
392391
break;
393392
}
@@ -430,7 +429,15 @@ private async Task<NegotiationResponse> NegotiateAsync(Uri url, HttpClient httpC
430429
urlBuilder.Path += "/";
431430
}
432431
urlBuilder.Path += "negotiate";
433-
var uri = Utils.AppendQueryString(urlBuilder.Uri, $"negotiateVersion={_protocolVersionNumber}");
432+
Uri uri;
433+
if (urlBuilder.Query.Contains("negotiateVersion"))
434+
{
435+
uri = urlBuilder.Uri;
436+
}
437+
else
438+
{
439+
uri = Utils.AppendQueryString(urlBuilder.Uri, $"negotiateVersion={_protocolVersionNumber}");
440+
}
434441

435442
using (var request = new HttpRequestMessage(HttpMethod.Post, uri))
436443
{
@@ -469,7 +476,7 @@ private static Uri CreateConnectUrl(Uri url, string connectionId)
469476
throw new FormatException("Invalid connection id.");
470477
}
471478

472-
return Utils.AppendQueryString(url, $"negotiateVersion={_protocolVersionNumber}&id=" + connectionId);
479+
return Utils.AppendQueryString(url, $"id={connectionId}");
473480
}
474481

475482
private async Task StartTransport(Uri connectUrl, HttpTransportType transportType, TransferFormat transferFormat, CancellationToken cancellationToken)
@@ -633,14 +640,10 @@ private async Task<NegotiationResponse> GetNegotiationResponseAsync(Uri uri, Can
633640
// If the negotiationVersion is greater than zero then we know that the negotiation response contains a
634641
// connectionToken that will be required to conenct. Otherwise we just set the connectionId and the
635642
// connectionToken on the client to the same value.
636-
if (negotiationResponse.Version > 0)
637-
{
638-
_connectionId = negotiationResponse.ConnectionId;
639-
_connectionToken = negotiationResponse.ConnectionToken;
640-
}
641-
else
643+
_connectionId = negotiationResponse.ConnectionId;
644+
if (negotiationResponse.Version == 0)
642645
{
643-
_connectionToken = _connectionId = negotiationResponse.ConnectionId;
646+
negotiationResponse.ConnectionToken = _connectionId;
644647
}
645648

646649
_logScope.ConnectionId = _connectionId;

‎src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ public class HubConnection {
5757
private Map<String, Observable> streamMap = new ConcurrentHashMap<>();
5858
private TransportEnum transportEnum = TransportEnum.ALL;
5959
private String connectionId;
60-
private String connectionToken;
6160
private final int negotiateVersion = 1;
6261
private final Logger logger = LoggerFactory.getLogger(HubConnection.class);
6362

@@ -262,7 +261,7 @@ private Single<NegotiateResponse> handleNegotiate(String url) {
262261
HttpRequest request = new HttpRequest();
263262
request.addHeaders(this.localHeaders);
264263

265-
return httpClient.post(Negotiate.resolveNegotiateUrl(url), request).map((response) -> {
264+
return httpClient.post(Negotiate.resolveNegotiateUrl(url, this.negotiateVersion), request).map((response) -> {
266265
if (response.getStatusCode() != 200) {
267266
throw new RuntimeException(String.format("Unexpected status code returned from negotiate: %d %s.",
268267
response.getStatusCode(), response.getStatusText()));
@@ -342,12 +341,11 @@ public Completable start() {
342341
});
343342

344343
stopError = null;
345-
String urlWithQS = Utils.appendQueryString(baseUrl, "negotiateVersion=" + negotiateVersion);
346344
Single<NegotiateResponse> negotiate = null;
347345
if (!skipNegotiate) {
348-
negotiate = tokenCompletable.andThen(Single.defer(() -> startNegotiate(urlWithQS, 0)));
346+
negotiate = tokenCompletable.andThen(Single.defer(() -> startNegotiate(baseUrl, 0)));
349347
} else {
350-
negotiate = tokenCompletable.andThen(Single.defer(() -> Single.just(new NegotiateResponse(urlWithQS))));
348+
negotiate = tokenCompletable.andThen(Single.defer(() -> Single.just(new NegotiateResponse(baseUrl))));
351349
}
352350

353351
CompletableSubject start = CompletableSubject.create();
@@ -449,21 +447,21 @@ private Single<NegotiateResponse> startNegotiate(String url, int negotiateAttemp
449447
throw new RuntimeException("There were no compatible transports on the server.");
450448
}
451449

450+
String connectionToken = "";
452451
if (response.getVersion() > 0) {
453452
this.connectionId = response.getConnectionId();
454-
this.connectionToken = response.getConnectionToken();
453+
connectionToken = response.getConnectionToken();
455454
} else {
456-
this.connectionToken = this.connectionId = response.getConnectionId();
455+
connectionToken = this.connectionId = response.getConnectionId();
457456
}
458457

459-
String finalUrl = Utils.appendQueryString(url, "id=" + this.connectionToken);
458+
String finalUrl = Utils.appendQueryString(url, "id=" + connectionToken);
460459

461460
response.setFinalUrl(finalUrl);
462461
return Single.just(response);
463462
}
464463

465-
String redirectUrl = Utils.appendQueryString(response.getRedirectUrl(), "negotiateVersion=" + negotiateVersion);
466-
return startNegotiate(redirectUrl, negotiateAttempts + 1);
464+
return startNegotiate(response.getRedirectUrl(), negotiateAttempts + 1);
467465
});
468466
}
469467

@@ -525,7 +523,6 @@ private void stopConnection(String errorMessage) {
525523
handshakeResponseSubject.onComplete();
526524
redirectAccessTokenProvider = null;
527525
connectionId = null;
528-
connectionToken = null;
529526
transportEnum = TransportEnum.ALL;
530527
this.localHeaders.clear();
531528
this.streamMap.clear();

‎src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/Negotiate.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
package com.microsoft.signalr;
55

66
class Negotiate {
7-
public static String resolveNegotiateUrl(String url) {
7+
public static String resolveNegotiateUrl(String url, int negotiateVersion) {
88
String negotiateUrl = "";
99

1010
// Check if we have a query string. If we do then we ignore it for now.
@@ -15,7 +15,7 @@ public static String resolveNegotiateUrl(String url) {
1515
negotiateUrl = url;
1616
}
1717

18-
//Check if the url ends in a /
18+
// Check if the url ends in a /
1919
if (negotiateUrl.charAt(negotiateUrl.length() - 1) != '/') {
2020
negotiateUrl += "/";
2121
}
@@ -27,6 +27,10 @@ public static String resolveNegotiateUrl(String url) {
2727
negotiateUrl += url.substring(queryStringIndex);
2828
}
2929

30+
if (!url.contains("negotiateVersion")) {
31+
negotiateUrl = Utils.appendQueryString(negotiateUrl, "negotiateVersion=" + negotiateVersion);
32+
}
33+
3034
return negotiateUrl;
3135
}
3236
}

0 commit comments

Comments
 (0)