On Mon, 16 Nov 2020 18:20:02 GMT, Weijun Wang <[email protected]> wrote:
>> src/java.base/share/classes/java/security/spec/MGF1ParameterSpec.java line
>> 168:
>>
>>> 166: @Override
>>> 167: public String toString() {
>>> 168: return "MGF1:" + mdName;
>>
>> I would replace "MGF1" or perhaps add "DigestAlgorithm" which is the name of
>> the attribute. Is it necessary to print that this is an MGF1?
>> PSSParameterSpec does not print that it is an RSASSA-PSS-params, and also
>> prints "MGF", so it seems there would be some duplication. It almost seems
>> like we should have some rules regarding how these parameters are printed
>> out so everything is consistent.
>>
>> Or perhaps it makes sense to have brackets around the fields. Otherwise when
>> you chain several toStrings together, it makes it more difficult to discern
>> when one field ends and the next begins. Hmm.
>
> "MGF1" is only one kind of MGF and we might see "MGF2" in the future so it's
> worth printing out. Of course, `PSSParameterSpec` already has a
> `getMGFAlgorithm()` so the name can either be printed in the `toString` of
> the outer data type or the inner one.
>
> As for `DigestAlgorithm` I don't think it's necessary because it's the only
> field for MGF1 so there's no ambiguity.
>
> That said, we can try to provide some consistency here. If the types had been
> defined as records with
> static record MGF1Params(String messageDigest) {}
> static record PssParams(String messageDigest, String mgfAlgorithm,
> MGF1Params mgfParams, int saltLength, int trailerField) {}
> The automatically generated `toString` would output something like
> PssParams[messageDigest=SHA-1, mgfAlgorithm=MGF1,
> mgfParams=MGF1Params[messageDigest=SHA-1], saltLength=20, trailerField=1]
> It's a little verbose. Do you like this style?
I like the brackets surrounding the fields of each type. I think we should try
to use brackets in the toString methods of security classes as there is often a
deep hierarchy of objects and the brackets helps to see that. I also like
seeing the actual type for non-primitives, and not an abbreviated form. As for
the names of the fields, I'm ok with whatever seems most reasonable, the ASN.1
names, or the method or parameter names.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1208
