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

8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently #5679

Closed
wants to merge 1 commit into from

Conversation

masyano
Copy link

@masyano masyano commented Sep 24, 2021

Could you please review the 8250678 bug fixes?

The parse method of ModuleDescriptor.Version class behaves incorrectly when the input string contains consecutive delimiters.

The parse method treats strings as three sections, but the parsing method differs between the method for the version sections and the ones for others. In version sections, the parse method takes a single character in a loop and determines whether it is a delimiter. In pre and build sections, the parse method takes two characters in a loop and determines whether the second character is the delimiter. Therefore, if the string contains a sequence of delimiters in pre or build section, the parse method treats character at the odd-numbered position as a token and the one at even-numbered as a delimiter.

A string containing consecutive delimiters is an incorrect version string, but this behavior does not follow the API specification.
https://download.java.net/java/early_access/jdk18/docs/api/java.base/java/lang/module/ModuleDescriptor.Version.html

Therefore, I propose to fix the parsing method of pre and build section in the same way as the version.


Progress

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

Issue

  • JDK-8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5679/head:pull/5679
$ git checkout pull/5679

Update a local copy of the PR:
$ git checkout pull/5679
$ git pull https://git.openjdk.java.net/jdk pull/5679/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5679

View PR using the GUI difftool:
$ git pr show -t 5679

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5679.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 24, 2021

👋 Welcome back myano! 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 Sep 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2021

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

  • core-libs

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 core-libs label Sep 24, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 24, 2021

Webrevs

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 27, 2021

I think this is okay, just maybe unusual to have repeated punctuation creating the case where a component is empty. @mbreinhold may wish to comment on this PR.

@masyano
Copy link
Author

@masyano masyano commented Sep 30, 2021

@mbreinhold Could you comment on this pull request?

@masyano
Copy link
Author

@masyano masyano commented Oct 6, 2021

@AlanBateman I can't seem to get a comment on this PR from @mbreinhold , so could you please review it?

@mbreinhold
Copy link
Member

@mbreinhold mbreinhold commented Oct 6, 2021

@mbreinhold Could you comment on this pull request?

This is somewhat tricky code. I’ll take a look, but give me a few days.

@masyano
Copy link
Author

@masyano masyano commented Oct 13, 2021

@mbreinhold I waited for about a week, but I haven't received an answer yet. Could you comment on this?

@masyano
Copy link
Author

@masyano masyano commented Oct 18, 2021

@mbreinhold (@AlanBateman ) Could you tell me how many more days do you need for your confirmation?

@masyano
Copy link
Author

@masyano masyano commented Oct 21, 2021

@AlanBateman I have been waiting for a reply from @mbreinhold , but I haven't received it yet. I would like to contribute this PR as soon as possible. Do you have any ideas on how to do it?

@masyano
Copy link
Author

@masyano masyano commented Oct 27, 2021

@mbreinhold @AlanBateman Could you please reply to the above comments?

@masyano
Copy link
Author

@masyano masyano commented Nov 1, 2021

@mbreinhold @AlanBateman Are you aware of my comment? Could you please reply?

@mlchung
Copy link
Member

@mlchung mlchung commented Nov 2, 2021

I reviewed the change. It is reasonable to fix the parsing of the pre-release version and the build version be consistent with parsing of the version number which currently skips consecutive delimiters.

The spec is unclear on whether an empty token separated by the delimiters is ignored. The spec may needs some clarification.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 3, 2021

I reviewed the change. It is reasonable to fix the parsing of the pre-release version and the build version be consistent with parsing of the version number which currently skips consecutive delimiters.

The spec is unclear on whether an empty token separated by the delimiters is ignored. The spec may needs some clarification.

Yes, I think we should fix the consistency issues although the cases where the inconsistency show up seem unusual.

In two minds on whether to should do the spec clarification as part of this PR or create a separate issue.

@masyano
Copy link
Author

@masyano masyano commented Nov 5, 2021

Thank you for reviewing. Does clarifying the spec in this PR mean modifying the comments in javadoc?

@mlchung
Copy link
Member

@mlchung mlchung commented Nov 5, 2021

I would suggest to create a separate issue to follow up the spec clarification and keep this PR to fix the implementation.

The version parsing code is tricky. The fix is straight-forward, just moving the check of the delimiters as the first check when iterating the char sequence. I think it's fine for this fix to go in first. @AlanBateman what do you think?

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 8, 2021

I would suggest to create a separate issue to follow up the spec clarification and keep this PR to fix the implementation.

The version parsing code is tricky. The fix is straight-forward, just moving the check of the delimiters as the first check when iterating the char sequence. I think it's fine for this fix to go in first. @AlanBateman what do you think?

I agree, let's separate the spec clarification to a separate issue and fix the consistency issue with this PR.

@mlchung
Copy link
Member

@mlchung mlchung commented Nov 8, 2021

I created https://bugs.openjdk.java.net/browse/JDK-8276826 to follow up the spec clarification.

mlchung
mlchung approved these changes Nov 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 2021

@masyano 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:

8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently

Reviewed-by: mchung, alanb

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 570 new commits pushed to the master branch:

  • 8747882: 8276790: Rename GenericCDSFileMapHeader::_base_archive_path_offset
  • 38e6d5d: 8276677: Malformed Javadoc inline tags in JDK source in javax/net/ssl
  • a7dedb5: 8276772: Refine javax.lang.model docs
  • 14d66bd: 8276654: element-list order is non deterministic
  • 905e3e8: 8231490: Ugly racy writes to ZipUtils.defaultBuf
  • e383d26: 8275199: Bogus warning generated for serializable records
  • 7e73bca: 8276408: Deprecate Runtime.exec methods with a single string command line argument
  • 75adf54: 8276306: jdk/jshell/CustomInputToolBuilder.java fails intermittently on storage acquisition
  • 7320b77: 8276548: Use range based visitor for Howl-Full cards
  • ea23e73: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
  • ... and 560 more: https://git.openjdk.java.net/jdk/compare/d91e227abb94953129adc297fbd456c55bb2ae10...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 (@mlchung, @AlanBateman) 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 Nov 8, 2021
@masyano
Copy link
Author

@masyano masyano commented Nov 9, 2021

/integrate

@openjdk openjdk bot added the sponsor label Nov 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 9, 2021

@masyano
Your change (at version 86defea) is now ready to be sponsored by a Committer.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 9, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Nov 9, 2021

Going to push as commit e198594.
Since your change was applied there have been 573 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Nov 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 9, 2021

@AlanBateman @masyano Pushed as commit e198594.

💡 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
Labels
core-libs integrated
4 participants