PostRevisor can't revise posts in deleted topics

Bug
1

tl;dr

For some posts, calling PostRevisor sets the post_id to nil. Am I crazy?

Answer: No. PostRevisor accesses post.topic, which is nil for a post in a deleted topic. And then it sets post.topic to nil, which in turn sets post.topic_id to nil.

I think PostRevisor should get the topic like this:

  @topic = topic || Topic.with_deleted.find_by(id: post_topic_id)

rather than like this:

  @topic = topic

post=Post.find_by(topic_id: 179227, post_number: 12)
post.topic_id => 179227
pr=PostRevisor.new(post)
post.topic_id => nil

The Full Story

I’m working on a script that fixes goo.gl links (the service is sunsetting soon, so the script finds goo.gl links, fetches what they are redirected to and gsubs the goo.gl URL with the one it’s directed to. It mostly works.

But

For a bunch of posts, everything looks like it’s going just fine, but then PostRevisor fails because post.acting_user is nil. And then, in my rescue, it seems that the topic_id is nil, but it’s not the post that’s nil, because it still has a post_number.

      begin
        puts "Revising (#{count}/#{total_posts}) https://mysite.com/t/#{post.topic_id}/#{post.post_number}"
        puts "missing topic_id for post #{post.id}" if !post.topic_id
        next if !post.topic_id
        PostRevisor.new(post).revise!(system_user, raw: new_raw, **revision_options)
      rescue => e
        puts "cannot revise (number: #{count} https://tw.forumosa.com/t/#{post.topic_id}/#{post.post_number}): #{e}"
      end

FIXING!!: https://goo.gl/maps/XaNG
B7qaZGzhBmM78 -----> https://www.google.com/maps/place/%E6%AD%A5%E9%81%93%E5%92%96%E5%95%A1%E9%A4%A8Cafe+Strada/@22.6300414,120.3153591,17z/d
ata=!3m1!4b1!4m5!3m4!1s0x346e04944a9b3471:0x520c1f01c3d62e57!8m2!3d22.6301115!4d120.3175543?shorturl=1
Revising (680/1773) https://mysite.com/t/207069/1817
cannot revise (number: 680 https://mysite.com/t//1817): undefined method `acting_user=' for nil

For the vast majority of posts, this works just fine, but for some subset of them, it fails like this. And if I run the code by hand line-by-line I get the same thing. It’s starting to look like, though, that if I do a pr=PostRevisor.new(post) I see that the post in the pr record has no topic_id and then if I inspect the post it now has topic_id set to nil.

1 Like
2

Out of interest, any reason why you aren’t just editing the Post model directly?

3

Because if you are changing 3000 posts with gsub and a complicated regex with a bunch of edge cases (fix: goo.gl, http://goo.gl, https://goo.gl, but don’t touch https://maps.app.goo.gl, or https://map.goo.gl, and maybe you get rate limited by goo.gl, and on and on) you might want to get back the post before you totally screwed it up!

It’s been very cool to be able to look at the edits and see the before and after and be able to revert! One version would make https://maps.app.goo.gl/abd12 into something like https://maps.app.https://maps.google.com/;lkajw3rpoazse;flknmase;faijserfasefklasdfa, for example.

1 Like
4

that makes sense :slight_smile:

1 Like
5

Some years ago there was some similar script that I tried to get CDCK to run for some client and they said (in my words, not theirs) “Dude. You think we’re going to run your code that just willy-nilly changes a zillion posts and there’s no way to undo it? Think again.”

1 Like
6

Yeah, other approach is to run it on a backup in a controlled area. But I love your solution.

1 Like
7

So here’s the initializer:

So somehow the post is getting there and though post_id has a value, post does not?

8

This I cannot reproduce.

The last post.topic_id is not nil and as expected a repetition of the prior result

9

Yeah. It worked on most of the posts. There’s something funky with some of them.

[63] pry(main)> post.topic_id
=> 179227
[64] pry(main)> post.topic
=> nil
[65] pry(main)>

I’m pretty sure that’s not supposed to happen. :person_shrugging:

But wait:

 Topic.find(post.topic_id)
ActiveRecord::RecordNotFound: Couldn't find Topic with 'id'=179227 [WHERE "topics"."deleted_at" IS NULL]
from /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/activerecord-7.2.2.1/lib/active_record/relation/finder_methods.rb:428:in `raise_record_not_found_exception!'

The topic is deleted.

So then the question is whether it’s a bug that you can’t revise posts in deleted topics.

I guess I’m not going to care and have my script check post.topic for nil rather than post.topic_id

1 Like
10

Yeah some nasty lack of integrity there with the foreign key!

I think some of the guard-rails(ahem) are switched off because the tables are usually too large.

Maybe someone performed a topic.delete not destroy. Oops.

11

My search for the posts was like Post.where("raw like '%goo.gl%'"), and that will return posts in deleted topics. And then those posts have a topic_id, but not a topic. I think there's some way to get Topic.find` to return deleted topics (because an admin can see those topics, which is why I was so confused. That little trash can is easy to miss.)

So that’s like this:

deleted_topic = Topic.with_deleted.find_by(id: 123)

So maybe I should have done that and updated the post before calling the PostRevisor?

But in the UX I can update a post in a deleted topic, so I’m thinking that

def initialize(post, topic = post.topic)
  @post = post
  @topic = topic
  # Make sure we have only one Topic instance
  post.topic = topic
end

should be

def initialize(post, topic = post.topic)
  @post = post
  @topic = topic || Topic.with_deleted.find_by(id: post_topic_id)
  # Make sure we have only one Topic instance
  post.topic = topic
end

I’m going to move this to Bug in case someone else thinks this is one.

But this works:

        if !post.topic # posts in delted topics have no topic and break PostRevisor
           post.topic = Topic.with_deleted.find_by(id: post.topic_id)
           next if !post.topic
        end
        PostRevisor.new(post).revise!(system_user, raw: new_raw, **revision_options)

There were posts in 3 topics that caused rails to crash. I’m giving up on that. :slight_smile:

1 Like
12

And another version of the above that does a deleted_topic = Topic.with_deleted.find_by(id: 123) to update post.topic also works.

Still seems like a bug, though. Or maybe since core does something else to manage this, it’s not a bug.

1 Like
13

Or a missing Feature (and therefore spec)?

Online, this will never be called for Posts in deleted Topics?

I agree, though, that it should manage that properly :thinking:

14

Perhaps

They’re clearly doing something to allow it to work on posts in deleted topics, probably like I am.

Yeah. Someone who makes such decisions can move this back to Dev if they think that’s appropriate.