On Thu, 7 Aug 2025 17:53:21 GMT, Artur Barashev <abaras...@openjdk.org> wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address review comments from Artur and added a regression test for
>> invalid property values.
>
> src/java.base/share/classes/sun/security/util/CryptoAlgorithmConstraints.java
> line 99:
>
>> 97: throw new IllegalArgumentException("Invalid entry: " +
>> dk);
>> 98: }
>> 99: if
>> (SUPPORTED_SERVICES.stream().anyMatch(e->e.equalsIgnoreCase
>
> Nit:
> - Spaces around `->`
> - Look up will be faster if we do `Set.contains` on upper-cased strings
> instead of iteration with `equalsIgnoreCase`. Although probably not
> noticeable with just 4 services we currently have.
Yes, I will add space around "->".
As for performance differences, the `Set,.contains`, approach will require an
additional `toUpperCase()` call for every lookup. Or we can also use a
`TreeSet` w/ case-insensitive ordering. Anyway, probably not difference when
there are just 4 services?
> test/jdk/java/security/MessageDigest/TestDisabledAlgorithms.java line 29:
>
>> 27: * @summary Test JCE layer algorithm restriction
>> 28: * @run main/othervm TestDisabledAlgorithms MessageDigest.Sha-512 true
>> 29: * @run main/othervm TestDisabledAlgorithms MessageDigest.what false
>
> Let's use differently mixed upper/lower cased versions of `MessageDigest`
> word to test proper case-insensitive match. In current versions of the tests
> all service names match exactly the names defines in
> `CryptoAlgorithmConstraints`. For example:
>
> - MessageDigest
> - messaGedigesT
> - MESSAGEdigest
>
> Same for other services' tests.
Good suggestion, will do.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26377#discussion_r2261329773
PR Review Comment: https://git.openjdk.org/jdk/pull/26377#discussion_r2261331035
