WithParam: exeption when using already existing param name outputs "key" instead of param name

[Clooney24]

Hi Charlotte,

when using WithParam with parameter name that already exists in query, the following exeption is thrown:

if (QueryWriter.ContainsParameterWithKey(key))
    throw new ArgumentException("A parameter with the given key is already defined in the query.", nameof(key));

But using nameof(key) will always add "Parameter name: key" to the exception message which doesn't help.

This should just be key to add the name of the misused parameter and to help the developer to identify the problem.

I could fix this together with isse 458 if you agree.

Best, Reiner

[Clooney24]

PR created: https://github.com/DotNet4Neo4j/Neo4jClient/pull/460

[cskardon]

Hey Reiner,

This should be nameof(key) - as the property on the ArgumentException is ParameterName not ParameterValue.

You could totally change the message to include the name of the key, but the parameter itself should state which parameter is invalid.

This about it from the POV of a method with multiple parameters, if you just put the value in, you can't tell which of the parameters has actually caused the exception.

All the best

Charlotte

[Clooney24]

Hi Charlotte,

I commented in the PR 2 weeks ago that nameof(key) always is "key" and the value of key contains the used key, please check it there.

Best, Reiner

[cskardon]

Hi Reiner - sorry - I had not 'completed' the review so my comments were pending - I can only apologise for that oversight.

I still fundamentally disagree. The Argument that is wrong is the key one. The value of that key may be the reason it's wrong, but the value isn't the parameter.

If I had this method:

public void DoIt(string a, string b, string c) { /**/ }

And then threw ArgumentExceptions like this:

if( a IS WRONG) throw new ArgumentException("The value isn't valid", a);
if( b IS WRONG) throw new ArgumentException("The value isn't valid", b);
if( c IS WRONG) throw new ArgumentException("The value isn't valid", c);

I can't tell which parameter is wrong. By changing the message to have the value, I can see that, but the ParameterName is a, b or c

if( a IS WRONG) throw new ArgumentException($"The value '{a}' isn't valid", nameof(a));
if( a IS WRONG) throw new ArgumentException($"The value '{b}' isn't valid", nameof(b));
if( a IS WRONG) throw new ArgumentException($"The value '{c}' isn't valid", nameof(c));

[Clooney24]

Yes, agree but in your example there is no cypher. As a user of Neo4jClient, I want a message that refers to the paramter name that I used twice in the cypher or that has a wrong syntax in the cypher. I don't care how this parameter is named in the c# code of Neo4jClient.

In your version, the message always outputs "wrong parameter name 'key'" - even when my cypher-parameter was named "foo". I need the parameter name that causes the problem in cypher and that name is the value of key.

It only works because you also named the cypher parameter "key" in your test. Please just rename it to "foo" in lines 62 and 63 and you'll see that your test will fail. But test should work with any parameter name used in cypher.

[Clooney24]

Also see that example with $uuid. When I use a parameter named "$uuid" it's not helpful when I get an error message "wrong parameter name 'key'" - it has to be "wrong parameter name '$uuid'" - and $uuid is the value of key. I wouldn't have changed this if I had not stumbled across it.

[cskardon]

The fact is that the key is incorrect it has been named incorrectly the value of the key is causing an exception, but the key is the parameter name - you can easily change the message to read:

The parameter named 'XXX' is invalid for these reasons...

But the fact is that the key is the ParameterName (which is the property on the exception) that is wrong.

[DarthSonic]

You two are talking completely past each other ;-)

To summarize in clearer words what @Clooney24 wants:

If currently an error occurs in the parameter with the name key, then a constant error message is thrown with the string "key", not the nameof(key) value. Say, if the key parameter has the value "myKey" - since it was passed that way in the query - the unchanged string "key" is still written to the error message, but not the variable string "nameof(myKey)":

new Exception("this is my exception in key parameter") <> new Exception("this is my exception in mykey parameter")

Therefore the variable nameof(key) should be passed to the string, not "key":

new Exception("this is my exception in 'key' parameter") <> new Exception($"this is my exception in '{nameof(mykey)}' parameter").

@cskardon is this more understandable now? The issue is that currently the same fixed string is always output and the name of the key parameter is not currently inserted dynamically.

[Clooney24]

This might only be helpful for someone just developing on the source code of Neo4jClient.

But for all users of the package, that message isn't helpful at all. Imagine I have a cypher containing 20 paramers and one of them causes trouble, wouldn't it be great when the exception tells me which of them is problematic instead of always telling me "parameter name key".

Can we agree to mention both? Like this:

throw new ArgumentException($"A parameter with the given key '{key}' is already defined in the query.", nameof(key));

[cskardon]

@DarthSonic The name of the parameter to the method is used. What is actually being discussed is the fact that the exception doesn't have the name of the cypher parameter - it's nothing to do with the string content - as - if you look at the original code, you'll see that the ParameterName property is set to nameof(key)

@Clooney24 - I have suggested this here https://github.com/DotNet4Neo4j/Neo4jClient/issues/459#issuecomment-1415834465 and here https://github.com/DotNet4Neo4j/Neo4jClient/pull/460/files#r1084318553 I have no problem at all with the Message being updated, but the ParameterName property should be the parameter into the method, I think that's the perfect solution - hence writing it there and here.

This is how C# is supposed to be written when using ArgumentException - it is an agreed upon practice (like *Never throwing or catching Exception) and in the C# guidelines: https://learn.microsoft.com/en-us/dotnet/api/system.argumentexception.paramname?view=net-7.0#remarks

[cskardon]

5.1.5 is in nuget now with the changes.