Request for refactoring: Config.getXXX()

[Valerie (Yu-Ching) Peng]

Yes, I prefer the idea of explicitly specifying the path for finding the 
values even if more calls are needed.
Valerie

On 06/18/12 19:07, Weijun Wang wrote:
> This is about the internal class sun.security.krb5.Config.
>
> If you want to get a value from inside krb5.conf, you can call 
> getDefault(String). This might be good to get a value from the 
> [libdefaults] section. However, the method was designed to be so smart 
> that it can recursively search for key/value pairs no matter how deep 
> it is.
>
> For example, given a krb5.conf
>
>    [s1]
>    a=b
>
>    [s2]
>    c=d
>
>    [s3]
>    e = {
>      f = g
>    }
>
> getDefault("a") = "b", getDefault("c") = "d", and astonishingly, 
> getDefault("f") = "g".
>
> I don't think this is a good design, for several reasons:
>
> 1. It depends on the order of sections if there are key/value pairs 
> with the same key.
>
> 2. It ignores wrong settings. For example, when doing a cross-realm 
> auth, the Realm.getRealmsList(from,to) is used to get a path which 
> should be defined in [capaths]. However, the method simply crawls 
> recursively into any subsection it found and won't notice the 
> [capaths] being mistakenly typed as [capath]
>
> 3. It lacks certain features. Because the function always return a 
> String (same with the getDefault(String,String) method), 
> getDefault("e") can only return a null. Therefore there is no way to 
> find out the existence of the subsection e unless we also know it 
> contains a key f.
>
> 4. The current Config class needs to know what subsections contains 
> more subsections, and it hardcodes names like [capaths] and [realms].
>
> In short, it's just too smart and becomes unsafe to use. I suggest 
> removing all this smartness and a user must use the full paths to get 
> a value, say,
>
>    kdc = config.get("realms", "SUN.COM", "kdc")
>
> My full spec is:
>
> 1. The Config class should understand a krb5.conf without knowing any 
> specific section names. All it maintains is a Value, which can be 
> either of
>
>     String
>     List<Value>
>     TreeMap<String,Value>
>
> Here I use TreeMap to preserve the order (might not be necessary).
>
> 2. The basic retrieval method will be
>
>     Value get(String... key)
>
> 3. There are simply methods if you already know what the type in your 
> case is
>
>     String getAsString(String... key)
>     List<String> getAsStringList(String... key)
>
> The compatibility risk will be low, and if there really comes a 
> compatibility issue, most likely it will be because the caller had 
> written his krb5.conf wrong.
>
> One of the advantages of the original design is that when a key is 
> provided in both [libdefaults] and a given realm, the method can find 
> it anyway. This will be useful for keys like kdc_timeout, max_retries. 
> However, I think this automatic retrieval is confusing and 
> error-prone, I'd rather manually call the get() method twice.
>
> Thanks
> Max
>