[CSHARP-1186] LINQ skip implementation is not consistent with native linq to objects Created: 02/Mar/15  Updated: 03/Dec/20  Resolved: 03/Dec/20

Status: Closed
Project: C# Driver
Component/s: Linq
Affects Version/s: 1.9.2
Fix Version/s: None

Type: Bug Priority: Trivial - P5
Reporter: Scott Guymer Assignee: Unassigned
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

When using the skip method with a negative number the mongo driver raises the following exception

ArgumentException: Skip cannot be negative.

However this is not consistent with the linq to objects implementation and therefore makes for the possibility of runtime errors slipping through unit tests when linq is using the mongodb driver.



 Comments   
Comment by Rahman Mukras [ 09/Mar/15 ]

Thanks for this Craig,
I will move and try to find another ticket to work on.

Comment by Craig Wilson [ 09/Mar/15 ]

Perhaps you're right. The LINQ provider may end up calling here. If that's the case, we can't change this behavior as it's the correct behavior in one instance and the "wrong" behavior in another.

Comment by Rahman Mukras [ 09/Mar/15 ]

Hi Craig,
There was only one instance of "Skip cannot be negative." in the 1.x code base, and it matched the symptoms exactly so I did not think otherwise.

I am at odds at where else to look, and any pointers would be appreciated...

Comment by Craig Wilson [ 09/Mar/15 ]

Hi Rahman,

This code is not the problem. Since MongoDB doesn't support negative skips, we really should be throwing in this case. This ticket is about the LINQ implementation that does throw where LINQ to objects does not throw. That code will be in a different place.

Craig

Comment by Rahman Mukras [ 09/Mar/15 ]

I wanted to pick this nice and small issue just to get started, but could not assign it to myself and so I could not assume that no one has started work on it.

Is anyone working on this?

In any case, here are the two diffs of the test and code that I plan to submit.

+        [Test]
+        public void TestSetSkip()
+        {
+            _collection.RemoveAll();
+            _collection.Insert(new BsonDocument { { "x", 1 }, { "y", 2 } });
+            _collection.Insert(new BsonDocument { { "x", 2 }, { "y", 2 } });
+            _collection.Insert(new BsonDocument { { "x", 3 }, { "y", 2 } });
+
+            var result = _collection.FindAll().SetSkip(0).SetSkip(-1).SetSkip(1).Select(x => x["x"].AsInt32).ToList();
+            Assert.AreEqual(2, result.Count());
+            CollectionAssert.AreEqual(new[] { 2, 3 }, result);
+        }

         public virtual MongoCursor SetSkip(int skip)
         {
             if (_isFrozen) { ThrowFrozen(); }
-            if (skip < 0) { throw new ArgumentException("Skip cannot be negative."); }
+            if (skip < 0) { skip = 0; }
             _skip = skip;
             return this;
         }

Comment by Craig Wilson [ 02/Mar/15 ]

Our process is pretty loose. Unfortunately, we have a number of PRs sitting there and haven't managed that backlog very well. We are deep into trying to get 2.0.0 out and this is going to slip through the cracks. If you'd like to submit a PR, submit against 1.x and put a link in this ticket and we'll get to it once 2.0.0 is released.

Thanks.

Comment by Scott Guymer [ 02/Mar/15 ]

Im more than happy to make the fix and submit the PR myself.

Although i admit i havent read your process for allowing people to contribute yet!

Comment by Scott Guymer [ 02/Mar/15 ]

Thanks for this Craig

Totally understand what you are saying about unit tests but testing against Mongo would be part of our integrations tests rather than unit testing the logic in the code.

The issue was more that is was less the fact that it wasnt supported by the driver but more that it was raising an unexpected exception at run time.

Comment by Craig Wilson [ 02/Mar/15 ]

You're right. It looks like Linq to Objects treats negative skips as 0.

// System.Linq.Enumerable
private static IEnumerable<TSource> SkipIterator<TSource>(IEnumerable<TSource> source, int count)
{
	using (IEnumerator<TSource> enumerator = source.GetEnumerator())
	{
		while (count > 0 && enumerator.MoveNext())
		{
			count--;
		}
		if (count <= 0)
		{
			while (enumerator.MoveNext())
			{
				yield return enumerator.Current;
			}
		}
	}
	yield break;
}

So, I'll re-open this and we'll discuss changing our behavior. I'd like to caution you that writing unit tests against LINQ to Objects and then running your code against MongoDB (or any other queryable provider) will probably end up badly because no provider will support the full breadth of the possible operators and methods.

Comment by Scott Guymer [ 02/Mar/15 ]

Dont think its a case of supporting negative skips - im not sure in what circumstance it would be needed

Its more of a case that linq to objects seems to absorb the problem (maybe transparently setting to 0) where as the mongo implementation throws an exception that you cant test for in a unit test with going full on integration test.

I havent investigated the .net core code to see what it does but it doesnt throw an exception.

Comment by Craig Wilson [ 02/Mar/15 ]

Hi Scott,

Thanks for the report. While we do strive to be consistent with Linq to Objects as much as possible, there are a great number of things that simply aren't supported at all. LINQ is a great big leaky abstraction since not all data sources provide the same features. This is one of those. MongoDB simply doesn't support negative skips.

I'm closing this as won't fix. Feel free to respond if you feel this is a mistake and how you think we could support this feature.

Thanks,
Craig

Generated at Wed Feb 07 21:38:54 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.