On Mon, 16 Nov 2020 18:20:02 GMT, Weijun Wang <wei...@openjdk.org> 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

Reply via email to