Sitelet https://github.com/npgsql/npgsql/pull/6073/files
Skip to content

Fix adding to hash lookup while renaming an unnamed parameter #6073

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vonzshik merged 1 commit into from
Mar 26, 2025
Merged

Fix adding to hash lookup while renaming an unnamed parameter #6073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Npgsql/NpgsqlParameterCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ internal void ChangeParameterName(NpgsqlParameter parameter, string? value)
var oldTrimmedName = parameter.TrimmedName;
parameter.ChangeParameterName(value);

if (_caseInsensitiveLookup is null || _caseInsensitiveLookup.Count == 0)

@NinoFloris NinoFloris Mar 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is a strange check, and it's not mirrored in any of the other operations...

Maybe at one point the idea was to allow the lookup to be fully bypassed again until the threshold would be reached once more.

@vonzshik vonzshik Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was to support cached NpgsqlCommand, as for them we clear lookups, but do not null them. But yeah, since other checks ignore this case, it's better to just remove it from here rather than try to make the lookup completely lazy.

@NinoFloris NinoFloris Mar 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would end up doing the same thing right? We enable the lookup at some point and we never disable it.

I recall that when I made this there was some thought that it would be nice if we would drop back to the fast path until the threshold.

Seeing where we landed in the end - and in all other operations (lookupadd etc) we only check for null - I must have missed this check when I decided it was not worth the complexity.

if (_caseInsensitiveLookup is null)
return;

var index = IndexOf(parameter);
Expand Down
28 changes: 28 additions & 0 deletions test/Npgsql.Tests/NpgsqlParameterCollectionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,34 @@ public void Hash_lookup_parameter_rename_bug()
Assert.That(command.Parameters.IndexOf("a_new_name"), Is.GreaterThanOrEqualTo(0));
}

[Test]
[IssueLink("https://github.com/npgsql/npgsql/issues/6067")]
public void Hash_lookup_unnamed_parameter_rename_bug()
{
if (_compatMode == CompatMode.TwoPass)
return;

using var command = new NpgsqlCommand();

for (var i = 0; i < 3; i++)
{
// Put plenty of parameters in the collection to turn on hash lookup functionality.
for (var j = 0; j < LookupThreshold; j++)
{
// Create and add an unnamed parameter before renaming it
var parameter = command.CreateParameter();
command.Parameters.Add(parameter);
parameter.ParameterName = $"{j}";
}

// Make sure hash lookup is generated.
Assert.AreEqual(command.Parameters["3"].ParameterName, "3");

// Remove all parameters to clear hash lookup
command.Parameters.Clear();
}
}

[Test]
public void Remove_duplicate_parameter([Values(LookupThreshold, LookupThreshold - 2)] int count)
{
Expand Down