FIX: Race-condition

FIX: Race-condition

There’s no ordering guarantee between webhooks, so we could potentially receive the commit comments for a commit before the push hook. These comments would then never be pulled into discourse.

diff --git a/app/controllers/discourse_code_review/code_review_controller.rb b/app/controllers/discourse_code_review/code_review_controller.rb
index 3f7fa81..2567632 100644
--- a/app/controllers/discourse_code_review/code_review_controller.rb
+++ b/app/controllers/discourse_code_review/code_review_controller.rb
@@ -40,9 +40,11 @@ module DiscourseCodeReview
         importer = Importer.new(repo)
 
         if type == "commit_comment"
-          importer.import_comments
+          commit_sha = params["comment"]["commit_id"]
+
+          importer.sync_commit_sha(commit_sha)
         elsif type == "push"
-          importer.import_commits
+          importer.sync_merged_commits
         end
       end
 
diff --git a/lib/discourse_code_review/github_repo.rb b/lib/discourse_code_review/github_repo.rb
index 7a55a5d..af49683 100644
--- a/lib/discourse_code_review/github_repo.rb
+++ b/lib/discourse_code_review/github_repo.rb
@@ -18,15 +18,6 @@ module DiscourseCodeReview
       @octokit_client = octokit_client
     end
 
-    def current_comment_page
-      (PluginStore.get(DiscourseCodeReview::PluginName, CommentPage + @name) || 1).to_i
-    end
-
-    def current_comment_page=(v)
-      PluginStore.set(DiscourseCodeReview::PluginName, CommentPage + @name, v)
-      v
-    end
-
     def clean_name
       @name.gsub(/[^a-z0-9]/i, "_")
     end
@@ -56,18 +47,20 @@ module DiscourseCodeReview
       false
     end
 
-    def commit_comments(page = nil)
-      # TODO add a distributed lock here
-      git("pull")
+    def master_contains?(ref)
+      git('pull')
 
-      page ||= current_comment_page
+      hash = git('rev-parse', ref)
+      git('merge-base', 'origin/master', hash) == hash
+    end
 
-      octokit_client.list_commit_comments(@name, page: page).map do |hash|
+    def commit_comments(commit_sha)
+      git("pull")
 
+      octokit_client.commit_comments(@name, commit_sha).map do |hash|
         line_content = nil
 
         if hash[:path].present? && hash[:position].present?
-
           diff = git("diff", "#{hash[:commit_id]}~1", hash[:commit_id], hash[:path], raise_error: false)
           if diff.present?
             # 5 is preamble
@@ -92,7 +85,6 @@ module DiscourseCodeReview
           line_content: line_content
         }
       end
-
     end
 
     def commit(hash)
diff --git a/lib/discourse_code_review/importer.rb b/lib/discourse_code_review/importer.rb
index 097df69..9cca305 100644
--- a/lib/discourse_code_review/importer.rb
+++ b/lib/discourse_code_review/importer.rb
@@ -8,14 +8,15 @@ module DiscourseCodeReview
       @github_repo = github_repo
     end
 
-    def self.import_commit(sha)
+    def self.sync_commit(sha)
       client = DiscourseCodeReview.octokit_client
       GithubCategorySyncer.each_repo_name do |repo_name|
         repo = GithubRepo.new(repo_name, client)
         importer = Importer.new(repo)
 
+
         if commit = repo.commit(sha)
-          importer.import_commit(commit, update_last_commit: false)
+          importer.sync_commit(commit)
           return repo_name
         end
       end
@@ -30,9 +31,12 @@ module DiscourseCodeReview
         ).id
     end
 
-    def import_commits
+    def sync_merged_commits
+      last_commit = nil
       github_repo.commits_since.each do |commit|
-        import_commit(commit)
+        sync_commit(commit)
+
+        github_repo.last_commit = commit[:hash]
       end
     end
 
@@ -83,7 +87,20 @@ module DiscourseCodeReview
       result
     end
 
-    def import_commit(commit, update_last_commit: true)
+    def sync_commit_sha(commit_sha)
+      commit = github_repo.commit(commit_sha)
+      sync_commit(commit)
+    end
+
+    def sync_commit(commit)
+      topic_id = import_commit(commit)
+      import_comments(topic_id, commit[:hash])
+      topic_id
+    end
+
+    def import_commit(commit)
+      return unless github_repo.master_contains?(commit[:hash])
+
       link = <<~LINK
         [<small>GitHub</small>](https://github.com/#{github_repo.name}/commit/#{commit[:hash]})
       LINK
@@ -111,8 +128,17 @@ module DiscourseCodeReview
         github_id: commit[:author_id]
       )
 
-      if !TopicCustomField.exists?(name: DiscourseCodeReview::CommitHash, value: commit[:hash])
+      topic_id =
+        TopicCustomField
+          .where(
+            name: DiscourseCodeReview::CommitHash,
+            value: commit[:hash]
+          )
+          .limit(1)
+          .pluck(:topic_id)
+          .first
 
+      if topic_id.nil?
         post = PostCreator.create!(
           user,
           raw: raw,
@@ -129,10 +155,6 @@ module DiscourseCodeReview
           value: commit[:hash]
         )
 
-        if update_last_commit
-          github_repo.last_commit = commit[:hash]
-        end
-
         linked_topics.values.each do |topic|
           topic.add_moderator_post(
             user,
@@ -143,66 +165,47 @@ module DiscourseCodeReview
           )
         end
 
-        post
+        topic_id = post.topic_id
       end
-    end
-
-    def import_comments
-      page = github_repo.current_comment_page
-
-      while true
-        comments = github_repo.commit_comments(page)
 
-        break if comments.blank?
-
-        comments.each do |comment|
-          import_comment(comment)
-        end
-
-        github_repo.current_comment_page = page
-        page += 1
-      end
+      topic_id
     end
 
-    protected
+    def import_comments(topic_id, commit_sha)
+      github_repo.commit_comments(commit_sha).each do |comment|
+        # skip if we already have the comment
+        unless PostCustomField.exists?(name: DiscourseCodeReview::GithubId, value: comment[:id])
+          login = comment[:login] || "unknown"
+          user = DiscourseCodeReview.github_user_syncer.ensure_user(name: login, github_login: login)
 
-    def import_comment(comment)
+          context = ""
+          if comment[:line_content]
+            context = <<~MD
+              [quote]
+              #{comment[:path]}
 
-      # skip if we already have the comment
-      return if PostCustomField.exists?(name: DiscourseCodeReview::GithubId, value: comment[:id])
+              `‍``diff
+              #{comment[:line_content]}
+              `‍``
 
-      # do we have the commit?
-      if topic_id = TopicCustomField.where(name: DiscourseCodeReview::CommitHash, value: comment[:commit_hash]).pluck(:topic_id).first
-        login = comment[:login] || "unknown"
-        user = DiscourseCodeReview.github_user_syncer.ensure_user(name: login, github_login: login)
+              [/quote]
 
-        context = ""
-        if comment[:line_content]
-          context = <<~MD
-            [quote]
-            #{comment[:path]}
-
-            `‍``diff
-            #{comment[:line_content]}
-            `‍``
+            MD
+          end
 
-            [/quote]
+          custom_fields = { DiscourseCodeReview::GithubId => comment[:id] }
+          custom_fields[DiscourseCodeReview::CommentPath] = comment[:path] if comment[:path].present?
+          custom_fields[DiscourseCodeReview::CommentPosition] = comment[:position] if comment[:position].present?
 
-          MD
+          PostCreator.create!(
+            user,
+            raw: context + comment[:body],
+            skip_validations: true,
+            created_at: comment[:created_at],
+            topic_id: topic_id,
+            custom_fields: custom_fields
+          )
         end
-
-        custom_fields = { DiscourseCodeReview::GithubId => comment[:id] }
-        custom_fields[DiscourseCodeReview::CommentPath] = comment[:path] if comment[:path].present?
-        custom_fields[DiscourseCodeReview::CommentPosition] = comment[:position] if comment[:position].present?
-
-        PostCreator.create!(
-          user,
-          raw: context + comment[:body],
-          skip_validations: true,
-          created_at: comment[:created_at],
-          topic_id: topic_id,
-          custom_fields: custom_fields
-        )
       end
     end
   end

[... diff too long, it was truncated ...]

GitHub sha: b7ea6ed2

It’s not exactly the same (it retrieves post_number == 1) but you might want to consider Topic.find(topic_id).first_post

1 Like

I wrote this before I read the discussion about posts and ordered posts. Thanks for bringing it up!

2 Likes