UniqueValue should use UUID::randomUUID for boundary and messageid #460

[jmehrens]

#460

@lukasj Here is the start of changes for this issue but I'm looking for some feedback before I continue. So hold off on merging this.

Here is the todo list:

1. Remove and create an issue for the MailHandlerTest change. Then pull that out of the review.
2. Add entry to the changes.txt
3. I'm unable to get the JavaDoc to build so I can't verify my doc changes yet. I'll get this fixed and check for errors.
4. MimeMessage.updateMessageID needs more documentation

Feedback list:

1. Property values of string instead of boolean. I thought about adding booleans but I think it makes more sense to use string to deal with more types of random string generation including custom classes in the future. Thoughts?
2. Prop key names are mail.mime.messageid.factory and mail.mime.multipart.boundary.factory. I thought about using '.supplier', ,'.class' and, '.pattern' instead of '.factory'. The idea behind pattern would be to allow some sort of printf style pattern. One thing that bugs me is that for both UniqueValue methods we are only changing a specific part of the value with these proeprties and not the whole value. Makes be think I should use '.prefix' and '.suffix' instead of '.factory'. Any other ideas?
3. The old UniqueValue code was as the first call StringBuilder.hashCode(). StringBuilder doesn't override Object.hashCode so I simply changed the calls to identityHashCode. I think this makes the code intent more clear.
4. One call of StringBuilder.hashCode() was being stored in a long with sign. I figure it is best to keep the original behavior but, I wonder if the intent was to convert the hash codes to an unsigned values to get rid of leading dashes.
5. Should I add documentation to MimeMultipart? Looks like other properties are documented there and in the package.html.
6. I assume we want to keep these properties as JakartaMail implementation specific and not spec required.

[lukasj]

Feedback list:

1. Property values of string instead of boolean. I thought about adding booleans but I think it makes more sense to use string to deal with more types of random string generation including custom classes in the future. Thoughts?

If we want to allow this sort of customization, then we should have some contract (interface?) defined for it. Since we don't have it (yet?) or I just don't see it, what about using boolean for now and extending this to allow class name once there will be need for it, or an interface, in place? So in the end, there would be true/false to toggle new/old behaviour (not necessarily in this order) or a class name for custom behaviour. Property names should probably be slightly changed to make it clear they expect boolean in first place. What do you think?

2. Prop key names are mail.mime.messageid.factory and mail.mime.multipart.boundary.factory. I thought about using '.supplier', ,'.class' and, '.pattern' instead of '.factory'. The idea behind pattern would be to allow some sort of printf style pattern. One thing that bugs me is that for both UniqueValue methods we are only changing a specific part of the value with these proeprties and not the whole value. Makes be think I should use '.prefix' and '.suffix' instead of '.factory'. Any other ideas?

no, not yet; it can be caused by my thinking about the previous comment/solution which would probably make this irrelevant

3. The old UniqueValue code was as the first call StringBuilder.hashCode(). StringBuilder doesn't override Object.hashCode so I simply changed the calls to identityHashCode. I think this makes the code intent more clear.

ok

4. One call of StringBuilder.hashCode() was being stored in a long with sign. I figure it is best to keep the original behavior but, I wonder if the intent was to convert the hash codes to an unsigned values to get rid of leading dashes.

sorry, I have no idea what to original intent was in this case. Not sure if checking out the javaee repo would help, so we both might be guessing here.

5. Should I add documentation to MimeMultipart? Looks like other properties are documented there and in the package.html.

No, not to MimeMultipart. But yes, documentation should be added to some com.sun.mail package. A case of simple question and complicated answer :-)

6. I assume we want to keep these properties as JakartaMail implementation specific and not spec required.

Correct and this also implies my answer for (5). Keep in mind that whatever gets added to jakarta.mail classes is required to be supported by all vendors. Such change also means a change in the spec even if it's defined as may not be supported by all implementations and that in turn means additional (paper) work to be done on the release process part. I'm not saying that is a big deal but this does not look to me like something we really have to have in the spec. Definitely not in 1.6.x and preferably not for 2.0.1 targeted to Jakarta EE 9.1. We may consider adding these properties to API for the version targeted to Jakarta EE 10. Till then we should keep changes in the API classes to the acceptable minimum (=typos, blockers for API usage on Java SE 11).

[jmehrens]

@lukasj Thanks for the feedback. This points me in the right direction so I'll work on implementing the changes.

[jmehrens]

@lukasj I've updated the pull request based on the feedback. I'm not too excited about the name of the properties but this is a starting point. I haven't spent a lot of time thinking about what these changes will actually do in the wild and what the impacts are for enabling UUID as default.

1. I still need to add entry to the changes.txt
2. I still need to test JavaDocs.

[lukasj]

And you probably want to move docs to com.sun... package...