On Sun, Dec 29, 2019 at 4:23 PM Michael Osipov <[email protected]> wrote:
> Am 2019-12-29 um 21:08 schrieb Gary Gregory: > > It just dawned on me that J. Bloch addresses enum extensibility by having > > an enum implement an interface and having the API implemented in terms of > > the interface. > > > > This gives you the best of both worlds: you get to define an enum with > your > > precanned values AND you allow users to provide custom implementations. > > I am aware of that scheme that this looks like a lot of complexity for > something which does not need it. > > Does it? > >From on high, no, but OTOH, I've not had the need to provide a custom auth scheme so I cannot say how well such a thing would all look and feel in code ;-) But, we already have the 'complexity' by the mere fact that we have an auth scheme interface AND enum! So either we simplify what we have now or we clean it up, it just feels weird to leave it as it. The guts of the JRE has the enum sun.net.www.protocol.http.AuthScheme, no interfaces, no classes, so who knows what the best design is... Gary > > > On Sun, Dec 29, 2019, 08:15 Michael Osipov <[email protected]> wrote: > > > >> Am 2019-12-28 um 17:17 schrieb Gary Gregory: > >>> 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. > >> > >> Exactly! > >> > >>> 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>)) > >> > >> Yes, the usage of strings implies that you can put in whatever you want. > >> This makes sense because of two reasons: > >> > >> * You can swap your default impls against your own > >> * You can add you own auth schemes which operate over WWW-Authenticate, > >> e.g., Bearer, HOBA, Mutual [1]. > >> > >>> 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. > >> > >> Absolutely correct! > >> > >>> 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... > >> > >> I share this. For me a provider holds a collection a factory > >> implementations for names entities. > >> > >> Please create a JIRA issue to track this. > >> > >> I have a few ideas, I will provide a PR soon for this. > >> > >> Michael > >> > >>> * 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 > >> > >> [1] > >> > >> > https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#Basic_authentication_scheme > >> > > > >
