Skip to content
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

Test for handler functions not interfering with CSS id queries #1890

Closed
wants to merge 1 commit into from

Conversation

twalpole
Copy link
Contributor

@twalpole twalpole commented Apr 12, 2019

This PR adds a test for an issue that shows up when using JRuby. When using JRuby css id queries (at least - maybe others too) where the id partially contains the name of a function in the passed in handler causes the CSS to return no elements. This does not happen on MRI. Not really sure where to even start looking for the root cause.

@codeclimate
Copy link

codeclimate bot commented Apr 12, 2019

Code Climate has analyzed commit 4cbe7b5 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 93.5% (0.0% change).

View more on Code Climate.

@twalpole
Copy link
Contributor Author

twalpole commented Apr 12, 2019

I think the issue is at https://github.com/sparklemotion/nokogiri/blob/master/ext/java/nokogiri/XmlXpathContext.java#L123 where it appears to indiscriminately replace any matching text in the query whether it's something that could be a function call or not.

@flavorjones
Copy link
Member

Thanks for submitting this. Tagging @jvshahid so he's aware and maybe we can pair on it.

@flavorjones
Copy link
Member

This issue goes back to 468c757 in 2010 when XPath custom handler support was first added. It's because a very rough method is being use to try to infer method calls. Will stare at it a bit today.

@flavorjones
Copy link
Member

See #2147 for a suggested future path around this functionality.

@flavorjones flavorjones added this to the v1.11.0 milestone Dec 26, 2020
flavorjones added a commit that referenced this pull request Dec 26, 2020
flavorjones added a commit that referenced this pull request Dec 26, 2020
flavorjones added a commit that referenced this pull request Dec 27, 2020
…space-inference

fix(jruby): make less-naive inference of xpath custom function ns

---

**What problem is this PR intended to solve?**

Closes #1890
Related to #2147

JRuby's logic for inferring a namespace for the custom XPath function handler has been naive since it was introduced in 468c757. This PR makes it less naive and less likely to break things by looking for function calls in the query via regex, and looking for matches among the handler's declared Ruby methods.

Ideally we should require namespaces in both JRuby and CRuby implementations at some point to avoid having to do this kind of heavy lifting. See #2147 for a proposal.

**Have you included adequate test coverage?**

Sure.


**Does this change affect the behavior of either the C or the Java implementations?**

Only changes the JRuby implementation, to do a better job of matching the CRuby behavior.
@flavorjones
Copy link
Member

Will be fixed in v1.11.0.rc4, sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants