CSHARP-5106: Disallow comma character in authMechanismProperties conn… #1354
Conversation
|
Is this the right branch? While there are two tests regarding commas there's a ton of seemingly unrelated connection stuff in this PR. |
|
Yep, this is the proper branch. CSharp driver has the appropriate functionality done in scope of OIDC implementation. This changes needed to add spec tests to verify the behavior + some minor fixes in spec tests. So the most changes are about spec tests sync (*.yml and *.json files were copied from spec repository) |
|
|
||
| if (Environment == "test" && !string.IsNullOrEmpty(PrincipalName)) | ||
| { | ||
| throw new ArgumentException($"{nameof(PrincipalName)} is not supported by {Environment} environment.", nameof(PrincipalName)); |
There was a problem hiding this comment.
ArgumentException seems weird here (there is no argument named PrincipalName, that's a property name). But I do see that there is lots of code above this that already does that.
This seems more like a configuration error than an argument exception.
Not sure what the correct exception should be though, .NET does not seem to have an appropriate exception for configuration errors.
If we want to be thorough we could define our own ConfigurationException class.
Alternatively consider throwing NotSupportedException?
Or just ignore this whole comment and throw ArgumentException... :)
There was a problem hiding this comment.
I normally go with InvalidOperationException
The exception that is thrown when a method call is invalid for the object's current state.
There was a problem hiding this comment.
I wondered about that. Usually "invalid operation" is thrown by methods. The meaning is that the "operation" is not valid in the current state of the object, but that if you changed the state of the object you might be able to call the method again (i.e. execute the operation) successfully.
No method was called (by the user) in this case. I suppose you could argue that the "operation" here is the constructor call.
There was a problem hiding this comment.
@rstam as it turned out we do have MongoConfigurationException. I'll switch to using it here... the only question should I change all exceptions in the method to throw MongoConfigurationException instead of ArgumentException? Changing that would be a breaking change, but the code is relatively new so probably we can do that. @JamesKovacs WDYT?
There was a problem hiding this comment.
MongoConfigurationException seems quite appropriate here. Although it is technically a breaking to change from ArgumentException to MongoConfigurationException, it's acceptable since OIDC was recently introduced and thus hasn't been widely adopted by our user community yet.
…ection string value
…ection string value