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

8276141: XPathFactory set/getProperty method #6215

Closed
wants to merge 11 commits into from

Conversation

JoeWang-Java
Copy link
Member

@JoeWang-Java JoeWang-Java commented Nov 2, 2021

Add setProperty/getProperty methods to the XPath API so that properties can be supported in the future.


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6215

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

Using diff file

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

@JoeWang-Java
Copy link
Member Author

@JoeWang-Java JoeWang-Java commented Nov 2, 2021

/csr

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 2, 2021

👋 Welcome back joehw! 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 Nov 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 2, 2021

@JoeWang-Java this pull request will not be integrated until the CSR request JDK-8276143 for issue JDK-8276141 has been approved.

@openjdk openjdk bot added the csr label Nov 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 2, 2021

@JoeWang-Java 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 Nov 2, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 2, 2021

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 2, 2021

Are you planning to add tests? Also don't forget @SInCE.

@JoeWang-Java
Copy link
Member Author

@JoeWang-Java JoeWang-Java commented Nov 3, 2021

Thanks Alan. Added "since"; Added default impl; Added tests. Note that I removed the run with security manager from this test. As the Security Manager is deprecated, the run with security manager in tests unrelated to secure processing will need to be removed.

*
* @implSpec
* The default implementation in the XPath API throws
* {@link java.lang.UnsupportedOperationException}.
Copy link
Contributor

@AlanBateman AlanBateman Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can drop "in the XPath API" from the sentence.

Copy link
Member Author

@JoeWang-Java JoeWang-Java Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped.

* @return the value of the property.
*
* @throws XPathFactoryConfigurationException if the property name is not
* recognized
Copy link
Contributor

@AlanBateman AlanBateman Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure XPathFactoryConfigurationException is the right exception here? This exception suggests "a configuration error in a XPathFactory environment". I would expect something like IllegalArgumentException here.

Copy link
Member Author

@JoeWang-Java JoeWang-Java Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, IAE would have been more appropriate. The only reason I went with XCE was consistency among the XPath API. It's unfortunate it has not been consistent among the XML libs. The DOM/StAX/XSLT went with IAE, SAX/Validation SAXNotRecognizedException and SAXNotSupportedException, and then XPath XCE which was designed for config error. But if we don't plan to fix XPathFactory::set/getFeature, should we go with sth different for the property methods?

Copy link
Member Author

@JoeWang-Java JoeWang-Java Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger also suggested changing to IAE. I've just pushed a change to IAE. Thanks.

throw new NullPointerException("the name parameter is null");
}
throw new UnsupportedOperationException("not implemented");
}
Copy link
Contributor

@AlanBateman AlanBateman Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it to throw IAE looks much better. You can drop "throws IllegalArgumentException" from the method signature as it's a runtime exception.

Copy link
Member Author

@JoeWang-Java JoeWang-Java Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IAE dropped from the method signature.


/**
* Sets a property for this {@code XPathFactory} and {@code XPath}
* created by this factory.
Copy link
Contributor

@AlanBateman AlanBateman Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are properties inherited by the XPath object at create time? The javadoc for newXPath suggests so and I'm wondering if the javadoc for setProperty needs to consistent with that. Maybe the intention is really that this method sets a XPathFactory property and it is used when configuring the XPath object returned by the newXPath method.

Copy link
Member Author

@JoeWang-Java JoeWang-Java Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Made a clarification to the javadoc, and while we are here, to setFeature method as well.

Copy link
Contributor

@AlanBateman AlanBateman Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other suggestion for wording is to change:
"A property set on the factory is effective to the XPath object created thereafter"
to something like:
"The property applies to XPath objects that the XPathFactory creates. It has no impact on XPath objects that are already created."

I don't have any other comments on the API.

Copy link
Member Author

@JoeWang-Java JoeWang-Java Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording is adjusted accordingly. Thanks.

Copy link
Contributor

@AlanBateman AlanBateman Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording is adjusted accordingly. Thanks.

Thanks for the update. The API additions (and clarification) look good to me. Do you have other reviewers for the impl change and tests?

Copy link
Member Author

@JoeWang-Java JoeWang-Java Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, Alan. I'll ask around for help.

/**
* Sets a property for this {@code XPathFactory}. A property set on the
* factory is effective to the {@code XPath} object created thereafter, but
* not ones that may have been created beforehand.
Copy link
Contributor

@AlanBateman AlanBateman Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be clear to say something like "The property has no impact on XPath object that are already created".

Copy link
Member Author

@JoeWang-Java JoeWang-Java Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Alan. Updated accordingly.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Looks good.

* or the value can not be assigned
* @throws UnsupportedOperationException if the implementation does not
* support the method
* @throws NullPointerException if the {@code name} is {@code null}.
Copy link
Contributor

@RogerRiggs RogerRiggs Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the "." at the end of the sentence (to be consistent with the other throws).

* @throws IllegalArgumentException if the property name is not recognized
* @throws UnsupportedOperationException if the implementation does not
* support the method
* @throws NullPointerException if the {@code name} is {@code null}.
Copy link
Contributor

@RogerRiggs RogerRiggs Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.


// property name not recognized
String fmsg = XSLMessages.createXPATHMessage(
XPATHErrorResources.ER_GETTING_UNKNOWN_PROPERTY,
Copy link
Member

@naotoj naotoj Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be ER_PROPERTY_UNKNOWN?

@JoeWang-Java
Copy link
Member Author

@JoeWang-Java JoeWang-Java commented Nov 18, 2021

Thanks Roger, Naoto. Fixed as suggested.

naotoj
naotoj approved these changes Nov 19, 2021
Copy link
Member

@irisclark irisclark left a comment

Associated CSR also Reviewed.

@openjdk openjdk bot removed the csr label Nov 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 2021

@JoeWang-Java 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:

8276141: XPathFactory set/getProperty method

Reviewed-by: rriggs, naoto, lancea, iris, 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 362 new commits pushed to the master branch:

  • 98a9f03: 8277602: Deopt code does not extend the stack enough if the caller is an optimize entry blob
  • 9150840: 8277899: Parallel: Simplify PSVirtualSpace::initialize logic
  • 01cefc9: 8277977: Incorrect references to --enable-reproducible-builds in docs
  • 69f56a0: 8264485: build.tools.depend.Depend.toString(byte[]) creates malformed hex strings
  • fecf906: 8267928: Loop predicate gets inexact loop limit before PhaseIdealLoop::rc_predicate
  • a5f2a58: 8277846: Implement fast-path for ASCII-compatible CharsetEncoders on ppc64
  • ceae380: 8277843: [Vector API] scalar2vector generates incorrect type info for mask operations if Op_MaskAll is unavailable
  • 3ee26c6: 8267767: Redundant condition check in SafepointSynchronize::thread_not_running
  • d230fee: 8277931: Parallel: Remove unused PSVirtualSpace::expand_into
  • fde6fe7: 8277824: Remove empty RefProcSubPhasesWorkerTimeTracker destructor
  • ... and 352 more: https://git.openjdk.java.net/jdk/compare/495c828ae95205885b091dde795b517ba332a2b1...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 30, 2021
@JoeWang-Java
Copy link
Member Author

@JoeWang-Java JoeWang-Java commented Nov 30, 2021

Hi all, refer to Joe's comment on the CSR, adding a statement to clarify where properties may be defined, that is, the method may be used for setting properties that may have been defined by the spec or the underlying impl.

@JoeWang-Java
Copy link
Member Author

@JoeWang-Java JoeWang-Java commented Dec 2, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2021

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

  • 09522db: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
  • 67745fa: 8276299: G1: Unify the wording buffer/node/element in G1SegmentedArrayXxx, G1CardSetXxx and related classes
  • 51d6d7a: 8266839: Enable pandoc on macosx-aarch64 at Oracle
  • 0dfb3a7: 8268582: javadoc throws NPE with --ignore-source-errors option
  • f41e768: 8277762: Allow configuration of HOTSPOT_BUILD_USER
  • a363b7b: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST
  • 9b3e672: 8278014: [vectorapi] Remove test run script
  • 1e9ed54: 8193682: Infinite loop in ZipOutputStream.close()
  • abaa073: 8277946: NMT: Deprecate and remove VM.native_memory shutdown jcmd command option
  • 37ff7f3: 8277866: gc/epsilon/TestMemoryMXBeans.java failed with wrong initial heap size
  • ... and 383 more: https://git.openjdk.java.net/jdk/compare/495c828ae95205885b091dde795b517ba332a2b1...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 2, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2021

@JoeWang-Java Pushed as commit b226ab9.

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

@JoeWang-Java JoeWang-Java deleted the JDK-8276141 branch Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
6 participants