Skip to content
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.

Sign up for GitHub

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

Fix ping in SDK and add user agent headers #3672

Merged
merged 5 commits into from
Jan 13, 2025
Merged

Fix ping in SDK and add user agent headers #3672

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bd1ead7
ping should ping a static asset and add user agent headers
adamhathcock Jan 10, 2025
f52f424
fix: fmt
didimitrie Jan 10, 2025
516b54d
Removed frontend headers check for FE1 servers
JR-Morgan Jan 13, 2025
751f1d0
Removed another test that relied on this pinging
JR-Morgan Jan 13, 2025
b0b7f0b
Removed frontend headers check for FE1 servers (#3673)
JR-Morgan Jan 13, 2025
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
10 changes: 9 additions & 1 deletion Core/Core/Helpers/Http.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public static async Task<HttpResponseMessage> HttpPing(Uri uri)
try
{
using var httpClient = GetHttpProxyClient();
HttpResponseMessage response = await httpClient.GetAsync(uri).ConfigureAwait(false);
HttpResponseMessage response = await httpClient.GetAsync(GetPingUrl(uri)).ConfigureAwait(false);
response.EnsureSuccessStatusCode();
SpeckleLog.Logger.Information("Successfully pinged {uri}", uri);
return response;
Copy link
Member

Choose a reason for hiding this comment

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

This function is used by IsFrontend2 to determine if the server supports FE2.
This PR breaks this, because /favicon.ico doesn't have the FE headers its expecting.

I've checked with @gjedlicska, and FE1 is now deprecated, we can safely assume now that all servers are FE2. (I.e. always set Fe2 = true on accounts, rather than fetch the headers.

Expand All @@ -155,6 +155,12 @@ public static async Task<HttpResponseMessage> HttpPing(Uri uri)
}
}

public static Uri GetPingUrl(Uri serverUrl)
{
var server = serverUrl.GetLeftPart(UriPartial.Authority);
return new Uri(new(server), "/favicon.ico");
}

public static HttpClient GetHttpProxyClient(SpeckleHttpClientHandler? speckleHttpClientHandler = null)
{
IWebProxy proxy = WebRequest.GetSystemWebProxy();
Expand All @@ -166,6 +172,8 @@ public static HttpClient GetHttpProxyClient(SpeckleHttpClientHandler? speckleHtt
{
Timeout = Timeout.InfiniteTimeSpan //timeout is configured on the SpeckleHttpClientHandler through policy
};
client.DefaultRequestHeaders.UserAgent.Clear();
client.DefaultRequestHeaders.UserAgent.Add(new("SpeckleSDK", "2.0.0"));
return client;
}

Expand Down
11 changes: 11 additions & 0 deletions Core/Tests/Speckle.Core.Tests.Unit/Helpers/Path.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,17 @@ namespace Speckle.Core.Tests.Unit.Helpers;
[TestOf(nameof(SpecklePathProvider))]
public class SpecklePathTests
{
[Test]
[TestCase("https://app.speckle.systems", "https://app.speckle.systems/favicon.ico")]
[TestCase("https://app.speckle.systems/", "https://app.speckle.systems/favicon.ico")]
[TestCase("https://app.speckle.systems/auth/login", "https://app.speckle.systems/favicon.ico")]
[TestCase("https://latest.speckle.systems", "https://latest.speckle.systems/favicon.ico")]
public void TestGetPingUrl(string given, string expected)
{
var x = Http.GetPingUrl(new Uri(given));
Assert.That(x, Is.EqualTo(new Uri(expected)));
}

[Test]
public void TestUserApplicationDataPath()
{
Expand Down