Change namespace cache strategy#333
Open
tompng wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors REXML namespace resolution to incorporate ATTLIST-declared xmlns values (and adds caching in the XPath parser), along with a new regression test that validates namespace precedence.
Changes:
- Add support for applying ATTLIST-declared xmlns values into element/attribute namespace resolution.
- Introduce per-parser caching for element namespace computations in
XPathParser. - Add a new test for ATTLIST vs element-declared namespace precedence; remove the previous namespaces cache regression test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/xpath/test_base.rb | Removes a regression test that previously exercised namespace cache invalidation behavior. |
| test/test_core.rb | Adds a test asserting ATTLIST-declared xmlns precedence vs inherited/locally-declared namespaces. |
| lib/rexml/xpath_parser.rb | Adds namespace caching and routes namespace comparisons through cached lookup helpers. |
| lib/rexml/element.rb | Refactors namespace calculation/lookup internals and integrates ATTLIST-declared namespaces. |
| lib/rexml/document.rb | Adds extraction of ATTLIST-declared xmlns mappings per element name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+66
to
+68
| @attrlist_per_element_namespaces = nil | ||
| @document = nil | ||
| @element_namespaces_cache = {} |
Comment on lines
+91
to
+92
| @document = node.document | ||
| match( path_stack, node ) |
Comment on lines
+195
to
+198
| def element_namespaces(element) | ||
| @attrlist_per_element_namespaces ||= @document&.send(:attrlist_per_element_namespaces) || {} | ||
| element.send(:calculate_namespaces, @element_namespaces_cache, @attrlist_per_element_namespaces) | ||
| end |
Comment on lines
+183
to
+185
| def attribute_namespace(attribute) | ||
| attribute.prefix == '' ? '' : element_namespace_lookup(attribute.element, attribute.prefix) | ||
| end |
| def calculate_namespaces(cache_hash = nil, attrlist_element_namespaces = nil) | ||
| return cache_hash[self] if cache_hash && cache_hash.key?(self) | ||
|
|
||
| inherited_namespaces = parent ? parent.send(:calculate_namespaces, cache_hash, attrlist_element_namespaces) : {} |
Comment on lines
+1525
to
+1530
| # Inherited namespaces can be overridden by attribute list declaration, and both can be overridden by its own attributes | ||
| namespaces = inherited_namespaces | ||
| namespaces = namespaces.merge(attrlist_namespaces) if attrlist_namespaces | ||
| namespaces = namespaces.merge(own_namespaces) if own_namespaces.any? | ||
| cache_hash[self] = namespaces if cache_hash | ||
| namespaces |
| assert_equal correct, prefixes | ||
| end | ||
|
|
||
| def test_attrlist_namespace_priority |
Comment on lines
+195
to
+198
| def element_namespaces(element) | ||
| @attrlist_per_element_namespaces ||= @document&.send(:attrlist_per_element_namespaces) || {} | ||
| element.send(:calculate_namespaces, @element_namespaces_cache, @attrlist_per_element_namespaces) | ||
| end |
Namespace calculation for each node cant' be cached to document because document lookup is slow for deeply nested nodes. Change namespace cache strategy, inject cached hash as an argument to retrieve namespace/namespaces from XPath match operation.
tompng
force-pushed
the
xpath_namespace_lookup_cache
branch
from
8d141ec to
8a7840a
Compare
tompng
commented
Jun 14, 2026
| @@ -2424,19 +2447,12 @@ def prefixes | |||
| # d.root.attributes.namespaces # => {"xmlns"=>"foo", "x"=>"bar", "y"=>"twee"} | |||
| # | |||
| def namespaces | |||
Member
Author
There was a problem hiding this comment.
This method is no longer called except from the added test.
We need to keep this if it's a public API, but if not, we can remove it (perhaps with deprecation first)
tompng
commented
Jun 14, 2026
Comment on lines
+1527
to
+1528
| namespaces = namespaces.merge(attlist_namespaces) if attlist_namespaces | ||
| namespaces = namespaces.merge(own_namespaces) if own_namespaces.any? |
Member
Author
There was a problem hiding this comment.
There was a small bug in master: precedence of attlist attributes and own attirbutes was reversed. Test is added for this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Even if namespace lookup is cached, namespace lookup was slow for deeply nested XML nodes.
Namespaces are cached in
document, butdocumentlookup from a deep node costsO(n).We can't cache it in a document or root of the node, because we also need to cache document lookup result.
It's not good to cache to node itself, for maintainability reason, cache invalidation, etc.
In this PR, Hash for caching node's namespace/namespaces, and cached namespace attributes defined in attrlist decl can be injected through method arguments.
Benchmark
Time consumption was: Scan: 0.0046s (1.8%), Sort: 0.17s (60%), Namespace lookup: 0.11s (38%). This PR removes the 38% cost.