Possible bug in QueryModel.Clone
Gordon Watts
Hi,
I have a possible bug in QueryModel.Clone. I wrote a small test case, and put it into the re-linq source (you can see the commit to a fork here: https://github.com/gordonwatts/Relinq/commit/06ba52ab38dbbbcaf22633a0227ab72738a58d37).
The case looks like the following:
[Test]
public void Clone_AgreegateBomb()
{
var q1 = ExpressionHelper.CreateQueryable<int[]>();
var rInt = from e in q1
select e.Count();
var r = rInt.Aggregate(0.0, (acc, val) => acc + val);
var qm = StubQueryExecutor.LastQM;
qm.Clone();
}
[I made a minor modification to StubQueryExecutor b.c. I am a bit too lazy to construct the QM from scratch – I get that this makes this closer to an integration test, but… The modification is also part of the above commit.]
The exception occurs here:
public void AddMapping (IQuerySource querySource, Expression expression)
{
ArgumentUtility.CheckNotNull ("querySource", querySource);
ArgumentUtility.CheckNotNull ("expression", expression);
try
{
_lookup.Add (querySource, expression);
}
catch (ArgumentException)
{
throw new InvalidOperationException (string.Format ("Query source ({0}) has already been associated with an expression.", querySource));
}
}
In the source code QuerySourceMapping.cs. Of course, given the nature of this error, I suspect the actual bug is far away from here! 😊
Cheers,
Gordon
Gordon Watts
Hi,
After our other conversations I put a few things together on my way into work this morning. I think the problem with the Clone statement I’m seeing is that the MainFromClause can reference another MainFromClause in the query. You don’t want to clone both of those – in short, MainFromClause’s Clone shouldn’t always clone the from clause.
So, I added a little code to get around this, and it looks like it does the job (and all other tests I was able to run, run). The code is clearly not production, but if you agree, hopefully I’ve managed to write the general code you need to fix this. I am also happy to go through the work to turn this into a pull request, though there is a bunch of work to do (see at the bottom). See the check in after the one below for the code:
https://github.com/gordonwatts/Relinq/commit/c9fb978dc2559bab2d969280e1931a2105d6d97b
Cheers,
Gordon.
P.S. Work:
- The funny way I get a QueryModel out of an expression
- The “throw” in the middle of the method
- There is no good “compare” currently, in the test, to make sure the two query model’s are the same.
You received this message because you are subscribed to the Google Groups "re-motion Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to re-motion-use...@googlegroups.com.
To post to this group, send email to re-moti...@googlegroups.com.
Visit this group at https://groups.google.com/group/re-motion-users.
For more options, visit https://groups.google.com/d/optout.
Michael Ketting
(private back-reference: https://groups.google.com/d/msg/re-motion-users/Q8z-AhME4qo/gIQN5lFjAgAJ )
But I'm happy you found a workaround for the moment and aren't stuck. Gives a nice bit of breathing room while tracking this down. Did I already mention that I've really learned to dislike mutable datastructures?
Best regards, Michael
To unsubscribe from this group and stop receiving emails from it, send an email to re-motion-users+unsubscribe@googlegroups.com.
To post to this group, send email to re-motion-users@googlegroups.com.
Gordon Watts
Hi,
😊 Mutability is the new evil, indeed.
I will cut/paste the DLL into my current project and see if it can deal with the fairly complex queries that are being built. If this bug fix turns out not to work, or if it turns up other bugs, I’ll let you know.
Cheers,
Gordon.
To post to this group, send email to re-moti...@googlegroups.com.
Gordon Watts
Hi,
I’ve done some more testing. So far that fix I propose below seems to work.
I’ve moved it back into my re-linq specific project (the one you looked at), and the tests, slightly more complex than the one I put into the re-linq project, worked. I then moved all of those binaries built against that bug fix back into my research project that makes heavy use of re-linq with some complex (and very long) queries. The next step (no time tonight) is to move it even further into the project where there are some really hairy queires and see how it does. I hope to find time to do that tomorrow.
Speaking of which, I’ve been cut pasting DLL’s into the project. Since it works well enough, I think it is time for me to put a test version of re-linq on my myget feed (not in nugget!!). Are there instructions on how to package up a nuget package from a build of re-linq? This means I can also run it on my build server, where the testing is even more complete.
Again, thanks so much for your help!
Cheers,
Gordon.
Michael Ketting
Building: there is a console build that created nuget packages via Build.cmd (option #2 gives you a full build including the tests and nuget packages). The MSBuild Scripts also run on a BuildServer. You just have to do a package restore first for the solution and then call Remotion.Server.build via MSBuild, using the "Server_NightlyBuild" target. You will also need to pass a couple of properties (listed in Remotion.Server.build and Remotion.build). That said, the local build is definitely easier to do.
Best regards, Michael
Gordon Watts
Hi,
After cleaning up some further issues with my .Concat implementation, I’m able to run some pretty complex queries. The bug fix I suggested to re-linq seems to be holding up quite well. So something that effects the same outcome should make its way into the trunk of re-linq. Let me know if I can be of help!
To post to this group, send email to re-moti...@googlegroups.com.
Michael Ketting
Thanks for the update. Will look into this as soon as possible :)
Best regards, Michael
Gordon Watts
Cheers,
Gordon.
Subject: Re: [re-motion-users] Possible bug in QueryModel.Clone
Building: there is a console build that created nuget packages via Build.cmd (option #2 gives you a full build including the tests and nuget packages). The MSBuild Scripts also run on a BuildServer. You just have to do a package restore first for the solution and then call Remotion.Server.build via MSBuild, using the "Server_NightlyBuild" target. You will also need to pass a couple of properties (listed in Remotion.Server.build and Remotion.build). That said, the local build is definitely easier to do.
Best regards, Michael
On Wednesday, March 2, 2016 at 9:51:12 AM UTC+1, Gordon Watts wrote:
Hi,
I’ve done some more testing. So far that fix I propose below seems to work.
I’ve moved it back into my re-linq specific project (the one you looked at), and the tests, slightly more complex than the one I put into the re-linq project, worked. I then moved all of those binaries built against that bug fix back into my research project that makes heavy use of re-linq with some complex (and very long) queries. The next step (no time tonight) is to move it even further into the project where there are some really hairy queires and see how it does. I hope to find time to do that tomorrow.
Speaking of which, I’ve been cut pasting DLL’s into the project. Since it works well enough, I think it is time for me to put a test version of re-linq on my myget feed (not in nugget!!). Are there instructions on how to package up a nuget package from a build of re-linq? This means I can also run it on my build server, where the testing is even more complete.
Again, thanks so much for your help!
Cheers,
Gordon.
From: Gordon Watts
Sent: Monday, February 29, 2016 2:54 PM
To: Michael Ketting; re-motion Users
Subject: RE: [re-motion-users] Possible bug in QueryModel.Clone
Hi,
Mutability is the new evil, indeed.
In the source code QuerySourceMapping.cs. Of course, given the nature of this error, I suspect the actual bug is far away from here!
Cheers,
Gordon
--
You received this message because you are subscribed to the Google Groups "re-motion Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to re-motion-users+unsubscribe@googlegroups.com.
To post to this group, send email to re-motion-users@googlegroups.com.
Visit this group at https://groups.google.com/group/re-motion-users.
For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups "re-motion Users" group.
To post to this group, send email to re-moti...@googlegroups.com.
Michael Ketting
Yeah, it's still on my shortlist for re-linq but available time's very limited at the moment. The 2.1.x release is a bit of a special case as I had to integrate .NET Core as a target platform. I most definitely want to get rid of your need for a fork at the earliest opportunity, definitely before I do any other major work.
Best regards, Michael
PS: I'm terribly sorry that I've neglected the mailing list like this :/
On Monday, June 20, 2016 at 8:04:57 AM UTC+2, Gordon Watts wrote:
Hi,
I was wondering if you (or others) had managed to find some time to look into this bug? I was looking at the bug tracker and didn't see anything obvious. We could create a bug with just the top level test case and without my proposed solution. While my proposed solution may very well be in error for the reasons you describe below, I do think the Clone statement should work.The reason I'm sending this is I note that there is a new release in nuget now, and I don't want to start having to follow your code changes and re-applying the patches. :-)
Cheers,
Gordon.
Date: Wed, 2 Mar 2016 01:40:49 -0800
From: michael.ketting
To: re-motion-users
Gordon Watts
No worries. Thanks for all the time you spend on re-linq. It definitely has a following and I’m pretty sure none of us are paying you. 😊