RFR: JDK-8346465 : Add a check in setData() to restrict the update of Built-In ICC_Profiles [v5]

[Phil Race]

On Thu, 20 Feb 2025 21:36:06 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>>> > Although, there's no time where such a modification will be allowed
>>> 
>>> Exactly, it will throw exception every time a built-In profile is passed unlike IllegalStateException which is thrown only during an inappropriate time for instance when you are trying to add an element to the queue when it is already full. Plus setData() already throws IAE for other invalid argument cases.
>> 
>> I see no contradiction here. Inappropriate time is a vague definition. For built-in object, it is always inappropriate time to call `setData`.
>> 
>> `IllegalArgumentException` implies that the argument is invalid, and if I change the argument, the call will succeed — but the call will never succeed for a built-in profile *because **the state of the object** doesn't allow it*.
>> 
>> This is why `IllegalStateException` is more appropriate.
>> 
>>> Although it is not a checked exception, the fact that IllegalArgumentException is already documented on this method is considered to outweigh any benefit of a new Exception type such as IllegalStateException.
>> 
>> I do not agree here. The exception type should convey the reason it's thrown, otherwise there wouldn't be so many types of exceptions.
>> 
>> And two `@throws` with the same type aren't allowed in javadoc, which means you have to add the new reason to throw `IllegalArgumentException` into the existing one.
>
> Let's consider a scenario.
> 
> If I call `setData` with `tagSignature` and `tagData` which cannot be interpreted correctly, I get `IllegalArgumentException`, which is reasonable — I passed invalid arguments.
> 
> Then if I call `setData` with valid values for `tagSignature` and `tagData`, the call succeeds even with a built-in color profile. After this fix, this call would fail with `IllegalArgumentException` but I know that the arguments are **valid** because the call succeeded previously. And however I change the arguments, the call will never succeed.
> 
> Throwing `IllegalStateException` will convey the updated behaviour in a cleaner way: it is *the state* of the object that makes the call to fail, not the arguments.

> And two @throws with the same type aren't allowed in javadoc, which means you have to add the new reason to throw IllegalArgumentException into the existing one.

That's demonstrably untrue. This method already has two.

Regarding the scenario above, this apparent and has all been considered already and the IAE is the preferred solution.

And FWIW I don't think ISE is really any better.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964393430