On Fri, 13 Mar 2026 16:11:47 GMT, Mikhail Yankelevich
<[email protected]> wrote:
>> Adding new methods to `X509Certificate` to return `Instant` instead of
>> `Date` as well as moving away from `Date` in internal packages wherever
>> possible.
>
> Mikhail Yankelevich has updated the pull request incrementally with one
> additional commit since the last revision:
>
> minor doc change
Some comments on the API.
src/java.base/share/classes/java/security/cert/X509Certificate.java line 175:
> 173: * Checks that the given instant is within the certificate's
> 174: * validity period. In other words, this determines whether the
> 175: * certificate would be valid at the given date/time.
I think we can just say "the given instant".
src/java.base/share/classes/java/security/cert/X509Certificate.java line 190:
> 188: * with respect to the {@code instant} supplied.
> 189: * @throws CertificateNotYetValidException if the certificate is
> not
> 190: * yet valid with respect to the {@code instant} supplied.
Since this is new, we can add a `@throws NPE` to be clear.
src/java.base/share/classes/java/security/cert/X509Certificate.java line 378:
> 376: *
> 377: * @return the start date of the validity period.
> 378: * @see #checkValidity
There are several `@see #checkValidity` in this class (both existing and newly
added). I think they will linked to the one without any argument. If this is
intended, I would make it clear by adding `()` at the end. Otherwise, if you
actually want to link to one using the same type, also make it clear by adding
`(Date)` or `(Instant)`.
src/java.base/share/classes/java/security/cert/X509Certificate.java line 383:
> 381: public Instant getNotBeforeInstant() {
> 382: final Date date = getNotBefore();
> 383: return date == null ? null : date.toInstant();
Now that we are creating a new API, can we require the return values to be
non-null? Even if there is no guarantee what a bad implementation could do, we
can probably add an `@implSpec` for it. I briefly checked the callers of these
methods and seems no one expects a null.
We can at least fail noisily here.
-------------
PR Review: https://git.openjdk.org/jdk/pull/30047#pullrequestreview-4055595439
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3032749642
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3032865778
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3032863960
PR Review Comment: https://git.openjdk.org/jdk/pull/30047#discussion_r3032908139
