[
https://issues.apache.org/jira/browse/TINKERPOP-2480?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17640641#comment-17640641
]
ASF GitHub Bot commented on TINKERPOP-2480:
-------------------------------------------
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('
', '_');
```
> User agent for Gremlin drivers
> ------------------------------
>
> Key: TINKERPOP-2480
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2480
> Project: TinkerPop
> Issue Type: Improvement
> Components: driver, server
> Affects Versions: 3.4.8
> Reporter: Divij Vaidya
> Priority: Minor
>
> Currently, a server does not distinguish amongst the different types of
> clients connecting to it. This issue is to add a new feature to add user
> agent field in the HTTP and WebSocket request header which could be used to
> identify the specific client from which the request was made.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
