8000 Fix duplicate responses in XPath following, following-sibling, preceding, preceding-sibling by naitoh · Pull Request #255 · ruby/rexml · GitHub
[go: up one dir, main page]

Skip to content

Fix duplicate responses in XPath following, following-sibling, preceding, preceding-sibling #255

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

Merged

Conversation

naitoh
Copy link
Contributor
@naitoh naitoh commented May 4, 2025

Why?

See: #251 (comment)

Expected values

  • XPath : a/d/preceding::* => ["d", "c", "b"]
<a>
  <b/> <!-- a/d/preceding::b -->
  <c/> <!-- a/d/preceding::c -->
  <d/> <!-- a/d/preceding::d -->
  <d/> <!-- self -->
  <e/>
  <f/>
</a>
  • XPath : a/d/following::* => ["d", "e", "f"]
<a>
  <b/>
  <c/>
  <d/> <!-- self -->
  <d/> <!-- a/d/following::d -->
  <e/> <!-- a/d/following::e -->
  <f/> <!-- a/d/following::f -->
</a>
  • XPath : a/b/x/following-sibling:* => ["c", "d", "e"]
<a>
  <b>
    <x/> <!-- self -->
    <c/> <!-- a/b/x/following-sibling::c -->
    <d/> <!-- a/b/x/following-sibling::d -->
  </b>
  <b>
    <x/> <!-- self -->
    <e/> <!-- a/b/x/following-sibling::e -->
  </b>
</a>
  • XPath : a/b/x/following-sibling:* => ["c", "d", "x", "e"]
<a>
  <b>
    <x/> <!-- self -->
    <c/> <!-- a/b/x/following-sibling::c -->
    <d/> <!-- a/b/x/following-sibling::d -->
    <x/> <!-- a/b/x/following-sibling::x -->
    <e/> <!-- a/b/x/following-sibling::e -->
  </b>
</a>
  • XPath : a/b/x/preceding-sibling::* => ["e", "d", "c"]
<a>
  <b>
    <c/>  <!-- a/b/x/preceding-sibling::c -->
    <d/>  <!-- a/b/x/preceding-sibling::d -->
    <x/>  <!-- self -->
  </b>
  <b>
    <e/>  <!-- a/b/x/preceding-sibling::e -->
    <x/>  <!-- self -->
  </b>
</a>
  • XPath : a/b/x/preceding-sibling::* => ["e", "x", "d", "c"]
<a>
  <b>
    <c/>  <!-- a/b/x/preceding-sibling::c -->
    <d/>  <!-- a/b/x/preceding-sibling::d -->
    <x/>  <!-- a/b/x/preceding-sibling::x -->
    <e/>  <!-- a/b/x/preceding-sibling::e -->
    <x/>  <!-- self -->
  </b>
</a>
  • XPath : //a/following-sibling:*[1] => ["w", "x", "y", "z"]
<div>
  <div>
    <a/> <-- self -->
    <w/> <-- //a/following-sibling:*[1] -->
  </div>
  <a/> <-- self -->
  <x/> <-- //a/following-sibling:*[1] -->
  <a/> <-- self -->
  <y/> <-- //a/following-sibling:*[1] -->
  <a/> <-- self -->
  <z/> <-- //a/following-sibling:*[1] -->
</div>

@naitoh naitoh marked this pull request as ready for review May 4, 2025 14:10
@naitoh naitoh requested a review from kou May 4, 2025 14:10
@tompng
Copy link
Member
tompng commented May 5, 2025

It has a breaking change.

d = REXML::Document.new('<div>
  <div>
    <a/><w/>
  </div>
  <a/><x/>
  <a/><y/>
  <a/><z/>
</div>')
# Finds a node flowing <a/>
REXML::XPath.match(d, '//a/following-sibling::*[1]')
# Before: [<w/>, <x/>, <y/>, <z/>]
# After: [<w/>, <x/>]

For reference, the behavior in master branch is same as JavaScript xpath.

document.body.innerHTML = `
<div>
  <div><a></a><w></w></div>
  <a></a><x></x>
  <a></a><y></y>
  <a></a><z></z>
</div>`
result = document.evaluate('//a/following-sibling::*[1]', document, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null)
for(i=0;i<result.snapshotLength;i++) console.log(i, result.snapshotItem(i))
// 0 <w></w>
// 1 <x></x>
// 2 <y></y>
// 3 <z></z>

@naitoh naitoh force-pushed the fix_xpath_preceding_following_multiple_case branch 2 times, most recently from 2fdf0db to 15b3a80 Compare May 5, 2025 12:34
@naitoh
Copy link
Contributor Author
naitoh commented May 5, 2025

It has a breaking change.

Thanks!

I have corrected the issue you mentioned by removing the duplicate.

@naitoh naitoh force-pushed the fix_xpath_preceding_following_multiple_case branch from 15b3a80 to 6e6dfda Compare May 6, 2025 00:33
@naitoh naitoh changed the title fix: XPath following, following-sibling, preceding, preceding-sibling for multiple node Fix duplicate responses in XPath following, following-sibling, preceding, preceding-sibling May 6, 2025
@naitoh naitoh force-pushed the fix_xpath_preceding_following_multiple_case branch from 6e6dfda to effd307 Compare May 6, 2025 00:49
@naitoh naitoh requested a review from kou May 6, 2025 00:59
@naitoh
Copy link
Contributor Author
naitoh commented May 6, 2025

I have corrected the points you pointed out.
What do you think?

@naitoh naitoh force-pushed the fix_xpath_preceding_following_multiple_case branch from effd307 to 0471daf Compare May 6, 2025 07:57
@naitoh naitoh requested a review from kou May 6, 2025 08:05
@naitoh
Copy link
Contributor Author
naitoh commented May 6, 2025

I have corrected the points you pointed out.
What do you think?

<b/><c/><d/><d/><e/><f/>
</a>
XML
d = REXML::Document.new( source )
Copy link
Member

Choose a reason for hiding this comment

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

  • Let's use XXX(YYY) style not XXX( YYY ) style for newly added codes
  • Let's use doc or document instead of d because we have <d/> in this context
Suggested change
d = REXML::Document.new( source )
doc = REXML::Document.new(source)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.
I see.

</a>
XML
d = REXML::Document.new( source )
matches = REXML::XPath.match(d, "a/d/preceding::*")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
matches = REXML::XPath.match(d, "a/d/preceding::*")
matches = REXML::XPath.match(doc, "a/d/preceding::*")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.
I see.

…ing, preceding-sibling

## Why?
See: ruby#251 (comment)

- XPath : a/d/preceding::* => ["d", "c", "b"]
```xml
<a>
  <b/> <!-- a/d/preceding::b -->
  <c/> <!-- a/d/preceding::c -->
  <d/> <!-- a/d/preceding::d -->
  <d/> <!-- self -->
  <e/>
  <f/>
</a>
```

- XPath : a/d/following::* => ["d", "e", "f"]
```xml
<a>
  <b/>
  <c/>
  <d/> <!-- self -->
  <d/> <!-- a/d/following::d -->
  <e/> <!-- a/d/following::e -->
  <f/> <!-- a/d/following::f -->
</a>
```

- XPath : a/b/x/following-sibling:* => ["c", "d", "e"]
```xml
<a>
  <b>
    <x/> <!-- self -->
    <c/> <!-- a/b/x/following-sibling::c -->
    <d/> <!-- a/b/x/following-sibling::d -->
 </b>
  <b>
    <x/> <!-- self -->
    <e/> <!-- a/b/x/following-sibling::e -->
  </b>
</a>
```

- XPath : a/b/x/following-sibling:* => ["c", "d", "x", "e"]
```xml
<a>
  <b>
    <x/> <!-- self -->
    <c/> <!-- a/b/x/following-sibling::c -->
    <d/> <!-- a/b/x/following-sibling::d -->
    <x/> <!-- a/b/x/following-sibling::x -->
    <e/> <!-- a/b/x/following-sibling::e -->
  </b>
</a>
```

- XPath : a/b/x/preceding-sibling::* => ["e", "d", "c"]
```xml
<a>
  <b>
    <c/>  <!-- a/b/x/preceding-sibling::c -->
    <d/>  <!-- a/b/x/preceding-sibling::d -->
    <x/>  <!-- self -->
  </b>
  <b>
    <e/>  <!-- a/b/x/preceding-sibling::e -->
    <x/>  <!-- self -->
  </b>
</a>
```

- XPath : a/b/x/preceding-sibling::* => ["e", "x", "d", "c"]
```xml
<a>
  <b>
    <c/>  <!-- a/b/x/preceding-sibling::c -->
    <d/>  <!-- a/b/x/preceding-sibling::d -->
    <x/>  <!-- a/b/x/preceding-sibling::x -->
    <e/>  <!-- a/b/x/preceding-sibling::e -->
    <x/>  <!-- self -->
  </b>
</a>
```

- XPath : //a/following-sibling:*[1] => ["w", "x", "y", "z"]
```xml
<div>
  <div>
    <a/> <-- self -->
    <w/> <-- //a/following-sibling:*[1] -->
  </div>
  <a/> <-- self -->
  <x/> <-- //a/following-sibling:*[1] -->
  <a/> <-- self -->
  <y/> <-- //a/following-sibling:*[1] -->
  <a/> <-- self -->
  <z/> <-- //a/following-sibling:*[1] -->
</div>
```
@naitoh naitoh force-pushed the fix_xpath_preceding_following_multiple_case branch from 0471daf to 054d207 Compare May 6, 2025 12:14
@naitoh naitoh requested a review from kou May 6, 2025 12:25
@naitoh
Copy link
Contributor Author
naitoh commented May 6, 2025

I have corrected the points you pointed out.
What do you think?

@kou kou merged commit 249d770 into ruby:master May 6, 2025
66 of 67 checks passed
@kou
Copy link
Member
kou commented May 6, 2025

Thanks.

@naitoh naitoh deleted the fix_xpath_preceding_following_multiple_case branch May 11, 2025 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0