gh-54873: Add support for namespaces prefixes to `xml.sax.expatreader` by ukarroum · Pull Request #118317 · python/cpython

ukarroum · 2024-04-26T12:16:19Z

Issue: xml.sax.expatreader should support namespace prefixes #54873

…ader from PyXml (0.8.4)

picnixz

Thanks, but as this is a new feature:

Better tests must be written. Why are we using a new file?
A What's New entry must be added in addition to the NEWS entry.
Documentation must be added.
PyXML is no longer maintained. What about other XML parsers?

picnixz · 2025-11-02T16:42:10Z

+        self.parser.setContentHandler(h)
+        self.parser.feed("<Q:E xmlns:Q='http://example.org/testuri'/>")
+        self.parser.close()
+        print("self.assertEqual")


What are those?

A shamelessly forgot debug print.
I removed it

picnixz · 2025-11-02T16:42:28Z

+        self.parser.feed("<Q:E xmlns:Q='http://example.org/testuri'/>")
+        self.parser.close()
+        print("self.assertEqual")
+        self.assertFalse(h.qname is None)


This is not sufficient. We should match against the exact qname.

picnixz · 2025-11-02T16:42:52Z

@@ -0,0 +1,24 @@
+import unittest


Why are we having a new test file? there should already be a testfile for expat reader.

I assumed all xml unit tests were in the test_xml_* files. Looks like there is a test_sax.py.
Moved test there.

picnixz · 2025-11-02T16:43:23Z

@@ -0,0 +1,2 @@
+Backported namespaces prefixes support for xml.sax.expatreader from PyXml


A What's New entry should be written; this is a new feature. Also, I don't think we should indicate PyXML either. Please look at other changelog entries involving pyexpat to have an idea of how to write them.

Done.
reason why I added the "from PyXml" is that I didn't write the code from scratch.
I backported / copied it from the existing PyXml implementation.

Not sure how to correctly credit the original library.

picnixz · 2025-11-02T16:43:59Z

-        elif name == feature_namespace_prefixes:
-            if state:
-                raise SAXNotSupportedException(
-                    "expat does not report namespace prefixes")


Was it a limitation from the C extension module? was it a Expat version limitation?

I did a quick git blame, and looks like this specific check was added in this commit: 18476a3 (from 2002).
and in this commit Expat version used is 1.95 (which should support namespace prefixes), so I would say the limitation was probably just in the python wrapper.

picnixz · 2025-11-02T16:48:04Z

        pair = name.split()
        if len(pair) == 1:
            # no namespace
+            elem_qname = name


I have no idea whether this is correct or not here. Is there some specs that we could follow for the implementation?

if you're referring specificaly to elem_qname, there is the spec: https://www.w3.org/TR/xml-names/#ns-qualnames.

I also added another test to test the "else" branch.

@picnixz these lines are 1:1 from PyXML 0.8.4:

bedevere-app · 2025-11-02T16:48:13Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

picnixz · 2025-11-02T19:57:13Z

+        parser.feed("<E xmlns='http://example.org/testuri'/>")
+        parser.close()
+        self.assertEqual(h.qname, "E")
+


Don't leave 3 blank lines, only 2 is sufficient.

picnixz · 2025-11-02T19:57:34Z

+        parser.feed("<E xmlns='http://example.org/testuri'/>")
+        parser.close()


Use parser.parse(..., True)

Done, but I didn't add the True, it seems that the parse method only accept one argument.

Oh. Wait, @hartwork should we do feed() + close() or parse()?

@picnixz hi!

My understanding is that:

create_parser is xml.sax.expatreader.create_parser and it creates an xml.sax.xmlreader.XMLReader, first sentence in the docs

xml.sax.xmlreader.XMLReader provides .parse that parses in one single go, one single parameter, no boolean isFinal

xml.sax.xmlreader.XMLReader does not provide .feed or .close because that needs IncrementalParser, not a plain XMLReader.

(xml.sax.expatreader.create_parser creates xml.sax.expatreader.ExpatParser that is an instance of both IncrementalParser and XMLReader for me in practice.)

I therefore consider parser.parse("<E xmlns='http://example.org/testuri'/>") to be better suited here because availability of .feed and .close is not guaranteed on interface level.

(If you want to stick to .feed here or elsewhere maybe add self.assertIsInstance(parser, IncrementalParser) prior to a call to communicate that expectation and have the test fail meaningfully for regressions in the future.)

What do you think?

picnixz · 2025-11-02T19:58:27Z

        self._entity_stack = []
        self._external_ges = 0
        self._interning = None
+        self._namespace_prefixes = 1


Why is it 1 by default?

Changed to use a new namespacePrefixesHandling argument.

picnixz · 2025-11-02T19:59:17Z

        pair = name.split()
        if len(pair) == 1:
            # no namespace
+            elem_qname = name


picnixz · 2025-11-02T19:59:38Z


  .. _billion laughs: https://en.wikipedia.org/wiki/Billion_laughs_attack

+* Add support for namespace prefixes.


This should be also documented in the expat docs.

do you mean I should add a link to the C libexpat doc ?

EDIT: I think you probably meant to document this here: https://docs.python.org/3/library/pyexpat.html

No, to the specs

picnixz · 2025-11-02T20:18:02Z

Sorry, I meant: keep the NEWS entry and add the What's New entry (IOW, both of them must be added).

Changes were made.

picnixz · 2025-11-02T21:34:31Z

cc @hartwork as the maintainer of Expat

hartwork · 2025-11-02T22:18:20Z

cc @hartwork as the maintainer of Expat

@picnixz thanks!

At least for the first half of the starting week my attention will be elsewhere. Also I will feel more free to participate here after #139460 is merged and fully off my plate.

hartwork

Hi @ukarroum and @picnixz,

I believe I understand the ideas behind this pull request now. I have compared with what PyXML 0.8.4 did and will also attach my local play code here (that is derived from the test case in this pull request):

Download: sax_debug.py

I am optimistic that this pull request will be in mergable shape soon. Here is what I found:

hartwork · 2025-11-26T19:00:56Z

+* Add support for `namespace prefixes <https://www.w3.org/TR/xml-names/#dt-prefix>`_.
+  (Contributed by Yassir Karroum in :gh:`118317`.)
+


This is adding to section xml.parsers.expat while the changes in here are patching code elsewhere. (I'm unsure when/if news files should be written to directly. I have only seen things go through news files myself.)

@ukarroum did you see this one? ⬆️

I agree, I will move it to a new section.

I believe it's adding it in two places now, it looks like this upper entry ⬇️ still needs to be dropped?

sorry should have reviewed the diffs one more time before tagging you ^^"
fixed

hartwork · 2025-11-26T20:09:16Z

    """SAX driver for the pyexpat C module."""

-    def __init__(self, namespaceHandling=0, bufsize=2**16-20):
+    def __init__(self, namespaceHandling=0, namespacePrefixesHandling=0, bufsize=2**16-20):


Just thinking aloud: if setFeature can toggle this after initialization and PyXML 0.8.4 did not have this, maybe we do not need this direct channel and can just initialize with = 0 below. So my vote for simplification.

hartwork · 2025-11-26T20:11:44Z

+        elif name == feature_namespace_prefixes:
+            self._namespace_prefixes = state


My vote for moving this one further down — right after elif name == feature_external_pes: — to match the left side of the diff.

hartwork · 2025-11-26T20:29:39Z

+            elem_qname = name
            pair = (None, name)
        elif len(pair) == 3:
+            elem_qname = "%s:%s" % (pair[2], pair[1])


Note to self: This is the key value provider in this pull request. pair[2] was previously not exposed.

hartwork · 2025-11-26T20:37:00Z

        pair = name.split()
        if len(pair) == 1:
            # no namespace
+            elem_qname = name


@picnixz these lines are 1:1 from PyXML 0.8.4:

hartwork · 2025-11-26T20:50:49Z

@@ -0,0 +1,2 @@
+Backported namespaces prefixes support for xml.sax.expatreader from PyXml


Suggested change

Backported namespaces prefixes support for xml.sax.expatreader from PyXml

Backported namespaces prefixes support for xml.sax.expatreader from PyXML

hartwork · 2025-11-26T21:18:08Z

+        for xml_s, expected_qname in zip(["<Q:E xmlns:Q='http://example.org/testuri'/>", "<E xmlns='http://example.org/testuri'/>", "<E />"], ["Q:E", "E", "E"]):
+            parser = create_parser()
+            parser.setFeature(handler.feature_namespaces, 1)
+            parser.setFeature(handler.feature_namespace_prefixes, 1)


I believe there should also be a test for the behavior with this disabled where all QNames returned are None as previously.

github-actions · 2026-04-16T06:36:54Z

This PR is stale because it has been open for 30 days with no activity.

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

…x-issue-54873

hartwork · 2026-04-17T14:57:16Z

-
 from xml.sax import make_parser, ContentHandler, \
                    SAXException, SAXReaderNotAvailable, SAXParseException
+from xml.sax.handler import feature_namespaces, feature_namespace_prefixes


Can this line be dropped? It looks like both of these symbols are imported twice now?

Indeed, removed it.

hartwork · 2026-04-17T14:57:25Z

+* Add support for `namespace prefixes <https://www.w3.org/TR/xml-names/#dt-prefix>`_.
+  (Contributed by Yassir Karroum in :gh:`118317`.)
+


@ukarroum did you see this one? ⬆️

ukarroum · 2026-04-17T15:37:44Z

@hartwork thanks a lot for the review !
I addressed all comments please let me know if you see any other issue

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

…x-issue-54873

hartwork · 2026-04-17T15:46:46Z

-            if state:
-                raise SAXNotSupportedException(
-                    "expat does not report namespace prefixes")
+            self._namespace_prefixes = state


@ukarroum I wonder what the expected behaviors is here when feature_namespaces is currently disabled. Or more generally speaking: Do we know anything about the relation between these two features? Potential options seem to be:

Block enabling/disabling when their dependency would be violated.

Auto-enable/disable to respect dependency at all times

Keep the two fully separate but check for both rather than just self._namespace_prefixes where checking for being active?

Just thinking aloud.

good point, I think it will make sense to just enable feature_namespaces when feature_namespace_prefixes is also enabled (and also disable feature_namespace_prefixes if feature_namespaces is disabled).
I will update both set/get feature

actually one drawback I see for this is :

enable feature_namespace_prefixes
disable feature_namespaces
enable feature_namespaces

this will result in feature_namespace_prefixes being disabled which feels odd.

I went with 1) instead.
I pushed a small change to check if namespace is enabled when we enable namespace prefixes.

@ukarroum I like it. I think one small dedicated test for that would be great where namespaces are disabled and the exception is raise with the expected message. (Arguably, the success case is already covered by existing tests.)

hartwork · 2026-04-17T15:47:56Z

@hartwork thanks a lot for the review ! I addressed all comments please let me know if you see any other issue

@ukarroum thanks for the adjustments! I don't have any approval or merge permissions here, but from my point of view it's close to mergable by now.

ukarroum · 2026-04-17T16:23:11Z

I have made the requested changes; please review again

bedevere-app · 2026-04-17T16:23:18Z

Thanks for making the requested changes!

: please review the changes made to this pull request.

…ce is not

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

hartwork · 2026-04-17T17:28:46Z

@ukarroum thanks for the adjustments!

@picnixz @AA-Turner I believe this is ready for official review now, could you have a look? Thank you! 🙏

bpo-54873: Backported namespaces prefixes support for xml.sax.expatre…

6ecfc28

…ader from PyXml (0.8.4)

bedevere-app bot mentioned this pull request Apr 26, 2024

xml.sax.expatreader should support namespace prefixes #54873

Open

bedevere-app bot added the awaiting review label Apr 26, 2024

ukarroum added 4 commits April 26, 2024 14:21

bpo-54873: Added News entry

e9ac454

Merge branch 'main' into fix-issue-54873

9c365c2

Merge branch 'main' into fix-issue-54873

2fc666a

Merge branch 'main' into fix-issue-54873

fe9d82f

picnixz previously requested changes Nov 2, 2025

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Nov 2, 2025

Moved test_namespace_prefix to test_sax.py

be878cb

ukarroum requested a review from AA-Turner as a code owner November 2, 2025 19:52

picnixz reviewed Nov 2, 2025

View reviewed changes

picnixz self-requested a review November 2, 2025 20:18

bedevere-app bot added awaiting review and removed awaiting changes labels Nov 2, 2025

ukarroum added 2 commits November 2, 2025 21:14

test_sax: Replaced namespace tests with one test on qualified names

0e3f870

ExpatParser: Add namespacePrefixesHandling

c88e7ed

picnixz changed the title ~~gh-54873: Backported namespaces prefixes support for xml.sax.expatreader from PyXml (0.8.4)~~ gh-54873: Add support for namespaces prefixes to xml.sax.expatreader Nov 2, 2025

namespace prefixes: add link to w3.org

27cb273

Merge branch 'main' into fix-issue-54873

91112b9

ukarroum requested a review from hartwork November 24, 2025 04:02

hartwork suggested changes Nov 26, 2025

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Nov 26, 2025

github-actions bot added the stale Stale PR or inactive for long period of time. label Apr 16, 2026

ukarroum and others added 3 commits April 17, 2026 07:41

Merge branch 'main' into fix-issue-54873

d3cbe5f

Update imports in Lib/test/test_sax.py

1d343f1

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

Approuved readibility of unit test

256a97b

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

hartwork reviewed Apr 17, 2026

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2024-04-26-14-21-04.gh-issue-54873.vf2bfp.rst Outdated

ukarroum and others added 4 commits April 17, 2026 16:15

Update pyXML spelling in News.d

7c0da60

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

Update Lib/xml/sax/expatreader.py

e219450

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

Addressed PR comments

34af031

Merge branch 'fix-issue-54873' of github.com:ukarroum/cpython into fi…

8075f8b

…x-issue-54873

hartwork reviewed Apr 17, 2026

View reviewed changes

ukarroum added 2 commits April 17, 2026 17:33

Removed unused import

2004846

Update news entry

c3e00e2

hartwork reviewed Apr 17, 2026

View reviewed changes

Comment thread Doc/whatsnew/3.15.rst Outdated

ukarroum and others added 3 commits April 17, 2026 17:38

Update Doc/whatsnew/3.15.rst

d3b08ec

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

Update news entry

e8bcb30

Merge branch 'fix-issue-54873' of github.com:ukarroum/cpython into fi…

dfff704

…x-issue-54873

hartwork reviewed Apr 17, 2026

View reviewed changes

Add a check on namespace feature when enabling namespace prefixes

7de4ce5

bedevere-app bot added awaiting change review and removed awaiting core review labels Apr 17, 2026

Add test for feature_namespace_prefixes enabled while feature_namespa…

08d659e

…ce is not

hartwork reviewed Apr 17, 2026

View reviewed changes

Comment thread Lib/test/test_sax.py Outdated

ukarroum and others added 2 commits April 17, 2026 19:22

Update Lib/test/test_sax.py

7cdc402

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

Update test

cd7de73

		@@ -0,0 +1,2 @@
		Backported namespaces prefixes support for xml.sax.expatreader from PyXml

		parser.feed("<E xmlns='http://example.org/testuri'/>")
		parser.close()


		.. _billion laughs: https://en.wikipedia.org/wiki/Billion_laughs_attack

		* Add support for namespace prefixes.

		* Add support for `namespace prefixes <https://www.w3.org/TR/xml-names/#dt-prefix>`_.
		(Contributed by Yassir Karroum in :gh:`118317`.)

		elif name == feature_namespace_prefixes:
		self._namespace_prefixes = state

Uh oh!

Conversation

ukarroum commented Apr 26, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Nov 2, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hartwork Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ukarroum Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz commented Nov 2, 2025

Uh oh!

picnixz commented Nov 2, 2025

Uh oh!

hartwork commented Nov 2, 2025

Uh oh!

hartwork left a comment

Choose a reason for hiding this comment

Uh oh!

ukarroum commented Apr 26, 2024 •

edited by bedevere-app bot

Loading

hartwork Nov 2, 2025 •

edited

Loading

ukarroum Nov 2, 2025 •

edited

Loading