Skip to content

Make SAX parsing consistent on all adapters - #83

Open
HassanAkbar wants to merge 8 commits into
mainfrom
xml-numeric-entities-parsing
Open

Make SAX parsing consistent on all adapters#83
HassanAkbar wants to merge 8 commits into
mainfrom
xml-numeric-entities-parsing

Conversation

@HassanAkbar

@HassanAkbar HassanAkbar commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This PR is for addressing lutaml/lutaml-model#641

Status matrix

main:

Method nokogiri ox oga rexml libxml
SAX parse — attr ❌ ✅ ❌ ❌ ❌
SAX parse — text ✅ ✅ ❌ ✅ ✅
DOM parse — attr ✅ ✅ ❌ ✅ ✅
DOM parse — text ✅ ✅ ❌ ✅ ✅
Serialize ✅ ✅ ❌ ✅ ✅

current:

Method nokogiri ox oga rexml libxml
SAX parse — attr ✅ ✅ ✅ ✅ ✅
SAX parse — text ✅ ✅ ✅ ✅ ✅
DOM parse — attr ✅ ✅ ✅ ✅ ✅
DOM parse — text ✅ ✅ ✅ ✅ ✅
Serialize ✅ ✅ ✅ ✅ ✅

Actual outputs (per adapter)

nokogiri

  SAX parse - attr
- main:   Ampersand: & Hex: & Copyright: ©
+ branch: Ampersand: & Hex: & Copyright: ©

  SAX parse - text     (unchanged)  Ampersand: & Hex: & Copyright: ©
  DOM parse - attr     (unchanged)  Ampersand: & Hex: & Copyright: ©
  DOM parse - text     (unchanged)  Ampersand: & Hex: & Copyright: ©
  Serialize            (unchanged)  <doc details="Ampersand: &amp;#38; Hex: &amp;#x26; Copyright: &amp;#169;"></doc>

ox

  All paths already correct on main; branch is identical.
  Parse:     Ampersand: &#38; Hex: &#x26; Copyright: &#169;
  Serialize: <doc details="Ampersand: &amp;#38; Hex: &amp;#x26; Copyright: &amp;#169;"></doc>

oga

  SAX parse - attr
- main:   Ampersand: & Hex: & Copyright: ©
+ branch: Ampersand: &#38; Hex: &#x26; Copyright: &#169;

  SAX parse - text
- main:   Ampersand: & Hex: & Copyright: ©
+ branch: Ampersand: &#38; Hex: &#x26; Copyright: &#169;

  DOM parse - attr
- main:   Ampersand: & Hex: & Copyright: ©
+ branch: Ampersand: &#38; Hex: &#x26; Copyright: &#169;

  DOM parse - text
- main:   Ampersand: & Hex: & Copyright: ©
+ branch: Ampersand: &#38; Hex: &#x26; Copyright: &#169;

  Serialize
- main:   <doc details="Ampersand: &amp; Hex: &amp; Copyright: ©"></doc>
+ branch: <doc details="Ampersand: &amp;#38; Hex: &amp;#x26; Copyright: &amp;#169;"></doc>

rexml

  SAX parse - attr
- main:   Ampersand: &amp;#38; Hex: &amp;#x26; Copyright: &amp;#169;
+ branch: Ampersand: &#38; Hex: &#x26; Copyright: &#169;

  SAX parse - text     (unchanged)  Ampersand: &#38; Hex: &#x26; Copyright: &#169;
  DOM parse - attr     (unchanged)  Ampersand: &#38; Hex: &#x26; Copyright: &#169;
  DOM parse - text     (unchanged)  Ampersand: &#38; Hex: &#x26; Copyright: &#169;
  Serialize            (unchanged)  <doc details="Ampersand: &amp;#38; Hex: &amp;#x26; Copyright: &amp;#169;"></doc>

libxml

  SAX parse - attr
- main:   Ampersand: &#38;#38; Hex: &#38;#x26; Copyright: &#38;#169;
+ branch: Ampersand: &#38; Hex: &#x26; Copyright: &#169;

  SAX parse - text     (unchanged)  Ampersand: &#38; Hex: &#x26; Copyright: &#169;
  DOM parse - attr     (unchanged)  Ampersand: &#38; Hex: &#x26; Copyright: &#169;
  DOM parse - text     (unchanged)  Ampersand: &#38; Hex: &#x26; Copyright: &#169;
  Serialize            (unchanged)  <doc details="Ampersand: &amp;#38; Hex: &amp;#x26; Copyright: &amp;#169;"></doc>

github-advanced-security[bot]
github-advanced-security AI found potential problems May 21, 2026
View reviewed changes
Comment thread lib/moxml/adapter/libxml.rb Fixed
@HassanAkbar
HassanAkbar force-pushed the xml-numeric-entities-parsing branch from ac7d147 to 1a14522 Compare May 25, 2026 12:12
@HassanAkbar
HassanAkbar force-pushed the xml-numeric-entities-parsing branch 4 times, most recently from f80e6e7 to 6c68008 Compare June 5, 2026 14:41
@HassanAkbar
HassanAkbar marked this pull request as ready for review June 5, 2026 14:42
@HassanAkbar
HassanAkbar requested a review from ronaldtse June 5, 2026 14:42
HassanAkbar and others added 6 commits June 25, 2026 19:37
@HassanAkbar @ronaldtse
make numeric entities parsing consistent on all adapters
abb710d
@HassanAkbar @ronaldtse
fix code issues
d99fc9f
@HassanAkbar @ronaldtse
fix libxml doctype match issue
954a413
@HassanAkbar @ronaldtse
code refactor and removal
de54e9c
@HassanAkbar @ronaldtse
fix linter issues
d40f839
@ronaldtse
refactor: address initial audit findings on PR #83
32ff836 
- nokogiri.rb, libxml.rb: replace misleading bareword
  Nokogiri.decode_entities / Libxml.decode_entities with
  Moxml::Adapter::Base.decode_entities. The bareword relied on Ruby
  constant resolution to find the adapter class (which inherits the
  class method from Base), not the gem module, but reads as a gem
  call. The explicit receiver removes ambiguity.

- base.rb: freeze ENTITY_DECODE_RE and mark it private_constant,
  matching the treatment of NAMED_ENTITY_DECODE_MAP.

- customized_libxml/node.rb: narrow attr_accessor :native to
  attr_reader :native plus a private native= writer. Only
  replace_native_verbatim sets this; making it publicly mutable lets
  any caller swap the wrapper underlying node from anywhere.
@ronaldtse
ronaldtse force-pushed the xml-numeric-entities-parsing branch from 6c68008 to 32ff836 Compare June 25, 2026 11:38
ronaldtse added 2 commits June 25, 2026 19:40
@ronaldtse
refactor: expose replace_native! instead of public native= on libxml …
ae38278 
…wrapper

The previous audit fix narrowed attr_accessor :native to a private
writer. That broke replace_native_verbatim in the libxml adapter
because the call site lives in a different class. Replace the private
generic setter with a public semantic method replace_native! that
expresses the actual operation: swap the underlying native node after
libxml-ruby content= would have silently re-escaped stored text.
@ronaldtse
refactor: eliminate ivar-hacking from Oga adapter via prepend overrides
da66668 
Oga 3.4's stock entity decoder runs multiple passes, turning well-formed
`&amp;#38;` into `&` rather than the spec-correct `&#38;` (XML 1.0 §4.6
forbids recursive resolution of parsed entities). The previous fix walked
the parsed tree, read raw @text/@value ivars, decoded once, and wrote the
result back via instance_variable_set — the exact pattern forbidden by
the project's global rules.

Two CustomizedOga override modules now handle both paths without any
ivar access on foreign objects:

- EntityDecoderOverride prepends Oga::EntityDecoder.singleton_class and
  routes XML reads through Base.decode_entities for a single decode
  pass (HTML still falls through to Oga's stock decoder, which has
  HTML-specific legacy rules).

- RawValueOverride prepends Oga::XML::Attribute and Oga::XML::Text and
  consults the NativeAttachment sidecar first. User-authored values
  are stored as :raw_value / :raw_text sidecar entries and returned
  verbatim, bypassing the lazy decoder; parsed values fall through to
  Oga's normal lazy read, which the EntityDecoderOverride makes
  single-pass.

With both overrides in place, the post-parse `decode_entities_in_tree!`
walk and its `mark_decoded` / `write_decoded` ivar-writing helpers are
redundant and are removed. All entity state for user-authored nodes
lives in NativeAttachment; parsed nodes get correct single-pass lazy
decoding transparently.

All 2150 Oga-related specs pass (sax_entity_parity, integration,
oga_spec, entity_restoration).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ronaldtse ronaldtse Awaiting requested review from ronaldtse
1 more reviewer
@github-advanced-security github-advanced-security[bot] github-advanced-security[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@HassanAkbar @github-advanced-security @ronaldtse