On Sat, Dec 28, 2019 at 7:40 AM Michael Osipov <[email protected]> wrote:
> Folks, > > this has been bugging me for some time lately and I'd like to know your > opinion about it. I am also convinced that this may confuse some of your > client users: > > We have: > * interface AuthScheme with String #getName() > * enum AuthSchemes which should be singular and maybe StandardAuthScheme > with the auth schemes we support out of the box. Or similar to > StandardCharsets have "final class StandardAuthSchemes" which has only > "final static Sting SYMBOLIC_NAME = "AuthSchemeName". > For me the enum implies that this is an exhaustive list of schemes we > support, nothing else more possible. > Thanks for bringing all of this up Michael :-) It feels like there is a disconnect indeed and that the code is not complete or at least missing Javadoc: - org.apache.hc.client5.http.impl.auth.SystemDefaultCredentialsProvider uses hard coded strings instead of AuthSchemes id strings. - All references to AuthSchemes use the AuthSchemes.id String instead of an AuthSchemes itself (writing about this really shows the plural name as lame.) Always using the id field seems to defeat the purpose of having an enum in the first place. Do we intend for user pluggable AuthSchemes id String? It's confusing: - org.apache.hc.client5.http.impl.DefaultAuthenticationStrategy.DEFAULT_SCHEME_PRIORITY is a List<String> instead of a List<AuthSchemes> - This List is used with: - org.apache.hc.client5.http.config.RequestConfig.getProxyPreferredAuthSchemes() returns List<String> - org.apache.hc.client5.http.config.RequestConfig.getTargetPreferredAuthSchemes() returns List<String> That tells me I can provide whatever I want in RequestConfig in these two scheme lists. Is that the intent or do these list must contain only valid AuthSchemes ids? The Javadoc say nothing about the supported value range (RequestConfig.Builder.setTargetPreferredAuthSchemes(Collection<String>)) We ran into a similar-ish issue over at Log4j 2 with levels (INFO, WARN, and so on) and I've had to deal with the same pattern elsewhere, so TL;DR is if you want to define a type with known instances AND allow for custom instances you cannot use an enum, you have to use a class and/or interface. I am not sure of the intent here. The fact that we have both an enum and an interface with effectively the same name screams out for help ;-) java.nio.charset.StandardCharsets is a good compromise and we could follow suit but let's address the above first IMO. * class AuthScope with String #getAuthScheme(). Logically, I would > expect here to be returned an instance of AuthScheme and not a string. > Otherise it should be "String #getSchemeName()" > Indeed, if we have an enum, let's use it, otherwise, do we need it at all? If not we can go the java.nio.charset.StandardCharsets route. Maybe. Tangent: - org.apache.hc.client5.http.auth.AuthSchemeProvider is Javadoc'd as a factory class so why no call it AuthSchemeFactory? I think of providers as larger scale components (like a security provider) where a factory is smaller scale and creates instances. In this case, you cannot create an AuthScheme since it is an enum, but no, wait, this factory is typed to the interface so it does indeed create instances... we need to fix the names of these types... Gary * HTTPCLIENT-2041, HTTPCLIENT-2042 > * class AuthChallenge with "String #getScheme()" same with AuthScope > * class AuthExchange with "AuthSheme #getAuthScheme" > * RequestConfig "Collection<String > #(target|proxy)PreferredAuthSchemes()" same with AuthScope > > and maybe other spots I have missed. > > CookieSpec and friends have the same issue. > > Am I just paranoid or do you consider them to be inconsistent as well? > > Michael > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
