Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

NH-3004: DriverBase.RemoveUnusedCommandParameters removes all parameters when UseNamedPrefixInSql = true and UseNamedPrefixInParameter = false #48

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
2 participants
Owner

oskarb commented Jan 10, 2012

Thanks for the patch. However, there is a number of issues.

Biggest problem is that if the command includes an actually unused parameter, the code now fails with an exception.

I hope you can have a look at this - shouldn't be too hard. Before you do, please pull branch NH3004-prepare from my personal repo git://github.com/oskarb/nhibernate-core.git to your branch. This will incorporate cleanup of the below issues, and additonal test case to expose the above issue.

Other issues (fixed in my NH3004-prepare branch):
Formatting: For future work, please use tabs for indenting, as this is NHibernate policy. (If you find an existing file with wrong indenting, please either leave it as is, or make a separate commit with only indenting cleanup, for easer review.)

Also, the test case wasn't part of the project file, and I've removed some boilerplate code that isn't necessary for these tests.

Thanks for your comment and highlighting the problems with my pull request. I'm kicking myself for not trying the function with an unused parameter.

I pulled the branch NH3004-prepare from your personal repo and have made changes to both the unit test, where I added another assert, and RemoveUnusedCommandParameters, so it now handles the unused parameters properly.

I have pushed the new commit to the master branch of my personal repo, git://github.com/mcharalambous/nhibernate-core.git, but am not sure how to add it to the pull request. Any help would be appreciated.

Thanks
Michael

Update on my previous comment.

The commit with the changes I have made have been incorporated in the pull request. But these do not include Oscar's changes. What have I done wrong and how can I update the pull request so they include Oscar's changes?

Thanks

Owner

oskarb commented Jan 11, 2012

Thanks Michael for having a renewed look at this. The pull request seems fine to me - the Commits tab lists my commits, and the Diff tab includes the combined effect of your and mine commits. (When I merge to master I will do a squash merge, to keep a cleaner history.)

I have one concern though. What will the computational complexity of the current state be? Looking into this now...

Owner

oskarb commented Jan 11, 2012

Problems:
formatter.AssignedParameterNames is now called for every parameter. This method will allocate and fill an array from a list, on-the-fly, so we now do this once for every parameter, which isn't needed.

Also, Contains() is called on that (those) arrays once for each parameter. Contains() should be O(n), meaning the whole expression is now O(n²). The old code should have been linear, since Except() seems to be O(n). This probably doesn't matter all that much for most queries, but occasionally a larger number of parameters is used - especially when using the IN operator.

Just doing formatter.AssignedParameterNames.ToDictionary() before the main expression would probably work.

Hi Oscar. Thanks for looking into this.

Would it be better to make it a HashSet instead of a Dictionary? Then the code would become

var AssignedParameterNames = new HashSet<string>(formatter.AssignedParameterNames);

cmd.Parameters
  .Cast<IDbDataParameter>()
  .Select(p => p.ParameterName)
  .Where(p => !AssignedParameterNames.Contains(UseNamedPrefixInParameter ? p : FormatNameForSql(p)))                
  .ToList()
  .ForEach(unusedParameterName => cmd.Parameters.RemoveAt(unusedParameterName));

Would this be okay?

Owner

oskarb commented Jan 11, 2012

Sure, HashSet should be fine.

/Oskar

oskarb added a commit that referenced this pull request Jan 11, 2012

Merge pull request #48, fix for NH-3004, contributed by Michael Chara…
…lambous <michael.charalambous@gmail.com>, with some tweaks by me.
Owner

oskarb commented Jan 11, 2012

Thanks, applied.

@oskarb oskarb closed this Jan 11, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment