From: Tom Hughes <tom@compton.nu>
Date: Sun, 16 Feb 2025 09:04:17 +0000 (+0000)
Subject: Merge remote-tracking branch 'upstream/pull/4359'
X-Git-Tag: live~34
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/d90f353e5f30c35fdfc61a462fdb39b3504998ce?hp=ca6570513c1e6ec649e12cc3c06605b64687f091

Merge remote-tracking branch 'upstream/pull/4359'
---

diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb
index ec4de8f8e..acacec049 100644
--- a/app/abilities/api_ability.rb
+++ b/app/abilities/api_ability.rb
@@ -11,6 +11,7 @@ class ApiAbility
       can :create, Note unless user
 
       can [:read, :download], Changeset
+      can :read, ChangesetComment
       can :read, Tracepoint
       can :read, User
       can :read, [Node, Way, Relation, OldNode, OldWay, OldRelation]
diff --git a/app/controllers/api/changeset_comments_controller.rb b/app/controllers/api/changeset_comments_controller.rb
index c180571c5..808ac97ea 100644
--- a/app/controllers/api/changeset_comments_controller.rb
+++ b/app/controllers/api/changeset_comments_controller.rb
@@ -1,7 +1,9 @@
 module Api
   class ChangesetCommentsController < ApiController
-    before_action :check_api_writable
-    before_action :authorize
+    include QueryMethods
+
+    before_action :check_api_writable, :except => [:index]
+    before_action :authorize, :except => [:index]
 
     authorize_resource
 
@@ -9,6 +11,15 @@ module Api
 
     before_action :set_request_formats
 
+    ##
+    # show all comments or search for a subset
+    def index
+      @comments = ChangesetComment.includes(:author).where(:visible => true).order("created_at DESC")
+      @comments = query_conditions_time(@comments)
+      @comments = query_conditions_user(@comments, :author)
+      @comments = query_limit(@comments)
+    end
+
     ##
     # Add a comment to a changeset
     def create
diff --git a/app/controllers/api/changesets_controller.rb b/app/controllers/api/changesets_controller.rb
index 9111bb609..3df7b75ce 100644
--- a/app/controllers/api/changesets_controller.rb
+++ b/app/controllers/api/changesets_controller.rb
@@ -2,6 +2,8 @@
 
 module Api
   class ChangesetsController < ApiController
+    include QueryMethods
+
     before_action :check_api_writable, :only => [:create, :update, :upload, :subscribe, :unsubscribe]
     before_action :setup_user_auth, :only => [:show]
     before_action :authorize, :only => [:create, :update, :upload, :close, :subscribe, :unsubscribe]
@@ -30,7 +32,7 @@ module Api
       changesets = conditions_bbox(changesets, bbox)
       changesets = conditions_user(changesets, params["user"], params["display_name"])
       changesets = conditions_time(changesets, params["time"])
-      changesets = conditions_from_to(changesets, params["from"], params["to"])
+      changesets = query_conditions_time(changesets)
       changesets = conditions_open(changesets, params["open"])
       changesets = conditions_closed(changesets, params["closed"])
       changesets = conditions_ids(changesets, params["changesets"])
@@ -43,7 +45,7 @@ module Api
                    end
 
       # limit the result
-      changesets = changesets.limit(result_limit)
+      changesets = query_limit(changesets)
 
       # preload users, tags and comments, and render result
       @changesets = changesets.preload(:user, :changeset_tags, :comments)
@@ -337,33 +339,6 @@ module Api
       raise OSM::APIBadUserInput, e.message.to_s
     end
 
-    ##
-    # restrict changesets to those opened during a particular time period
-    # works similar to from..to of notes controller, including the requirement of 'from' when specifying 'to'
-    def conditions_from_to(changesets, from, to)
-      if from
-        begin
-          from = Time.parse(from).utc
-        rescue ArgumentError
-          raise OSM::APIBadUserInput, "Date #{from} is in a wrong format"
-        end
-
-        begin
-          to = if to
-                 Time.parse(to).utc
-               else
-                 Time.now.utc
-               end
-        rescue ArgumentError
-          raise OSM::APIBadUserInput, "Date #{to} is in a wrong format"
-        end
-
-        changesets.where(:created_at => from..to)
-      else
-        changesets
-      end
-    end
-
     ##
     # return changesets which are open (haven't been closed yet)
     # we do this by seeing if the 'closed at' time is in the future. Also if we've
@@ -403,19 +378,5 @@ module Api
         changesets.where(:id => ids)
       end
     end
-
-    ##
-    # Get the maximum number of results to return
-    def result_limit
-      if params[:limit]
-        if params[:limit].to_i.positive? && params[:limit].to_i <= Settings.max_changeset_query_limit
-          params[:limit].to_i
-        else
-          raise OSM::APIBadUserInput, "Changeset limit must be between 1 and #{Settings.max_changeset_query_limit}"
-        end
-      else
-        Settings.default_changeset_query_limit
-      end
-    end
   end
 end
diff --git a/app/controllers/api/notes_controller.rb b/app/controllers/api/notes_controller.rb
index a0095d954..af0c5e039 100644
--- a/app/controllers/api/notes_controller.rb
+++ b/app/controllers/api/notes_controller.rb
@@ -1,5 +1,7 @@
 module Api
   class NotesController < ApiController
+    include QueryMethods
+
     before_action :check_api_writable, :only => [:create, :comment, :close, :reopen, :destroy]
     before_action :setup_user_auth, :only => [:create, :show]
     before_action :authorize, :only => [:close, :reopen, :destroy, :comment]
@@ -36,7 +38,9 @@ module Api
       @max_lat = bbox.max_lat
 
       # Find the notes we want to return
-      @notes = notes.bbox(bbox).order("updated_at DESC").limit(result_limit).preload(:comments)
+      notes = notes.bbox(bbox).order("updated_at DESC")
+      notes = query_limit(notes)
+      @notes = notes.preload(:comments)
 
       # Render the result
       respond_to do |format|
@@ -234,8 +238,9 @@ module Api
 
       # Find the comments we want to return
       @comments = NoteComment.where(:note => notes)
-                             .order(:created_at => :desc).limit(result_limit)
-                             .preload(:author, :note => { :comments => :author })
+                             .order(:created_at => :desc)
+      @comments = query_limit(@comments)
+      @comments = @comments.preload(:author, :note => { :comments => :author })
 
       # Render the result
       respond_to do |format|
@@ -251,19 +256,8 @@ module Api
       @notes = bbox_condition(@notes)
 
       # Add any user filter
-      if params[:display_name] || params[:user]
-        if params[:display_name]
-          @user = User.find_by(:display_name => params[:display_name])
-
-          raise OSM::APIBadUserInput, "User #{params[:display_name]} not known" unless @user
-        else
-          @user = User.find_by(:id => params[:user])
-
-          raise OSM::APIBadUserInput, "User #{params[:user]} not known" unless @user
-        end
-
-        @notes = @notes.joins(:comments).where(:note_comments => { :author_id => @user })
-      end
+      user = query_conditions_user_value
+      @notes = @notes.joins(:comments).where(:note_comments => { :author_id => user }) if user
 
       # Add any text filter
       if params[:q]
@@ -271,29 +265,12 @@ module Api
       end
 
       # Add any date filter
-      if params[:from]
-        begin
-          from = Time.parse(params[:from]).utc
-        rescue ArgumentError
-          raise OSM::APIBadUserInput, "Date #{params[:from]} is in a wrong format"
-        end
-
-        begin
-          to = if params[:to]
-                 Time.parse(params[:to]).utc
-               else
-                 Time.now.utc
-               end
-        rescue ArgumentError
-          raise OSM::APIBadUserInput, "Date #{params[:to]} is in a wrong format"
-        end
-
-        @notes = if params[:sort] == "updated_at"
-                   @notes.where(:updated_at => from..to)
-                 else
-                   @notes.where(:created_at => from..to)
-                 end
-      end
+      time_filter_property = if params[:sort] == "updated_at"
+                               :updated_at
+                             else
+                               :created_at
+                             end
+      @notes = query_conditions_time(@notes, time_filter_property)
 
       # Choose the sort order
       @notes = if params[:sort] == "created_at"
@@ -311,7 +288,8 @@ module Api
                end
 
       # Find the notes we want to return
-      @notes = @notes.distinct.limit(result_limit).preload(:comments)
+      @notes = query_limit(@notes.distinct)
+      @notes = @notes.preload(:comments)
 
       # Render the result
       respond_to do |format|
@@ -328,20 +306,6 @@ module Api
     # utility functions below.
     #------------------------------------------------------------
 
-    ##
-    # Get the maximum number of results to return
-    def result_limit
-      if params[:limit]
-        if params[:limit].to_i.positive? && params[:limit].to_i <= Settings.max_note_query_limit
-          params[:limit].to_i
-        else
-          raise OSM::APIBadUserInput, "Note limit must be between 1 and #{Settings.max_note_query_limit}"
-        end
-      else
-        Settings.default_note_query_limit
-      end
-    end
-
     ##
     # Generate a condition to choose which notes we want based
     # on their status and the user's request parameters
diff --git a/app/controllers/changeset_comments/feeds_controller.rb b/app/controllers/changeset_comments/feeds_controller.rb
index fef48bb18..148873eea 100644
--- a/app/controllers/changeset_comments/feeds_controller.rb
+++ b/app/controllers/changeset_comments/feeds_controller.rb
@@ -1,5 +1,7 @@
 module ChangesetComments
   class FeedsController < ApplicationController
+    include QueryMethods
+
     before_action :authorize_web
     before_action :set_locale
 
@@ -19,10 +21,13 @@ module ChangesetComments
         changeset = Changeset.find(changeset_id)
 
         # Return comments for this changeset only
-        @comments = changeset.comments.includes(:author, :changeset).reverse_order.limit(comments_limit)
+        @comments = changeset.comments.includes(:author, :changeset).reverse_order
+        @comments = query_limit(@comments)
       else
         # Return comments
-        @comments = ChangesetComment.includes(:author, :changeset).where(:visible => true).order("created_at DESC").limit(comments_limit).preload(:changeset)
+        @comments = ChangesetComment.includes(:author, :changeset).where(:visible => true).order("created_at DESC")
+        @comments = query_limit(@comments)
+        @comments = @comments.preload(:changeset)
       end
 
       # Render the result
@@ -32,21 +37,5 @@ module ChangesetComments
     rescue OSM::APIBadUserInput
       head :bad_request
     end
-
-    private
-
-    ##
-    # Get the maximum number of comments to return
-    def comments_limit
-      if params[:limit]
-        if params[:limit].to_i.positive? && params[:limit].to_i <= 10000
-          params[:limit].to_i
-        else
-          raise OSM::APIBadUserInput, "Comments limit must be between 1 and 10000"
-        end
-      else
-        100
-      end
-    end
   end
 end
diff --git a/app/controllers/concerns/query_methods.rb b/app/controllers/concerns/query_methods.rb
new file mode 100644
index 000000000..eb0684283
--- /dev/null
+++ b/app/controllers/concerns/query_methods.rb
@@ -0,0 +1,92 @@
+module QueryMethods
+  extend ActiveSupport::Concern
+
+  private
+
+  ##
+  # Filter the resulting items by user
+  def query_conditions_user(items, filter_property)
+    user = query_conditions_user_value
+    items = items.where(filter_property => user) if user
+    items
+  end
+
+  ##
+  # Get user value for query filtering by user
+  # Raises OSM::APIBadUserInput if user not found like notes api does, changesets api raises OSM::APINotFoundError instead
+  def query_conditions_user_value
+    if params[:display_name] || params[:user]
+      if params[:display_name]
+        user = User.find_by(:display_name => params[:display_name])
+
+        raise OSM::APIBadUserInput, "User #{params[:display_name]} not known" unless user
+      else
+        user = User.find_by(:id => params[:user])
+
+        raise OSM::APIBadUserInput, "User #{params[:user]} not known" unless user
+      end
+
+      user
+    end
+  end
+
+  ##
+  # Restrict the resulting items to those created during a particular time period
+  # Using 'to' requires specifying 'from' as well for historical reasons
+  def query_conditions_time(items, filter_property = :created_at)
+    interval = query_conditions_time_value
+
+    if interval
+      items.where(filter_property => interval)
+    else
+      items
+    end
+  end
+
+  ##
+  # Get query time interval from request parameters or nil
+  def query_conditions_time_value
+    if params[:from]
+      begin
+        from = Time.parse(params[:from]).utc
+      rescue ArgumentError
+        raise OSM::APIBadUserInput, "Date #{params[:from]} is in a wrong format"
+      end
+
+      begin
+        to = if params[:to]
+               Time.parse(params[:to]).utc
+             else
+               Time.now.utc
+             end
+      rescue ArgumentError
+        raise OSM::APIBadUserInput, "Date #{params[:to]} is in a wrong format"
+      end
+
+      from..to
+    end
+  end
+
+  ##
+  # Limit the result according to request parameters and settings
+  def query_limit(items)
+    items.limit(query_limit_value)
+  end
+
+  ##
+  # Get query limit value from request parameters and settings
+  def query_limit_value
+    name = controller_path.sub(%r{^api/}, "").tr("/", "_").singularize
+    max_limit = Settings["max_#{name}_query_limit"]
+    default_limit = Settings["default_#{name}_query_limit"]
+    if params[:limit]
+      if params[:limit].to_i.positive? && params[:limit].to_i <= max_limit
+        params[:limit].to_i
+      else
+        raise OSM::APIBadUserInput, "#{controller_name.classify} limit must be between 1 and #{max_limit}"
+      end
+    else
+      default_limit
+    end
+  end
+end
diff --git a/app/views/api/changeset_comments/_changeset_comment.json.jbuilder b/app/views/api/changeset_comments/_changeset_comment.json.jbuilder
new file mode 100644
index 000000000..73b1ec908
--- /dev/null
+++ b/app/views/api/changeset_comments/_changeset_comment.json.jbuilder
@@ -0,0 +1,8 @@
+json.id changeset_comment.id
+json.visible changeset_comment.visible
+json.date changeset_comment.created_at.xmlschema
+if changeset_comment.author.data_public?
+  json.uid changeset_comment.author.id
+  json.user changeset_comment.author.display_name
+end
+json.text changeset_comment.body
diff --git a/app/views/api/changeset_comments/_changeset_comment.xml.builder b/app/views/api/changeset_comments/_changeset_comment.xml.builder
new file mode 100644
index 000000000..951556fc2
--- /dev/null
+++ b/app/views/api/changeset_comments/_changeset_comment.xml.builder
@@ -0,0 +1,12 @@
+cattrs = {
+  "id" => changeset_comment.id,
+  "date" => changeset_comment.created_at.xmlschema,
+  "visible" => changeset_comment.visible
+}
+if changeset_comment.author.data_public?
+  cattrs["uid"] = changeset_comment.author.id
+  cattrs["user"] = changeset_comment.author.display_name
+end
+xml.comment(cattrs) do |comment_xml_node|
+  comment_xml_node.text(changeset_comment.body)
+end
diff --git a/app/views/api/changeset_comments/index.json.jbuilder b/app/views/api/changeset_comments/index.json.jbuilder
new file mode 100644
index 000000000..0286b1a87
--- /dev/null
+++ b/app/views/api/changeset_comments/index.json.jbuilder
@@ -0,0 +1,5 @@
+json.partial! "api/root_attributes"
+
+json.comments(@comments) do |comment|
+  json.partial! comment
+end
diff --git a/app/views/api/changeset_comments/index.xml.builder b/app/views/api/changeset_comments/index.xml.builder
new file mode 100644
index 000000000..cfa59c914
--- /dev/null
+++ b/app/views/api/changeset_comments/index.xml.builder
@@ -0,0 +1,7 @@
+xml.instruct! :xml, :version => "1.0"
+
+xml.osm(OSM::API.new.xml_root_attributes) do |osm|
+  @comments.includes(:author).each do |comment|
+    osm << render(comment)
+  end
+end
diff --git a/app/views/api/changesets/_changeset.json.jbuilder b/app/views/api/changesets/_changeset.json.jbuilder
index f0e461320..7001a95e8 100644
--- a/app/views/api/changesets/_changeset.json.jbuilder
+++ b/app/views/api/changesets/_changeset.json.jbuilder
@@ -23,13 +23,6 @@ json.tags changeset.tags unless changeset.tags.empty?
 
 if @comments
   json.comments(@comments) do |comment|
-    json.id comment.id
-    json.visible comment.visible
-    json.date comment.created_at.xmlschema
-    if comment.author.data_public?
-      json.uid comment.author.id
-      json.user comment.author.display_name
-    end
-    json.text comment.body
+    json.partial! comment
   end
 end
diff --git a/app/views/api/changesets/_changeset.xml.builder b/app/views/api/changesets/_changeset.xml.builder
index 08cfbbc79..072f8fc5d 100644
--- a/app/views/api/changesets/_changeset.xml.builder
+++ b/app/views/api/changesets/_changeset.xml.builder
@@ -27,18 +27,7 @@ xml.changeset(attrs) do |changeset_xml_node|
   if @comments
     changeset_xml_node.discussion do |discussion_xml_node|
       @comments.each do |comment|
-        cattrs = {
-          "id" => comment.id,
-          "date" => comment.created_at.xmlschema,
-          "visible" => comment.visible
-        }
-        if comment.author.data_public?
-          cattrs["uid"] = comment.author.id
-          cattrs["user"] = comment.author.display_name
-        end
-        discussion_xml_node.comment(cattrs) do |comment_xml_node|
-          comment_xml_node.text(comment.body)
-        end
+        discussion_xml_node << render(comment)
       end
     end
   end
diff --git a/config/routes.rb b/config/routes.rb
index b3fc5af15..0ffd0a546 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -38,6 +38,8 @@ OpenStreetMap::Application.routes.draw do
   end
 
   namespace :api, :path => "api/0.6" do
+    resources :changeset_comments, :only => :index
+
     resources :nodes, :only => [:index, :create]
     resources :nodes, :path => "node", :id => /\d+/, :only => [:show, :update, :destroy] do
       scope :module => :nodes do
diff --git a/config/settings.yml b/config/settings.yml
index 2e0346f00..51f4444c4 100644
--- a/config/settings.yml
+++ b/config/settings.yml
@@ -35,6 +35,14 @@ tracepoints_per_page: 5000
 default_changeset_query_limit: 100
 # Maximum limit on the number of changesets returned by the changeset query api method
 max_changeset_query_limit: 100
+# Default limit on the number of changeset comments returned by the api
+default_changeset_comment_query_limit: 100
+# Maximum limit on the number of changesets comments returned by the api
+max_changeset_comment_query_limit: 10000
+# Default limit on the number of changeset comments in feeds
+default_changeset_comments_feed_query_limit: 100
+# Maximum limit on the number of changesets comments in feeds
+max_changeset_comments_feed_query_limit: 10000
 # Maximum number of nodes that will be returned by the api in a map request
 max_number_of_nodes: 50000
 # Maximum number of nodes that can be in a way (checked on save)
diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb
index e456a3ca4..ba4200d3f 100644
--- a/test/controllers/api/changeset_comments_controller_test.rb
+++ b/test/controllers/api/changeset_comments_controller_test.rb
@@ -5,6 +5,14 @@ module Api
     ##
     # test all routes which lead to this controller
     def test_routes
+      assert_routing(
+        { :path => "/api/0.6/changeset_comments", :method => :get },
+        { :controller => "api/changeset_comments", :action => "index" }
+      )
+      assert_routing(
+        { :path => "/api/0.6/changeset_comments.json", :method => :get },
+        { :controller => "api/changeset_comments", :action => "index", :format => "json" }
+      )
       assert_routing(
         { :path => "/api/0.6/changeset/1/comment", :method => :post },
         { :controller => "api/changeset_comments", :action => "create", :id => "1" }
@@ -31,6 +39,46 @@ module Api
       )
     end
 
+    def test_index
+      user1 = create(:user)
+      user2 = create(:user)
+      changeset1 = create(:changeset, :closed, :user => user2)
+      comment11 = create(:changeset_comment, :changeset => changeset1, :author => user1, :created_at => "2023-01-01", :body => "changeset 1 question")
+      comment12 = create(:changeset_comment, :changeset => changeset1, :author => user2, :created_at => "2023-02-01", :body => "changeset 1 answer")
+      changeset2 = create(:changeset, :closed, :user => user1)
+      comment21 = create(:changeset_comment, :changeset => changeset2, :author => user1, :created_at => "2023-03-01", :body => "changeset 2 note")
+      comment22 = create(:changeset_comment, :changeset => changeset2, :author => user1, :created_at => "2023-04-01", :body => "changeset 2 extra note")
+      comment23 = create(:changeset_comment, :changeset => changeset2, :author => user2, :created_at => "2023-05-01", :body => "changeset 2 review")
+
+      get api_changeset_comments_path
+      assert_response :success
+      assert_comments_in_order [comment23, comment22, comment21, comment12, comment11]
+
+      get api_changeset_comments_path(:limit => 3)
+      assert_response :success
+      assert_comments_in_order [comment23, comment22, comment21]
+
+      get api_changeset_comments_path(:from => "2023-03-15T00:00:00Z")
+      assert_response :success
+      assert_comments_in_order [comment23, comment22]
+
+      get api_changeset_comments_path(:from => "2023-01-15T00:00:00Z", :to => "2023-04-15T00:00:00Z")
+      assert_response :success
+      assert_comments_in_order [comment22, comment21, comment12]
+
+      get api_changeset_comments_path(:user => user1.id)
+      assert_response :success
+      assert_comments_in_order [comment22, comment21, comment11]
+
+      get api_changeset_comments_path(:from => "2023-03-15T00:00:00Z", :format => "json")
+      assert_response :success
+      js = ActiveSupport::JSON.decode(@response.body)
+      assert_not_nil js
+      assert_equal 2, js["comments"].count
+      assert_equal comment23.id, js["comments"][0]["id"]
+      assert_equal comment22.id, js["comments"][1]["id"]
+    end
+
     def test_create_by_unauthorized
       assert_no_difference "ChangesetComment.count" do
         post changeset_comment_path(create(:changeset, :closed), :text => "This is a comment")
@@ -422,5 +470,20 @@ module Api
       assert_response :success
       assert comment.reload.visible
     end
+
+    private
+
+    ##
+    # check that certain comments exist in the output in the specified order
+    def assert_comments_in_order(comments)
+      assert_dom "osm > comment", comments.size do |dom_comments|
+        comments.zip(dom_comments).each do |comment, dom_comment|
+          assert_dom dom_comment, "> @id", comment.id.to_s
+          assert_dom dom_comment, "> @uid", comment.author.id.to_s
+          assert_dom dom_comment, "> @user", comment.author.display_name
+          assert_dom dom_comment, "> text", comment.body
+        end
+      end
+    end
   end
 end
diff --git a/test/controllers/api/notes_controller_test.rb b/test/controllers/api/notes_controller_test.rb
index 17ceb1b9e..f1a0f766c 100644
--- a/test/controllers/api/notes_controller_test.rb
+++ b/test/controllers/api/notes_controller_test.rb
@@ -1066,6 +1066,37 @@ module Api
       assert_select "gpx", :count => 1 do
         assert_select "wpt", :count => 1
       end
+
+      user2 = create(:user)
+      get search_api_notes_path(:user => user2.id, :format => "xml")
+      assert_response :success
+      assert_equal "application/xml", @response.media_type
+      assert_select "osm", :count => 1 do
+        assert_select "note", :count => 0
+      end
+    end
+
+    def test_search_by_time_success
+      note1 = create(:note, :created_at => "2020-02-01T00:00:00Z", :updated_at => "2020-04-01T00:00:00Z")
+      note2 = create(:note, :created_at => "2020-03-01T00:00:00Z", :updated_at => "2020-05-01T00:00:00Z")
+
+      get search_api_notes_path(:from => "2020-02-15T00:00:00Z", :to => "2020-04-15T00:00:00Z", :format => "xml")
+      assert_response :success
+      assert_equal "application/xml", @response.media_type
+      assert_select "osm", :count => 1 do
+        assert_select "note", :count => 1 do
+          assert_select "id", note2.id.to_s
+        end
+      end
+
+      get search_api_notes_path(:from => "2020-02-15T00:00:00Z", :to => "2020-04-15T00:00:00Z", :sort => "updated_at", :format => "xml")
+      assert_response :success
+      assert_equal "application/xml", @response.media_type
+      assert_select "osm", :count => 1 do
+        assert_select "note", :count => 1 do
+          assert_select "id", note1.id.to_s
+        end
+      end
     end
 
     def test_search_by_bbox_success