HaveIBeenPwnedClientSettings.ApplicationName should default to an empty string and an exception must be thrown when it's not set

akamsteeg / AtleX.HaveIBeenPwned

A fully async .NET Standard client library for the API of HaveIBeenPwned.com

https://www.nuget.org/packages/AtleX.HaveIBeenPwned/

MIT License

5 stars 0 forks source link

HaveIBeenPwnedClientSettings.ApplicationName should default to an empty string and an exception must be thrown when it's not set #30

Closed akamsteeg closed 4 years ago

akamsteeg commented 5 years ago

Currently, HaveIBeenPwnedClientSettings.ApplicationName defaults to "AtleX.HaveIBeenPwned" so it's not mandatory for users to set it. However, the FAQ of the HaveIBeenPwned.com API makes it absolutely clear that a user should set this:

Not properly identifying the user agent such that it accurately describes the consumer of the API

We must remove the default application name and throw an exception from `HaveIBeenPwnedClient when a null or empty application name is set.

akamsteeg commented 5 years ago

This one's tricky because it results in exceptions at runtime instead of some nice errors in the developer's editor. When someone implementing this doesn't have tests on our lib in place it blows up, which is very much possible because why would you test an external lib? I'm not sure how to handle this yet but I'm open to suggestions.

akamsteeg commented 4 years ago

I considered marking `HaveIBeenPwnedClientSettings.ApplicationName with an ObsoleteAttribute and introduce a new property ApplicationIdentifier. This would introduce a warning for users, but that comes with a few drawbacks:

Many programmers ignore warnings;
If you don't use AtleX.HaveIBeenPwned directly but as a dependency of a lib you use, you won't see the warning unless you provide the HaveIBeenPwnedClientSettings yourself;
If you just use the default settings now by not passing any custom settings, you won't get a warning.

So, I figured this is not a good solution either.