RFR: 8296143: CertAttrSet's set/get mechanism is not type-safe [v5]

[Sean Mullan]

On Sun, 6 Nov 2022 22:00:41 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> The major change is to remove the `get` and `set` methods in various `CertAttrSet` child classes and change them to `setXyz` and `getXyz` methods. The `Xyz` words might come from the field name or the attribute name. For example, `X509CertInfo` now has `setExtensions` and `setValidity` instead of `set("extensions", exts)` and `set("validity", validity)`. This also has the benefit to remove a lot of try-catch blocks on `IOException`s on "unknown attributes" because everything is known now. At the same time, all the identifier name and attribute names are removed from `CertAttrSet` child classes. The only left is `NAME` in extensions since it's still used as keys in `CertificateExtensions`.
>> 
>> Besides assigning a new value to an internal field, the original `set` methods might also re-encode by calling `encodeThis`, invalidate the cached encoding (in `X509CertInfo`), or check for read-only flag (in `X509CertImp`). Newly added `setXyz` methods are doing the same. This is one place that future new setter methods should remember.
>> 
>> Most `get` implementations simply return an internal field. One exception in `X509CertImpl` is that when getting something inside the `X509CertInfo`, it wraps exceptions into a new `CertificateParsingException`. This is actually related to the way `CertificateExtensions::get` is implemented where an exception is thrown when an extension does not exist. `CertificateExtensions::getExtension` has been rewritten to follow the `CRLExtensions::getExtension` style where `null` is returned in this case.
>> 
>> The only method left in `CertAttrSet` is `encode`, and it no longer throws a `CertificateException`.
>> 
>> Several classes do have their attributes, and still has get/set methods. This includes `CertificateExtensions`, `CRLExtensions`, `ReasonFlags`, `KeyUsageExtension`, and `NetscapeCertTypeExtensions`. Some methods are renamed to be clearer. For example, in `CertificateExtensions`, we have `getExtension` instead of `get`.
>> 
>> There are no more `AttributeNameEnumeration.java` and `X509AttributeName.java`.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   return (abc) to return abc

Great stuff, just a few minor comments.

src/java.base/share/classes/java/security/cert/X509CertSelector.java line 1313:

> 1311:      */
> 1312:     public byte[] getIssuerAsBytes() throws IOException {
> 1313:         return issuer == null ? null: issuer.getEncoded();

Nit, add space between "null" and ":".

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 686:

> 684:         String n = oid.toString();
> 685:         KnownOIDs os = KnownOIDs.findMatch(n);
> 686:         return os == null? n : os.stdName();

Nit: add space between "null" and "?".

src/java.base/share/classes/sun/security/provider/certpath/Builder.java line 207:

> 205:         case GeneralNameInterface.NAME_NARROWS:
> 206:             /* base is descendant of test */
> 207:             return test.subtreeDepth()-base.subtreeDepth();

Nit: add spaces around "-".

src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1498:

> 1496: 
> 1497:         info.setKey(new CertificateX509Key(req.getSubjectPublicKeyInfo()));
> 1498:         info.setSubject(dname==null?req.getSubjectName():new X500Name(dname));

Add spaces between "?" and ":".

src/java.base/share/classes/sun/security/x509/CertificatePoliciesExtension.java line 185:

> 183:      */
> 184:     public List<PolicyInformation> getCertPolicies() {
> 185:         //XXXX May want to consider cloning this

I would remove this comment. This method is internal and as long as the List is not exposed via a public API (please double-check), a clone is not necessary.

src/java.base/share/classes/sun/security/x509/ExtendedKeyUsageExtension.java line 203:

> 201:      */
> 202:     public Vector<ObjectIdentifier> getUsages() {
> 203:         //XXXX May want to consider cloning this

Remove comment if returned Vector cannot be accessed via public APIs.

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

PR: https://git.openjdk.org/jdk/pull/10959