On Fri, 9 Jan 2026 20:01:47 GMT, Weijun Wang <[email protected]> wrote:
> Mostly looks good. Do you have a plan to update the callers of
> `KeyStore::getCreationDate`?
>
> Also, I see you skipped the SunPKCS11 and MSCAPI implementations. Are they
> too trivial and the default implementation is enough?
Apologies, missed this comment the last time I went through them.
`P11KeyStore` has no support for creation date and throws an exception so I
think the default should be enough.
As for `CKeyStore` It seems that it just creates a new date and nothing else.
I'm not sure if writing a whole separate method to always provide current time
is worth it when we have a default implementation. But if you believe it is -
I'm happy to add it
> src/java.base/share/classes/java/security/KeyStore.java line 1187:
>
>> 1185: * Returns the creation date of the entry identified by the given
>> alias.
>> 1186: * <p>
>> 1187: * Please consider using {@code getCreationTimestamp} instead.
>
> Better add a reason. For example, something like "This method returns a Date,
> which is mutable and more error-prone. Use getCreationTimestamp() instead." I
> feel hesitated to mention "legacy" or "modern" directly.
>
> Here I'm following the [`Thread::getId`
> words](https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/Thread.html#getId()).
Yes, this sounds better, thanks. Changed in the next commit.
> src/java.base/share/classes/java/security/KeyStore.java line 1208:
>
>> 1206:
>> 1207: /**
>> 1208: * Returns the creation timestamp (Instant) of the entry identified
>
> Maybe change `(Instance)` to `as an {@code Instant} value`, if you think it's
> worth mentioning the difference. Anyway, don't leave "Instant" in plain text.
>
> There should be a `@since 27`.
Done in the next commit
> src/java.base/share/classes/java/security/KeyStore.java line 1215:
>
>> 1213: *
>> 1214: * @throws KeyStoreException if the keystore has not been
>> initialized
>> 1215: * (loaded).
>
> Although `getCreationDate` is not deprecated, I wonder if you can add some
> comment there recommending the new method with some reason.
Done in the next commit
> src/java.base/share/classes/java/security/KeyStoreSpi.java line 138:
>
>> 136: * {@code engineGetCreationDate(alias)} which returns a {@code
>> Date}.
>> 137: * If the result is not @{code null} returns
>> 138: * Instance equivalent to received {@code Date}.
>
> The sentence should start with "The default implementation". For example,
> "The default implementation calls engineGetCreationDate(alias) and returns
> the output as an Instant value".
>
> There is no need to be so detailed about `null`.
>
> There should be a `@since 27`.
Done in the next commit
> src/java.base/share/classes/java/security/KeyStoreSpi.java line 141:
>
>> 139: */
>> 140: public Instant engineGetCreationTimestamp(String alias) {
>> 141: return engineGetCreationDate(alias).toInstant();
>
> 1. You might need to check for null first.
> 2. There should be an `@implSpec` saying "The default implementation
> returns..."
Yes, you're right. Done in the next commit.
> src/java.base/share/classes/sun/security/provider/DomainKeyStore.java line
> 238:
>
>> 236: }
>> 237:
>> 238: /**
>
> For this class, does it make sense to rewrite `engineGetCreationDate` like
> the other implementations?
I believe so, otherwise we will be either getting `Date` as before via
`engineGetCreationDate` or have a default implementation from `KeyStoreSpi`
which converts the result of `engineGetCreationDate`. This seems to me just an
extra operation, which I'm not convinced is necessary.
However, if you think in this case it might be ok - I'm happy to remove this
and simplify.
> src/java.base/share/classes/sun/security/provider/JavaKeyStore.java line 240:
>
>> 238: */
>> 239: public Date engineGetCreationDate(String alias) {
>> 240: return Date.from(this.engineGetCreationTimestamp(alias));
>
> Could `engineGetCreationTimestamp(alias)` return `null`?
Yes, missed it in the last change, thanks!
-------------
PR Comment: https://git.openjdk.org/jdk/pull/29140#issuecomment-3743866087
PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2681869462
PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2681871215
PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2676803707
PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2681901615
PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2676803086
PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2681920481
PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2681926844
