On Tue, 6 Aug 2024 21:02:50 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Francisco Ferrari Bihurriet has updated the pull request incrementally with
>> two additional commits since the last revision:
>>
>> - Ensure Security::setProperty() cannot issue an include
>>
>> Co-authored-by: Martin Balao <mba...@redhat.com>
>> Co-authored-by: Francisco Ferrari Bihurriet <fferr...@redhat.com>
>> - Simplify PropertyExpander::expandNonStrict()
>>
>> Co-authored-by: Martin Balao <mba...@redhat.com>
>> Co-authored-by: Francisco Ferrari Bihurriet <fferr...@redhat.com>
>
> src/java.base/share/classes/java/security/Security.java line 901:
>
>> 899: if (SecPropLoader.isInclude(key)) {
>> 900: return;
>> 901: }
>
> Don't you want to throw an exception here?
May be. This is a public API that only documents `SecurityException` for cases
in which there is a Security Manager and `NullPointerException` for cases in
which either the key or the value are `null`. Wouldn't be the exact case here,
unless we stretch it a bit and document a new type of unchecked exception. I
was thinking of `IllegalArgumentException`. What do you think?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1706206008
