Fix issue with DotNet 8 when a traverser is empty

[MavroudisMant]

I found an issue with the driver when upgrading to .NET 8.0, caused from changes on how the Enumerator works.
Current is always in a state of throwing an exception if the Traversers are empty. Before MoveNext() is called it throws "Enumeration has not started. Call MoveNext." and after the call it throws "System.InvalidOperationException: Enumeration already finished."

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.70%. Comparing base (d9e34fb) to head (cf7274f).
Report is 97 commits behind head on 3.6-dev.

Additional details and impacted files

@@              Coverage Diff              @@
##             3.6-dev    #2738      +/-   ##
=============================================
+ Coverage      75.14%   75.70%   +0.56%     
- Complexity     12346    12369      +23     
=============================================
  Files           1058     1060       +2     
  Lines          63610    64410     +800     
  Branches        6962     6974      +12     
=============================================
+ Hits           47801    48763     +962     
+ Misses         13225    13053     -172     
- Partials        2584     2594      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kenhuuu]

Small nit: this should probably be one line to follow the style that exists in our cs files.

[MavroudisMant]

Updated this to follow the style on the rest of the files

[kenhuuu]

I'm not sure if returning null is the more proper behavior. Iterate() is called on the Traversal which will move to the end of the enumerator. Then Next() is called which I think would be reasonable to throw an exception.

[kenhuuu]

Hi and thank you for contributing.

As I mentioned in a comment in the test file, I'm not sure if this change produces the right behavior. @FlorianHockmann do you have an opinion on this?

[MavroudisMant]

Hello and thanks for taking the time to review this.

I run the above test on dotnet 6 without my change on MoveNext() and it produced null, so I tried to make it have the same behaviour on dotnet 8.
In our case when we upgraded our first issue was on a query like this one await g.V(id).Promise(t => t.Next()); when there wasn't a vertex with the given id.

[kenhuuu]

I run the above test on dotnet 6 without my change on MoveNext() and it produced null, so I tried to make it have the same behaviour on dotnet 8.

Yea, generally, this makes sense to me since our policy is to prevent breaking changes. However, in this specific case, that old behavior was kind of undefined behavior and we've been OK with changing it in 3.6-dev recently (see #2424 (comment))

I don't have a strong opinion on this topic because I'm not that familiar with C# so if no one else that is experienced in C# opposes it then I'm fine with supporting this PR as is.

[FlorianHockmann]

Sorry for the late response @MavroudisMant and thanks for reporting this and offering a PR!

However, I think that we shouldn't change this behavior. I tried to describe the situation in this JIRA issue: TINKERPOP-3072.

The short version is that Gremlin also throws an exception in other languages like Java and Python if next() is called on a traversal that doesn't return any elements. Java throws a nice NoSuchElementException which is of course more descriptive than System.InvalidOperationException: Enumeration already finished, but it doesn't just silently return null.

So, I think we can improve the situation in .NET by also throwing an exception with a clear error message to inform the user that there are no elements to return, but I don't think that we should return null here.

null could also be the valid result of a traversal which is something different than having a traversal which doesn't return anything but the user still calls Next() on it and therefore expects a result.

[kenhuuu]

Based on Florian's explanation of the situation, I think we should close this PR as we no longer want the Traversal to behave this way moving forward.