Skip to content
This repository was archived by the owner on May 26, 2020. It is now read-only.

SANTUARIO-534 Fixed DOMNamespaceContext#23

Closed
peterdemaeyer wants to merge 1 commit intoapache:trunkfrom
peterdemaeyer:SANTUARIO-534
Closed

SANTUARIO-534 Fixed DOMNamespaceContext#23
peterdemaeyer wants to merge 1 commit intoapache:trunkfrom
peterdemaeyer:SANTUARIO-534

Conversation

@peterdemaeyer
Copy link
Copy Markdown

@peterdemaeyer peterdemaeyer commented Apr 11, 2020

Fixed DOMNamespaceContext such that it correctly implements the contract defined by NamespaceContext + made context node settable.

@peterdemaeyer peterdemaeyer changed the title SANTUARIO-534 DOMNamespaceContext now implements the NamespaceContext… SANTUARIO-534 Fixed DOMNamespaceContext Apr 11, 2020
@@ -0,0 +1,122 @@
package org.apache.xml.security.utils;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add an Apache license header here.


public String getNamespaceURI(String arg0) {
return namespaceMap.get(arg0);
public void setContext(Node context) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this only used for testing? It would be better to remove it and make context final.

Copy link
Copy Markdown
Author

@peterdemaeyer peterdemaeyer Apr 16, 2020

Choose a reason for hiding this comment

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

It's only used in tests, but not "for testing".
The test "illustrates the intent".
It's not used here yet in production code, but it's going to be used in SANTUARIO-532, which is what I was referring to in my comment on the JIRA issue SANTUARIO-534:

  1. Added DOMNamespaceContext.setContext(Node context) to allow reusing a single instance with multiple Node contexts, in anticipation of using it in SANTUARIO-532.

If you insist I can remove it here, but I was hoping to keep it in anticipation of a future PR... It would be more hassle for me to remove it here (including the unit test that illustrates its purpose), and then having to reintroduce it later.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK if it's needed for the other PR then it's fine to leave it.

for (Entry<String, String> entry : namespaceMap.entrySet()) {
if (entry.getValue().equals(arg0)) {
return entry.getKey();
public String getNamespaceURI(String prefix) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For getNamespaceURI, I'm wondering if it should be falling back to returning "" instead of return null? From the javadoc:

unbound prefix | XMLConstants.NULL_NS_URI("")

Comment on lines +29 to +31
/**
* @author Peter De Maeyer
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove, we don't use @author tags

@peterdemaeyer peterdemaeyer force-pushed the SANTUARIO-534 branch 3 times, most recently from b897aa9 to a918d77 Compare April 17, 2020 05:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants