diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index 4ed9708b40..6637acf472 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -13,6 +13,7 @@ def initialize(token) can :create, Note unless token can [:read, :download], Changeset + can :read, ChangesetComment can :read, Tracepoint can :read, User can :read, Node diff --git a/app/controllers/api/changeset_comments_controller.rb b/app/controllers/api/changeset_comments_controller.rb index c180571c58..808ac97ea3 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 @@ class ChangesetCommentsController < ApiController 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 9111bb609d..3df7b75cea 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 @@ def index 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 @@ def index 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 @@ def conditions_time(changesets, time) 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 @@ def conditions_ids(changesets, ids) 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 7e2e7fb793..df214210e8 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 @@ def index @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| @@ -231,8 +235,9 @@ def feed # 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| @@ -248,47 +253,19 @@ def search @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 @notes = @notes.joins(:comments).where("to_tsvector('english', note_comments.body) @@ plainto_tsquery('english', ?)", params[:q]) if params[:q] # 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" @@ -306,7 +283,8 @@ def search 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| @@ -323,20 +301,6 @@ def search # 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 fef48bb188..148873eea8 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 @@ def show 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 @@ def show 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 0000000000..eb06842833 --- /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 0000000000..73b1ec9080 --- /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 0000000000..951556fc20 --- /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 0000000000..0286b1a87f --- /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 0000000000..cfa59c9143 --- /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 f0e4613200..7001a95e85 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 08cfbbc79b..072f8fc5d6 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 aae32527a4..090ebd0b80 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -30,6 +30,8 @@ post "changeset/comment/:id/hide" => "changeset_comments#destroy", :as => :changeset_comment_hide, :id => /\d+/ post "changeset/comment/:id/unhide" => "changeset_comments#restore", :as => :changeset_comment_unhide, :id => /\d+/ + resources :changeset_comments, :only => [:index], :controller => "changeset_comments", :as => :api_changeset_comments + put "node/create" => "nodes#create" get "node/:id/ways" => "ways#ways_for_node", :as => :node_ways, :id => /\d+/ get "node/:id/relations" => "relations#relations_for_node", :as => :node_relations, :id => /\d+/ diff --git a/config/settings.yml b/config/settings.yml index db871775e7..5962aedf85 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -31,6 +31,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 21f30714c2..ce255daf2c 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -5,6 +5,14 @@ class ChangesetCommentsControllerTest < ActionDispatch::IntegrationTest ## # 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,48 @@ def test_routes ) end + ## + # view comments + 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 + ## # create comment success def test_create_comment_success @@ -320,5 +370,21 @@ def test_api_write_and_terms_agreed_via_token end assert_response :success end + + private + + ## + # check that certain comments exist in the output in the specified order + def assert_comments_in_order(comments) + assert_select "osm>comment", comments.size + comments.each_with_index do |comment, index| + assert_select "osm>comment:nth-child(#{index + 1})", 1 do + assert_select ">@id", comment.id.to_s + assert_select ">@uid", comment.author.id.to_s + assert_select ">@user", comment.author.display_name + assert_select ">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 5f69e6a2ac..33b1be5c23 100644 --- a/test/controllers/api/notes_controller_test.rb +++ b/test/controllers/api/notes_controller_test.rb @@ -1055,6 +1055,37 @@ def test_search_by_user_success 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