Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior #1701

Closed
wants to merge 4 commits into from

Conversation

@haimaychao
Copy link
Contributor

@haimaychao haimaychao commented Dec 8, 2020

This is a spec change with noreg-doc label.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1701/head:pull/1701
$ git checkout pull/1701

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 8, 2020

👋 Welcome back hchao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Dec 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

@haimaychao The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security label Dec 8, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 8, 2020

@@ -316,6 +316,7 @@ public abstract void engineStore(OutputStream stream, char[] password)
* algorithm could not be found
* @throws CertificateException if any of the certificates included in
* the keystore data could not be stored
* @throws UnsupportedOperationException if this method is not supported
Copy link
Member

@XueleiFan XueleiFan Dec 8, 2020

We might be able to get it more clear about the behavior. For example, adding an implSpec tag, like:

     * @implSpec The default implementation throws
     *           an {@link UnsupportedOperationException}.
     ...
     * @throws UnsupportedOperationException if the implementation
     *         has not overridden this method

Copy link
Contributor Author

@haimaychao haimaychao Dec 8, 2020

tag implSpec added. Thanks for the review.

@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Dec 8, 2020

There is a "Title mismatch between PR and JBS" issue. I think it could be solved by use the same title/subject for the pull request and bug in JBS.

@@ -316,6 +319,8 @@ public abstract void engineStore(OutputStream stream, char[] password)
* algorithm could not be found
* @throws CertificateException if any of the certificates included in
* the keystore data could not be stored
* @throws UnsupportedOperationException if the implementation
Copy link
Member

@XueleiFan XueleiFan Dec 8, 2020

Please add an extra white space after the "throws" tag so that this line align with the lines above.

Copy link
Member

@seanjmullan seanjmullan Dec 8, 2020

On line 310 and 315, can you also fix the typos as part of this change: s/KeyStore.LoadStoreParmeter/KeyStore.LoadStoreParameter/

Copy link
Contributor Author

@haimaychao haimaychao Dec 8, 2020

Added the white space. Sure, I can include those typos fix in this PR. Also, included another typo fix for line 305.

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Dec 8, 2020

This will also require a CSR since you are making some specification changes. I'm not sure if you were trying to get this into 16, but it is probably too late to make JDK 16 since RDP is a couple of days away. You can push to 17/latest once 16 forks and CSR is approved.

@haimaychao haimaychao changed the title 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch t… 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior Dec 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

@haimaychao This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior

Reviewed-by: xuelei

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 278 new commits pushed to the master branch:

  • 763623d: 8258524: Instrumented EventHandler calls private instance method EventWriter.reset
  • 6e824b3: 8258408: SystemDictionary passes TRAPS to functions that don't throw exceptions
  • 9ed0b76: 8254850: Update terminology in java.awt.GridBagLayout source code comments
  • 7b05439: 8258057: serviceability/attach/RemovingUnixDomainSocketTest.java doesn't ignore VM warnings
  • 143998e: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array
  • 8251b74: 8257530: vmTestbase/metaspace/stressDictionary/StressDictionary.java timed out
  • 6aa8eed: 8258415: gtest for committed memory leaks reservation
  • 83be8a9: 8247732: validate user-input intrinsic_ids in ControlIntrinsic
  • 178c001: 8258479: Minor cleanups in VMError
  • c11525a: 8257772: Vectorizing clear memory operation using AVX-512 masked operations
  • ... and 268 more: https://git.openjdk.java.net/jdk/compare/03f3b8eadd89f33a027b59c68c4af5f40b0b65ff...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@XueleiFan) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Dec 8, 2020
@@ -316,6 +319,8 @@ public abstract void engineStore(OutputStream stream, char[] password)
* algorithm could not be found
* @throws CertificateException if any of the certificates included in
* the keystore data could not be stored
* @throws UnsupportedOperationException if the implementation
Copy link
Member

@seanjmullan seanjmullan Dec 8, 2020

On line 310 and 315, can you also fix the typos as part of this change: s/KeyStore.LoadStoreParmeter/KeyStore.LoadStoreParameter/

@@ -316,6 +319,8 @@ public abstract void engineStore(OutputStream stream, char[] password)
* algorithm could not be found
* @throws CertificateException if any of the certificates included in
* the keystore data could not be stored
* @throws UnsupportedOperationException if the implementation
* has not overridden this method
Copy link
Member

@seanjmullan seanjmullan Dec 8, 2020

I think we should keep this more general and just say "If the implementation does not support this operation". There may be legitimate cases in which an implementation may override this method but throw UnsupportedOperationException if it doesn't support particular aspects of the LoadStoreParameter.

Copy link
Contributor Author

@haimaychao haimaychao Dec 8, 2020

Good point. Fixed as suggested.

@@ -1421,6 +1421,7 @@ public final void store(OutputStream stream, char[] password)
* algorithm could not be found
* @throws CertificateException if any of the certificates included in
* the keystore data could not be stored
* @throws UnsupportedOperationException if this method is not supported
Copy link
Member

@seanjmullan seanjmullan Dec 8, 2020

I would change "method" to "operation" as it is a bit more consistent with the exception.

Copy link
Contributor Author

@haimaychao haimaychao Dec 8, 2020

Fixed as suggested. Thanks for the review.

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Dec 11, 2020

Please review the CSR (JDK-8258114) at:
https://bugs.openjdk.java.net/browse/JDK-8258114

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 11, 2020

Mailing list message from Jamil Nimeh on security-dev:

Some minor comments:

* General
o I think you can backtick the UnsupportedOperationException text
so it shows up in monospace format.
* Specification
o I think text-wise the wording is fine.? IIRC don't we normally
show the proposed changes as a diff?? Or is that just one
acceptable format and what's done here is fine too? Either way
the intent is clearly communicated, IMO.

I'll put myself down as a reviewer.

--Jamil

On 12/11/2020 10:37 AM, Hai-May Chao wrote:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20201211/562f1ab6/attachment.htm>

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Dec 11, 2020

Thanks for the review. Updated CSR to backtick the UnsupportedOperationException text. I thought about placing the diff in Specification section, but decided to keep the document change in this format which is same as our javadoc API specification. I thought it'd be more clear. I'm open to make a change if diff format is required.

@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Dec 11, 2020

It may improve the readability to have the context specification of the update. For example, have the full specification of the method, and mark the update of the spec, and the signature of the method. A diff is often used as it is easier to have the context in the CSR request.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 11, 2020

Mailing list message from Jamil Nimeh on security-dev:

No need to change it IMO.? I'm fine with it as is.--Jamil
-------- Original message --------From: Hai-May Chao <hchao at openjdk.java.net> Date: 12/11/20 11:45 AM (GMT-08:00) To: security-dev at openjdk.java.net Subject: Re: RFR: 8246005: KeyStoreSpi::engineStore(LoadStoreParameter) spec mismatch to its behavior [v2] On Fri, 11 Dec 2020 18:35:27 GMT, Hai-May Chao <hchao at openjdk.org> wrote:>> This will also require a CSR since you are making some specification changes. I'm not sure if you were trying to get this into 16, but it is probably too late to make JDK 16 since RDP is a couple of days away. You can push to 17/latest once 16 forks and CSR is approved.>> Please review the CSR (JDK-8258114) at:> https://bugs.openjdk.java.net/browse/JDK-8258114Thanks for the review. Updated CSR to backtick the UnsupportedOperationException text. I thought about placing the diff in Specification section, but decided to keep the document change in this format which is same as our javadoc API specification. I thought it'd be more clear. I'm open to make a change if diff format is required.-------------PR: https://git.openjdk.java.net/jdk/pull/1701
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20201211/1f07d94b/attachment-0001.htm>

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Dec 11, 2020

Thanks for the CSR review. Updated CSR to have diff on the full spec of the methods.

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Dec 12, 2020

/csr

@openjdk openjdk bot added the csr label Dec 12, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 12, 2020

@haimaychao this pull request will not be integrated until the CSR request JDK-8258114 for issue JDK-8246005 has been approved.

@openjdk openjdk bot added ready and removed ready csr labels Dec 12, 2020
@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Dec 17, 2020

/integrate

@openjdk openjdk bot added the sponsor label Dec 17, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2020

@haimaychao
Your change (at version 12db794) is now ready to be sponsored by a Committer.

@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Dec 17, 2020

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2020

@XueleiFan @haimaychao Since your change was applied there have been 278 commits pushed to the master branch:

  • 763623d: 8258524: Instrumented EventHandler calls private instance method EventWriter.reset
  • 6e824b3: 8258408: SystemDictionary passes TRAPS to functions that don't throw exceptions
  • 9ed0b76: 8254850: Update terminology in java.awt.GridBagLayout source code comments
  • 7b05439: 8258057: serviceability/attach/RemovingUnixDomainSocketTest.java doesn't ignore VM warnings
  • 143998e: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array
  • 8251b74: 8257530: vmTestbase/metaspace/stressDictionary/StressDictionary.java timed out
  • 6aa8eed: 8258415: gtest for committed memory leaks reservation
  • 83be8a9: 8247732: validate user-input intrinsic_ids in ControlIntrinsic
  • 178c001: 8258479: Minor cleanups in VMError
  • c11525a: 8257772: Vectorizing clear memory operation using AVX-512 masked operations
  • ... and 268 more: https://git.openjdk.java.net/jdk/compare/03f3b8eadd89f33a027b59c68c4af5f40b0b65ff...master

Your commit was automatically rebased without conflicts.

Pushed as commit b0b70df.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants