On Fri, 9 Jan 2026 14:16:41 GMT, Mikhail Yankelevich <[email protected]> wrote:
> Hi! > > This is my proposal to transfer `KeyStore` and `KeyStoreSpi` with internal > implementations to use `Instance`s instead of `Date`s. > I would be very grateful for your comments and suggestions. > > Thanks! > > P.S. this is related to > [JDK-8350953](https://bugs.openjdk.org/browse/JDK-8350953) 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? 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()). 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`. 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. 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`. 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..." 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? 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`? ------------- PR Review: https://git.openjdk.org/jdk/pull/29140#pullrequestreview-3645351472 PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2677424331 PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2677428454 PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2676561054 PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2677440504 PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2676552098 PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2677463193 PR Review Comment: https://git.openjdk.org/jdk/pull/29140#discussion_r2677465677
