RuntimeError: Could not reparent node

[YavorIvanov]

Hi,

I stumbled upon an interesting problem. I think this shouldn't really happen.

source = open("http://www.dnevnik.bg/izbori2013/2013/05/20/2064070_cvetan_cvetanov_predvijda_novi_predsrochni_izbori_sled/").read
Readability::Document.new(source, :tags => %w[p img a strong em], :attributes => %w[src href], :remove_empty_nodes => true).content
RuntimeError: Could not reparent node

Running with DIV tags produces the expected result though.

Readability::Document.new(source, :tags => %w[div p img a strong em], :attributes => %w[src href], :remove_empty_nodes => true).content

In case the source of the page changes I'm giving preview:

puts Readability::Document.new(source).content
<div><div>

 Заместник-председателят на ГЕРБ Цветан Цветанов изрази увереност, че след няколко месеца ще има нови предсрочни парламентарни избори. В сутрешния блок на БНТ бившият вицепремиер и вътрешен министър заяви, че е поискал изрично от лидера на партията Бойко Борисов да не бъде включван в проектокабинет, докато не бъде изчистено името му.<p>Цветанов подчерта, че оттук нататък ще има много спекулации за това, че депутати от ГЕРБ ще подкрепят правителство на Пламен Орешарски. "Към настоящия момент всички избрани народни представители от ГЕРБ ще бъдат единни", допълни обаче той.</p>
<p>"Важно е да видим тази тройна коалиция, която ще бъде прикрита пред едно програмно правителство. Много е важно за българските граждани да чуят лъжите, които се казаха по време на предизборната кампания от трите други партии в новия парламент – БСП, ДПС и "Атака", каза още зам.-лидерът на ГЕРБ, като предрече, че след няколко месеца ще има нови предсрочни избори.</p>
<p>По думите му БСП, ДПС и "Атака" имат много различия, като например БСП подкрепя изграждането на АЕЦ "Белене", а според ДПС това не може да се случи. ДПС пък настоява за плосък данък, "Атака" иска сега и веднага 1000 лева заплата, а 500 лева пенсия.</p>
<p>От друга страна, ГЕРБ е показала какво е свършила през своя мандат и постави основните акценти на партията като настояване за плосък данък от 10%, бързо възстановяване на ДДС, законодателни промени в ДКЕВР.</p>
<p>Цветан Цветанов заяви, че е напълно закономерно ГЕРБ да предлага и касиране на изборите и да подготвя кабинет. По думите му конституирането на 42-ото народно събрание ще е факт, докато се произнесе Конституционният съд. Партията вече е решила да предложи Цецка Цачева отново за председател на парламента, защото "тя се е доказала като знаеща и можеща".</p>
<p>Когато сме получили подкрепата на над един милион български граждани, най-естественото е да предложим правителство на обществото, защото е важно в период на финансова и политическа криза да поемеш отговорността, подчерта бившият вицепремиер.</p>
<p>Самият той няма да бъде включен в този проектокабинет, като самият той го е поискал категорично пред лидера Бойко Борисов. Цветанов подчерта, че не иска на всяка цена да е на власт. Той отново заяви, че е невинен и сега трябва да изчисти името си от обвиненията и спекулациите, изречени в условията на предизборна кампания</p>
<p>За бившия си колега Симеон Дянков, към когото бяха отправени сериозни критики за финансовото и икономическо състояние на държавата и социалното недоволство на хората преди кабинетът "Борисов" да подаде оставка, Цветанов каза, че никога не е имало остра реакция. "Критиката беше, че на моменти не е имало гъвкавост", обясни той. Отново за бюлетините в Костинброд</p>
<p>Това са толкова несериозни неща, че не знам как е възможно да ги коментираме, каза Цветанов на въпрос за бюлетините в печатницата "Мултипринт" в Костинброд, които бяха открити в деня за размисъл. "Как можеш да излезеш да твърдиш колко са бюлетините, като никой не ги е броил, а и никой не е доказал, че те са годни за експедиция", подчерта той. По думите му се вижда от снимките на прокуратурата, че, например, има бюлетини за външното министерство, а тези бюлетини не могат да стигнат за толкова кратко време до секциите в чужбина.</p>
<p>Зам.-председателят на ГЕРБ каза, че към настоящия момент не може да вини службите за изтеклата информация, тъй като кабинетът "Борисов" и предишният парламент са въвели контрол за специалните разузнавателни средства и службите, така че да няма течове. Все пак той заяви, че не обвинява и прокуратурата за това. "Вярвам в главния прокурор за проверката в Костинброд", каза Цветанов и призова да се провери кой е подал сигнала до ТВ7, така че медията да знае за извършваната акция.</p>
<p>"И трябва да ви кажа, че когато бъдат оповестени тези данни за тази проверка, защото аз съм убеден, че българската прокуратура в лицето на главния прокурор ще направи тази проверка, ако не е направена вече, за да се оповести кой е източникът, който подаде тази информация към тази медия. Защото всички знаем тази медия с кого се свързва, как се свързва и какво изпълнява в цялата тази предизборна кампания", заяви бившият вицепремиер.</p>
<p>Не се е стигало до такова брутално нарушаване на свещеното право на всеки гражданин да има спокойствие в предизборния ден, заяви Цветан Цветанов и допълни, че изтичането на информация за акцията в печатницата в Костинброд е най-голямото престъпление в предизборната кампания. Според него за следващите избори могат да бъдат заложени наказателни промени за много тежки наказания срещу всяка медия, която ги наруши, и да се стига дори до отнемане на лицензи.</p>
<p>Трябва да бъдем истинска правова държава – ако ние самите не направим така, че всичко да бъде законно, това означава, че денят за размисъл и занапред ще бъде много агресивен, подчерта бившият министър на вътрешните работи. Никой не може да твърди какви доказателства ще бъдат събрани само няколко часа след започването на акцията, допълни той за акцията в Костинброд. </p>

 </div></div>
 => nil 

Specs: ruby-readability (0.5.7) rails (4.0.0.rc1) ruby 2.0.0p0 (2013-02-24 revision 39474) [x86_64-darwin12.3.0]

[YavorIvanov]

First clue I have around this is that it's probably a Nokogiri issue.

The error is being yield when you do

obj.content

[cantino]

It does seem like a Nokogiri issue. Do you have any idea how to fix it?

[YavorIvanov]

Not really. Still working on it.

My guess is that nokogiri parsing should be done with the fix option on. Still looking at the code but I probably won't have strength to finish this tonight.

[YavorIvanov]

From what I toyed around with it seems that readability should be using the DocumentFragment.parse of Nokogiri

Nokogiri::HTML::DocumentFragment.parse

This seem to be better suited for working with fragments such as the example where the content has no parent tag. Still not able to figure out the readability internals though to make a meaningful change.

[cantino]

Interesting. Would you mind submitting a pull request?

On Thu, May 23, 2013 at 12:47 AM, Yavor Ivanov notifications@github.comwrote:

From what I toyed around it seems that readability should be using the DocumentFragment.parse of Nokogiri

Nokogiri::HTML::DocumentFragment.parse

This seem to be better suited for working with fragments such as the example where the content has no parent tag. Still not able to figure out the readability internals though to make a meaningful change.

— Reply to this email directly or view it on GitHubhttps://github.com/cantino/ruby-readability/issues/50#issuecomment-18328261 .

[YavorIvanov]

Sure. Let me figure things out and will make a pull request.

[YavorIvanov]

Hi, not sure how to fix the code. I've narrowed it down to these lines of code but can't change it so the tests pass. I'm probably missing something.

      ([node] + node.css("*")).each do |el|
        # If element is in whitelist, delete all its attributes
        if whitelist[el.node_name]
          el.attributes.each { |a, x| el.delete(a) unless @options[:attributes] && @options[:attributes].include?(a.to_s) }

          # Otherwise, replace the element with its contents
        else
          if replace_with_whitespace[el.node_name]
            el.swap(Nokogiri::XML::Text.new(' ' << el.text << ' ', el.document))
          else
            el.swap(Nokogiri::XML::Text.new(el.text, el.document))
          end
        end

      end

[cantino]

Do you know which line from that code is failing with your error case?

It may be that you should wrap your input in a <div></div>, but I'm not sure. If you send a pull request with a failing spec, I'll take a crack at it.

[magic003]

I meet this issue too. Actually it can be easily reproduced with the following code:

require 'readability'

page = '<div>test</div>'
doc = Readability::Document.new(page, :tags => [])
puts doc.content

The problem is in the else-clause of the code cited by @YavorIvanov, and I don't think it is a Nokogiri issue:

          # Otherwise, replace the element with its contents
        else
          if replace_with_whitespace[el.node_name]
            el.swap(Nokogiri::XML::Text.new(' ' << el.text << ' ', el.document))
          else
            el.swap(Nokogiri::XML::Text.new(el.text, el.document))
          end
        end

The problem is el might be a root node, which doesn't have a parent. So the swap() throws a "Could not reparent node" error. I add some logic:

            if el.parent.nil?
                node = Nokogiri::XML::Text.new(el.text, el.document)
                break
            else
              if replace_with_whitespace[el.node_name]
                el.swap(Nokogiri::XML::Text.new(' ' << el.text << ' ', el.document))
              else
                el.swap(Nokogiri::XML::Text.new(el.text, el.document))
              end
            end

This could fix the issue. Do you have any concern for this change?

[cantino]

Thanks for working on this @magic003! I think that's a reasonable solution. Can you send a pull request with a spec?

[magic003]

Pull request has been sent.

[cantino]

Thanks to @magic003 for fixing this!