FlorianHockmann commented on code in PR #1876:
URL: https://github.com/apache/tinkerpop/pull/1876#discussion_r1034639435
##########
gremlin-dotnet/src/Gremlin.Net/Process/Utils.cs:
##########
@@ -63,5 +66,23 @@ public static void Forget(this Task task)
t.Exception?.Handle(_ => true);
}, TaskContinuationOptions.ExecuteSynchronously);
}
+
+ /// <summary>
+ /// Returns a user agent for connection request headers.
+ ///
+ /// Format:
+ /// "[Application Name] [GLV Name].[Version] [Language Runtime
Version] [OS].[Version] [CPU Architecture]"
+ /// </summary>
+ private static string GenerateUserAgent()
+ {
+ var applicationName =
Assembly.GetExecutingAssembly().GetName().Name.Replace(' ', '_');
+ var driverVersion =
AssemblyName.GetAssemblyName("Gremlin.Net.dll")?.Version.ToString().Replace('
', '_');
+ var languageVersion = Environment.Version.ToString().Replace(' ',
'_');
+ var osName = Environment.OSVersion.Platform.ToString().Replace('
', '_');
+ var osVersion = Environment.OSVersion.Version.ToString().Replace('
', '_');
+ var cpuArchitecture =
System.Runtime.InteropServices.RuntimeInformation.OSArchitecture.ToString().Replace('
', '_');
Review Comment:
(nitpick) Please wrap this long line:
```suggestion
var cpuArchitecture =
System.Runtime.InteropServices.RuntimeInformation.OSArchitecture.ToString()
.Replace(' ', '_');
```
or import this static classes so they don't need to be written out here with
there namespaces.
##########
gremlin-dotnet/src/Gremlin.Net/Process/Utils.cs:
##########
@@ -32,6 +33,8 @@ namespace Gremlin.Net.Process
/// </summary>
internal static class Utils
{
+ public static readonly string UserAgent = GenerateUserAgent();
Review Comment:
A public field is usually discouraged in C#. It isn't a big problem here as
this class is `internal`, but I would still suggest that only make properties
and methods `public`. My suggestion is to turn this into a simple property with
a backing field:
```suggestion
public static string UserAgent => _userAgent ??= GenerateUserAgent();
private static string _userAgent;
```
##########
CHANGELOG.asciidoc:
##########
@@ -35,6 +35,7 @@
image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Dockerized all test environment for .NET, JavaScript, Python, Go, and
Python-based tests for Console, and added Docker as a build requirement.
* Async operations in .NET can now be cancelled. This however does not cancel
work that is already happening on the server.
* Bumped to `snakeyaml` 1.32 to fix security vulnerability.
+* Added user agent to web socket handshake in dotnet driver. Can be controlled
by EnableUserAgentOnConnect in `ConnectionPoolSettings`. It is enabled by
default.
Review Comment:
Please either change this from _dotnet driver_ to _the Gremlin.Net driver_
or _the .NET driver_.
And _EnableUserAgentOnConnect_ should also be marked as code, so:
\`EnableUserAgentOnConnect\`
##########
gremlin-dotnet/src/Gremlin.Net/Process/Utils.cs:
##########
@@ -63,5 +66,23 @@ public static void Forget(this Task task)
t.Exception?.Handle(_ => true);
}, TaskContinuationOptions.ExecuteSynchronously);
}
+
+ /// <summary>
+ /// Returns a user agent for connection request headers.
+ ///
+ /// Format:
+ /// "[Application Name] [GLV Name].[Version] [Language Runtime
Version] [OS].[Version] [CPU Architecture]"
+ /// </summary>
+ private static string GenerateUserAgent()
+ {
+ var applicationName =
Assembly.GetExecutingAssembly().GetName().Name.Replace(' ', '_');
Review Comment:
`Name` could be `null` which would throw a `NullReferenceException`. Please
use conditional access:
```suggestion
var applicationName =
Assembly.GetExecutingAssembly().GetName().Name?.Replace(' ', '_');
```
That way, `applicationName` will be `null` which isn't that nice, but at
least better than getting an exception in my opinion.
##########
gremlin-dotnet/src/Gremlin.Net/Driver/WebSocketSettings.cs:
##########
@@ -22,6 +22,7 @@
#endregion
using System;
+using System.ComponentModel;
Review Comment:
Remove unused import
##########
gremlin-dotnet/src/Gremlin.Net/Process/Utils.cs:
##########
@@ -63,5 +66,23 @@ public static void Forget(this Task task)
t.Exception?.Handle(_ => true);
}, TaskContinuationOptions.ExecuteSynchronously);
}
+
+ /// <summary>
+ /// Returns a user agent for connection request headers.
+ ///
+ /// Format:
+ /// "[Application Name] [GLV Name].[Version] [Language Runtime
Version] [OS].[Version] [CPU Architecture]"
+ /// </summary>
+ private static string GenerateUserAgent()
+ {
+ var applicationName =
Assembly.GetExecutingAssembly().GetName().Name.Replace(' ', '_');
+ var driverVersion =
AssemblyName.GetAssemblyName("Gremlin.Net.dll")?.Version.ToString().Replace('
', '_');
Review Comment:
`GetAssemblyName()` can't return `null` if I'm not mistaken. It will instead
throw if it cannot find the assembly (which should never happen), but `Version`
can be `null`. So please change this to:
```suggestion
var driverVersion =
AssemblyName.GetAssemblyName("Gremlin.Net.dll").Version?.ToString().Replace('
', '_');
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
