On Thu, 25 Aug 2022 02:59:42 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Existing provider filtering code only handles two standard attribute >> "KeySize" and "ImplementedIn", the rest are compared by exact match. Over >> time, more standard attributes are added which contain multiple values >> separated by "|". We should enhance the provider filtering code to better >> compare these. >> >> Documentation update for this is tracked separately under >> https://bugs.openjdk.org/browse/JDK-6447817. >> >> Thanks in advance for review~ > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > update test to use SHA256 and DSA throughout. Some comments. src/java.base/share/classes/java/security/Security.java line 530: > 528: // <crypto_service>.<algo_or_type> <attr_name>:<attr_value> > 529: key = filter.substring(0, index); > 530: value = filter.substring(index + 1); Do you need to ensure `value` is non-empty here? I am worried that somewhere later a `string.indexOf("")` is called and it's always true. src/java.base/share/classes/java/security/Security.java line 599: > 597: // Returns all installed providers > 598: // if the selection criteria is null. > 599: if ((keySet == null) || (allProviders == null)) { I'm not sure, but can `keySet` or `allProviders` be null? Or you meant `isEmpty()`? src/java.base/share/classes/java/security/Security.java line 931: > 929: // check individual component for match and bail if > no match > 930: if (prop.indexOf(st.nextToken()) == -1) { > 931: return false; So if `value` has several sub-values, all of them must appear in the `prop` value. Do we need to make this clear in the spec? Also, you use `indexOf` instead of an exact match to a sub-value in `prop`. Is this always correct? I am wondering if a value can be substring of a different value. I see you support simple class name in the test. Is it worth we doing this? I would rather be strict at the beginning. src/java.base/share/classes/java/security/Security.java line 945: > 943: > 944: static String[] getFilterComponents(String filterKey, String > filterValue) { > 945: Can we create a new class for the return value plus `filterValue`? Then we can make `isCriterionSatisfied(Provider)` a method of it. test/jdk/java/security/Security/ProviderFiltering.java line 89: > 87: doit(key + ":" + valComp2 + " ", p); > 88: // 4. partial value, e.g. class name only > 89: doit(key + ":" + valComp2CN, p); Do we really need to support cases 2-4? In fact, the only place mentioning spaces in the spec is about the ones between "SHA256withDSA" and "SupportedKeyClasses". I'd rather you add a test case on it. test/jdk/java/security/Security/ProviderFiltering.java line 109: > 107: filters.put("Signature.SHA256withDSA", ""); > 108: doit(filters, p); > 109: filters.put("Cipher.Nonexisting", ""); Even if it's not "Nonexisting", I assume it's still empty after filtering? ------------- PR: https://git.openjdk.org/jdk/pull/10008
