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

Fix error on mention link with Nokogiri v1.11.0+ #26

Conversation

pocke
Copy link
Contributor

@pocke pocke commented Jan 7, 2021

This pull request fixes an error on a mention link with Nokogiri v1.11.0+.

Problem

With Nokogiri v1.11.0, the MentionLInk filter raises an error.
We can confirm the error with the test.

$ bundle exec rake
Run options: --seed 25805

# Running:

..............EE...EE.E...........................

Finished in 0.315472s, 158.4926 runs/s, 120.4544 assertions/s.

  1) Error:
MentionLinkTest#test_mention_filder_for_include_escaped_span_class:
RuntimeError: Cannot replace a node with no parent
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node.rb:270:in `replace'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:57:in `block (2 levels) in call'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:239:in `block in each'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `upto'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:54:in `block in call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:39:in `block in process'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/test_helper.rb:44:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/html_filters/mention_link_test.rb:61:in `test_mention_filder_for_include_escaped_span_class'

  2) Error:
MentionLinkTest#test_mention_filter_for_multiple_valid_accounts:
RuntimeError: Cannot replace a node with no parent
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node.rb:270:in `replace'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:57:in `block (2 levels) in call'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:239:in `block in each'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `upto'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:54:in `block in call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:39:in `block in process'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/test_helper.rb:44:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/html_filters/mention_link_test.rb:35:in `test_mention_filter_for_multiple_valid_accounts'

  3) Error:
MentionLinkTest#test_mention_filter_for_multiple_valid_and_invalid_accounts:
RuntimeError: Cannot replace a node with no parent
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node.rb:270:in `replace'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:57:in `block (2 levels) in call'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:239:in `block in each'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `upto'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:54:in `block in call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:39:in `block in process'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/test_helper.rb:44:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/html_filters/mention_link_test.rb:40:in `test_mention_filter_for_multiple_valid_and_invalid_accounts'

  4) Error:
MentionLinkTest#test_mention_filter_for_valid:
RuntimeError: Cannot replace a node with no parent
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node.rb:270:in `replace'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:57:in `block (2 levels) in call'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:239:in `block in each'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `upto'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:54:in `block in call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:39:in `block in process'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/test_helper.rb:44:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/html_filters/mention_link_test.rb:31:in `test_mention_filter_for_valid'

  5) Error:
MentionLinkTest#test_mention_filder_for_include_escaped_span_class_with_mention_candidate_class:
RuntimeError: Cannot replace a node with no parent
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node.rb:270:in `replace'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:57:in `block (2 levels) in call'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:239:in `block in each'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `upto'
    /home/pocke/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/nokogiri-1.11.1-x86_64-linux/lib/nokogiri/xml/node_set.rb:238:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:54:in `block in call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/html_filters/mention_link.rb:53:in `call'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:39:in `block in process'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `each'
    /home/pocke/ghq/github.com/bitjourney/mato/lib/mato/processor.rb:37:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/test_helper.rb:44:in `process'
    /home/pocke/ghq/github.com/bitjourney/mato/test/html_filters/mention_link_test.rb:66:in `test_mention_filder_for_include_escaped_span_class_with_mention_candidate_class'

50 runs, 38 assertions, 0 failures, 5 errors, 0 skips

Cause

We can find the cause from the release note of nokogiri v1.11.0.
https://github.com/sparklemotion/nokogiri/releases/tag/v1.11.0

The Node methods add_previous_sibling, previous=, before, add_next_sibling, next=, after, replace, and swap now correctly use their parent as the context node for parsing markup. These methods now also raise a RuntimeError if they are called on a node with no parent. [nokogumbo#160]

In short, Node#replace raises an error if a node doesn't have a parent.

A node's parent will be nil if Node#replace is already called.
For example:

require 'nokogiri'

doc = Nokogiri::HTML('foo')
text_node = doc.xpath('.//text()').first
p text_node.parent # => #<Nokogiri::XML::Element:0x50 name="p" children=[#<Nokogiri::XML::Text:0x3c "foo">]>
text_node.replace('bar')
p text_node.parent # => nil

In our case, the link_builder calls Node#replace if it finds correct mentions.
For example:

node.replace("<a href='https://twitter.com/#{mention}' class='mention'>#{mention}</a>")

So we need to skip Node#replace to cleanup for correct mentions, but it doesn't.

Solution

Skip Node#replace to cleanup for correct mentions.

@pocke pocke requested a review from a team January 7, 2021 08:55
@pocke
Copy link
Contributor Author

pocke commented Jan 8, 2021

@bitjourney/engineers ping

I'd like to merge this pull request early because nokogiri v1.10.10 has a security issue

Copy link

@ywindish ywindish left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pocke
Copy link
Contributor Author

pocke commented Jan 8, 2021

Thanks for your reviewing!

@pocke pocke merged commit 7a3cb59 into bitjourney:master Jan 8, 2021
@pocke pocke deleted the Fix_error_on_mention_link_with_Nokogiri_v1_11_0_ branch January 8, 2021 07:00
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.

2 participants