From 6398bdc2cead052a726c52b9a13f3643a26ae31e Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 04:21:00 +0300
Subject: [PATCH 01/14] Assert that hashes are equal without iterating over
 items

---
 test/controllers/api/relations_controller_test.rb | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 0d0624518..3c26715d0 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -1159,12 +1159,7 @@ module Api
       a_tags = get_tags_as_hash(a)
       b_tags = get_tags_as_hash(b)
 
-      assert_equal a_tags.keys, b_tags.keys, "Tag keys should be identical."
-      a_tags.each do |k, v|
-        assert_equal v, b_tags[k],
-                     "Tags which were not altered should be the same. " \
-                     "#{a_tags.inspect} != #{b_tags.inspect}"
-      end
+      assert_equal a_tags, b_tags, "Tags should be identical."
     end
 
     ##

From a83dd7ecd00b556949d60990aff58038664c180e Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 04:29:14 +0300
Subject: [PATCH 02/14] Remove sorting before converting tags to hash

Sorting is no longer required because we're not comparing arrays of keys.
---
 test/controllers/api/relations_controller_test.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 3c26715d0..b6610df6e 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -1146,7 +1146,7 @@ module Api
     ##
     # returns a k->v hash of tags from an xml doc
     def get_tags_as_hash(a)
-      a.find("//osm/relation/tag").sort_by { |v| v["k"] }.each_with_object({}) do |v, h|
+      a.find("//osm/relation/tag").each_with_object({}) do |v, h|
         h[v["k"]] = v["v"]
       end
     end

From 83710cc9ebf181cfc572bfdbe64436c17187b30a Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 04:37:38 +0300
Subject: [PATCH 03/14] Shorten tags to hash conversion

---
 test/controllers/api/relations_controller_test.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index b6610df6e..899def60d 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -1146,8 +1146,8 @@ module Api
     ##
     # returns a k->v hash of tags from an xml doc
     def get_tags_as_hash(a)
-      a.find("//osm/relation/tag").each_with_object({}) do |v, h|
-        h[v["k"]] = v["v"]
+      a.find("//osm/relation/tag").to_h do |tag|
+        [tag["k"], tag["v"]]
       end
     end
 

From a27027a17f0137f311d2bf72ab11d9988ab5ace6 Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 05:22:48 +0300
Subject: [PATCH 04/14] Assert that relation member arrays are equal without
 iterating

---
 test/controllers/api/relations_controller_test.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 899def60d..983d336cb 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -1027,9 +1027,7 @@ module Api
         [m["ref"].to_i, m["type"].to_sym, m["role"]]
       end
 
-      doc_members.zip(new_members).each do |d, n|
-        assert_equal d, n, "members are not equal - ordering is wrong? (#{doc}, #{xml})"
-      end
+      assert_equal doc_members, new_members, "members are not equal - ordering is wrong? (#{doc}, #{xml})"
     end
 
     ##

From 4aa7327ef30eb468f69a3afc8b7d0c8dea91280d Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 05:53:37 +0300
Subject: [PATCH 05/14] Remove with_relation private method

---
 .../api/relations_controller_test.rb          | 96 +++++++++----------
 1 file changed, 47 insertions(+), 49 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 983d336cb..c808c57ae 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -387,23 +387,29 @@ module Api
 
       auth_header = bearer_authorization_header user
 
-      with_relation(relation.id) do |rel|
-        # alter one of the tags
-        tag = rel.find("//osm/relation/tag").first
-        tag["v"] = "some changed value"
-        update_changeset(rel, changeset.id)
+      get api_relation_path(relation)
+      assert_response :success
+      rel = xml_parse(@response.body)
 
-        # check that the downloaded tags are the same as the uploaded tags...
-        new_version = with_update(rel, auth_header) do |new_rel|
-          assert_tags_equal rel, new_rel
-        end
+      # alter one of the tags
+      tag = rel.find("//osm/relation/tag").first
+      tag["v"] = "some changed value"
+      update_changeset(rel, changeset.id)
 
-        # check the original one in the current_* table again
-        with_relation(relation.id) { |r| assert_tags_equal rel, r }
-
-        # now check the version in the history
-        with_relation(relation.id, new_version) { |r| assert_tags_equal rel, r }
+      # check that the downloaded tags are the same as the uploaded tags...
+      new_version = with_update(rel, auth_header) do |new_rel|
+        assert_tags_equal rel, new_rel
       end
+
+      # check the original one in the current_* table again
+      get api_relation_path(relation)
+      assert_response :success
+      assert_tags_equal rel, xml_parse(@response.body)
+
+      # now check the version in the history
+      get api_relation_version_path(relation, new_version)
+      assert_response :success
+      assert_tags_equal rel, xml_parse(@response.body)
     end
 
     ##
@@ -419,23 +425,29 @@ module Api
 
       auth_header = bearer_authorization_header user
 
-      with_relation(relation.id) do |rel|
-        # alter one of the tags
-        tag = rel.find("//osm/relation/tag").first
-        tag["v"] = "some changed value"
-        update_changeset(rel, changeset.id)
+      get api_relation_path(relation)
+      assert_response :success
+      rel = xml_parse(@response.body)
 
-        # check that the downloaded tags are the same as the uploaded tags...
-        new_version = with_update_diff(rel, auth_header) do |new_rel|
-          assert_tags_equal rel, new_rel
-        end
+      # alter one of the tags
+      tag = rel.find("//osm/relation/tag").first
+      tag["v"] = "some changed value"
+      update_changeset(rel, changeset.id)
 
-        # check the original one in the current_* table again
-        with_relation(relation.id) { |r| assert_tags_equal rel, r }
-
-        # now check the version in the history
-        with_relation(relation.id, new_version) { |r| assert_tags_equal rel, r }
+      # check that the downloaded tags are the same as the uploaded tags...
+      new_version = with_update_diff(rel, auth_header) do |new_rel|
+        assert_tags_equal rel, new_rel
       end
+
+      # check the original one in the current_* table again
+      get api_relation_path(relation)
+      assert_response :success
+      assert_tags_equal rel, xml_parse(@response.body)
+
+      # now check the version in the history
+      get api_relation_version_path(relation, new_version)
+      assert_response :success
+      assert_tags_equal rel, xml_parse(@response.body)
     end
 
     def test_update_wrong_id
@@ -445,11 +457,13 @@ module Api
       other_relation = create(:relation)
 
       auth_header = bearer_authorization_header user
-      with_relation(relation.id) do |rel|
-        update_changeset(rel, changeset.id)
-        put api_relation_path(other_relation), :params => rel.to_s, :headers => auth_header
-        assert_response :bad_request
-      end
+      get api_relation_path(relation)
+      assert_response :success
+      rel = xml_parse(@response.body)
+
+      update_changeset(rel, changeset.id)
+      put api_relation_path(other_relation), :params => rel.to_s, :headers => auth_header
+      assert_response :bad_request
     end
 
     # -------------------------------------
@@ -1073,22 +1087,6 @@ module Api
       end
     end
 
-    ##
-    # yields the relation with the given +id+ (and optional +version+
-    # to read from the history tables) into the block. the parsed XML
-    # doc is returned.
-    def with_relation(id, ver = nil)
-      if ver.nil?
-        get api_relation_path(id)
-      else
-        with_controller(OldRelationsController.new) do
-          get api_relation_version_path(id, ver)
-        end
-      end
-      assert_response :success
-      yield xml_parse(@response.body)
-    end
-
     ##
     # updates the relation (XML) +rel+ and
     # yields the new version of that relation into the block.

From 2c04ad1deeb85402b7a05ad7bdcaf92a695c725c Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 06:12:14 +0300
Subject: [PATCH 06/14] Remove with_update_* block/yield

---
 .../api/relations_controller_test.rb          | 42 +++++++------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index c808c57ae..bb5b2a1bd 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -390,6 +390,7 @@ module Api
       get api_relation_path(relation)
       assert_response :success
       rel = xml_parse(@response.body)
+      rel_id = rel.find("//osm/relation").first["id"].to_i
 
       # alter one of the tags
       tag = rel.find("//osm/relation/tag").first
@@ -397,9 +398,10 @@ module Api
       update_changeset(rel, changeset.id)
 
       # check that the downloaded tags are the same as the uploaded tags...
-      new_version = with_update(rel, auth_header) do |new_rel|
-        assert_tags_equal rel, new_rel
-      end
+      new_version = do_update(rel, rel_id, auth_header)
+      get api_relation_path(rel_id)
+      assert_response :success
+      assert_tags_equal rel, xml_parse(@response.body)
 
       # check the original one in the current_* table again
       get api_relation_path(relation)
@@ -428,6 +430,7 @@ module Api
       get api_relation_path(relation)
       assert_response :success
       rel = xml_parse(@response.body)
+      rel_id = rel.find("//osm/relation").first["id"].to_i
 
       # alter one of the tags
       tag = rel.find("//osm/relation/tag").first
@@ -435,9 +438,10 @@ module Api
       update_changeset(rel, changeset.id)
 
       # check that the downloaded tags are the same as the uploaded tags...
-      new_version = with_update_diff(rel, auth_header) do |new_rel|
-        assert_tags_equal rel, new_rel
-      end
+      new_version = do_update_diff(rel, auth_header)
+      get api_relation_path(rel_id)
+      assert_response :success
+      assert_tags_equal rel, xml_parse(@response.body)
 
       # check the original one in the current_* table again
       get api_relation_path(relation)
@@ -1089,30 +1093,19 @@ module Api
 
     ##
     # updates the relation (XML) +rel+ and
-    # yields the new version of that relation into the block.
-    # the parsed XML doc is returned.
-    def with_update(rel, headers)
-      rel_id = rel.find("//osm/relation").first["id"].to_i
+    # returns the new version of that relation.
+    def do_update(rel, rel_id, headers)
       put api_relation_path(rel_id), :params => rel.to_s, :headers => headers
       assert_response :success, "can't update relation: #{@response.body}"
       version = @response.body.to_i
 
-      # now get the new version
-      get api_relation_path(rel_id)
-      assert_response :success
-      new_rel = xml_parse(@response.body)
-
-      yield new_rel
-
       version
     end
 
     ##
     # updates the relation (XML) +rel+ via the diff-upload API and
-    # yields the new version of that relation into the block.
-    # the parsed XML doc is returned.
-    def with_update_diff(rel, headers)
-      rel_id = rel.find("//osm/relation").first["id"].to_i
+    # returns the new version of that relation.
+    def do_update_diff(rel, headers)
       cs_id = rel.find("//osm/relation").first["changeset"].to_i
       version = nil
 
@@ -1129,13 +1122,6 @@ module Api
         version = xml_parse(@response.body).find("//diffResult/relation").first["new_version"].to_i
       end
 
-      # now get the new version
-      get api_relation_path(rel_id)
-      assert_response :success
-      new_rel = xml_parse(@response.body)
-
-      yield new_rel
-
       version
     end
 

From 3c22a993c3fff1fac5509989d96b06b5e8fddb3e Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 06:20:44 +0300
Subject: [PATCH 07/14] Change method to compare tags with response

---
 .../api/relations_controller_test.rb          | 35 +++++++++----------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index bb5b2a1bd..60b6877d6 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -400,18 +400,15 @@ module Api
       # check that the downloaded tags are the same as the uploaded tags...
       new_version = do_update(rel, rel_id, auth_header)
       get api_relation_path(rel_id)
-      assert_response :success
-      assert_tags_equal rel, xml_parse(@response.body)
+      assert_tags_equal_response rel
 
       # check the original one in the current_* table again
       get api_relation_path(relation)
-      assert_response :success
-      assert_tags_equal rel, xml_parse(@response.body)
+      assert_tags_equal_response rel
 
       # now check the version in the history
       get api_relation_version_path(relation, new_version)
-      assert_response :success
-      assert_tags_equal rel, xml_parse(@response.body)
+      assert_tags_equal_response rel
     end
 
     ##
@@ -440,18 +437,15 @@ module Api
       # check that the downloaded tags are the same as the uploaded tags...
       new_version = do_update_diff(rel, auth_header)
       get api_relation_path(rel_id)
-      assert_response :success
-      assert_tags_equal rel, xml_parse(@response.body)
+      assert_tags_equal_response rel
 
       # check the original one in the current_* table again
       get api_relation_path(relation)
-      assert_response :success
-      assert_tags_equal rel, xml_parse(@response.body)
+      assert_tags_equal_response rel
 
       # now check the version in the history
       get api_relation_version_path(relation, new_version)
-      assert_response :success
-      assert_tags_equal rel, xml_parse(@response.body)
+      assert_tags_equal_response rel
     end
 
     def test_update_wrong_id
@@ -1134,14 +1128,17 @@ module Api
     end
 
     ##
-    # assert that all tags on relation documents +a+ and +b+
-    # are equal
-    def assert_tags_equal(a, b)
-      # turn the XML doc into tags hashes
-      a_tags = get_tags_as_hash(a)
-      b_tags = get_tags_as_hash(b)
+    # assert that tags on relation document +rel+
+    # are equal to tags in response
+    def assert_tags_equal_response(rel)
+      assert_response :success
+      response_xml = xml_parse(@response.body)
 
-      assert_equal a_tags, b_tags, "Tags should be identical."
+      # turn the XML doc into tags hashes
+      rel_tags = get_tags_as_hash(rel)
+      response_tags = get_tags_as_hash(response_xml)
+
+      assert_equal rel_tags, response_tags, "Tags should be identical."
     end
 
     ##

From 4794da0eea31e4e3158c9c61f0b04eb0e5439d1f Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 06:32:31 +0300
Subject: [PATCH 08/14] Inline single-use with_update_* methods

---
 .../api/relations_controller_test.rb          | 52 ++++++-------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 60b6877d6..7e3884954 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -396,9 +396,11 @@ module Api
       tag = rel.find("//osm/relation/tag").first
       tag["v"] = "some changed value"
       update_changeset(rel, changeset.id)
+      put api_relation_path(rel_id), :params => rel.to_s, :headers => auth_header
+      assert_response :success, "can't update relation: #{@response.body}"
+      new_version = @response.body.to_i
 
       # check that the downloaded tags are the same as the uploaded tags...
-      new_version = do_update(rel, rel_id, auth_header)
       get api_relation_path(rel_id)
       assert_tags_equal_response rel
 
@@ -433,9 +435,21 @@ module Api
       tag = rel.find("//osm/relation/tag").first
       tag["v"] = "some changed value"
       update_changeset(rel, changeset.id)
+      new_version = nil
+      with_controller(Api::ChangesetsController.new) do
+        doc = OSM::API.new.xml_doc
+        change = XML::Node.new "osmChange"
+        doc.root = change
+        modify = XML::Node.new "modify"
+        change << modify
+        modify << doc.import(rel.find("//osm/relation").first)
+
+        post api_changeset_upload_path(changeset), :params => doc.to_s, :headers => auth_header
+        assert_response :success, "can't upload diff relation: #{@response.body}"
+        new_version = xml_parse(@response.body).find("//diffResult/relation").first["new_version"].to_i
+      end
 
       # check that the downloaded tags are the same as the uploaded tags...
-      new_version = do_update_diff(rel, auth_header)
       get api_relation_path(rel_id)
       assert_tags_equal_response rel
 
@@ -1085,40 +1099,6 @@ module Api
       end
     end
 
-    ##
-    # updates the relation (XML) +rel+ and
-    # returns the new version of that relation.
-    def do_update(rel, rel_id, headers)
-      put api_relation_path(rel_id), :params => rel.to_s, :headers => headers
-      assert_response :success, "can't update relation: #{@response.body}"
-      version = @response.body.to_i
-
-      version
-    end
-
-    ##
-    # updates the relation (XML) +rel+ via the diff-upload API and
-    # returns the new version of that relation.
-    def do_update_diff(rel, headers)
-      cs_id = rel.find("//osm/relation").first["changeset"].to_i
-      version = nil
-
-      with_controller(Api::ChangesetsController.new) do
-        doc = OSM::API.new.xml_doc
-        change = XML::Node.new "osmChange"
-        doc.root = change
-        modify = XML::Node.new "modify"
-        change << modify
-        modify << doc.import(rel.find("//osm/relation").first)
-
-        post api_changeset_upload_path(cs_id), :params => doc.to_s, :headers => headers
-        assert_response :success, "can't upload diff relation: #{@response.body}"
-        version = xml_parse(@response.body).find("//diffResult/relation").first["new_version"].to_i
-      end
-
-      version
-    end
-
     ##
     # returns a k->v hash of tags from an xml doc
     def get_tags_as_hash(a)

From da5b59f4cc8e95add1c363d501bbf99740421181 Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 06:37:46 +0300
Subject: [PATCH 09/14] Remove unnecessary with_controller around
 check_ordering

---
 .../controllers/api/relations_controller_test.rb | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 7e3884954..42570a975 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -807,11 +807,9 @@ module Api
       check_ordering(doc, @response.body)
 
       # check the ordering in the history tables:
-      with_controller(OldRelationsController.new) do
-        get api_relation_version_path(relation_id, 2)
-        assert_response :success, "can't read back version 2 of the relation #{relation_id}"
-        check_ordering(doc, @response.body)
-      end
+      get api_relation_version_path(relation_id, 2)
+      assert_response :success, "can't read back version 2 of the relation #{relation_id}"
+      check_ordering(doc, @response.body)
     end
 
     ##
@@ -887,11 +885,9 @@ module Api
       check_ordering(doc, @response.body)
 
       # check the ordering in the history tables:
-      with_controller(OldRelationsController.new) do
-        get api_relation_version_path(relation_id, 1)
-        assert_response :success, "can't read back version 1 of the relation: #{@response.body}"
-        check_ordering(doc, @response.body)
-      end
+      get api_relation_version_path(relation_id, 1)
+      assert_response :success, "can't read back version 1 of the relation: #{@response.body}"
+      check_ordering(doc, @response.body)
     end
 
     ##

From 0da694cdda0c8d2b82dbd2289f389192dcfa25e2 Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 06:51:50 +0300
Subject: [PATCH 10/14] Convert check ordering method into assert on response

---
 .../api/relations_controller_test.rb          | 30 ++++++++-----------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 42570a975..843904f5c 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -720,8 +720,7 @@ module Api
 
           # get it back and check the ordering
           get api_relation_path(relation)
-          assert_response :success, "can't read back the relation: #{@response.body}"
-          check_ordering(relation_xml, @response.body)
+          assert_members_equal_response relation_xml
         end
       end
     end
@@ -783,8 +782,7 @@ module Api
 
       # get it back and check the ordering
       get api_relation_path(relation_id)
-      assert_response :success, "can't read back the relation: #{@response.body}"
-      check_ordering(doc, @response.body)
+      assert_members_equal_response doc
 
       # insert a member at the front
       new_member = XML::Node.new "member"
@@ -803,13 +801,11 @@ module Api
 
       # get it back again and check the ordering again
       get api_relation_path(relation_id)
-      assert_response :success, "can't read back the relation: #{@response.body}"
-      check_ordering(doc, @response.body)
+      assert_members_equal_response doc
 
       # check the ordering in the history tables:
       get api_relation_version_path(relation_id, 2)
-      assert_response :success, "can't read back version 2 of the relation #{relation_id}"
-      check_ordering(doc, @response.body)
+      assert_members_equal_response doc, "can't read back version 2 of the relation"
     end
 
     ##
@@ -848,8 +844,7 @@ module Api
 
       # get it back and check the ordering
       get api_relation_path(relation_id)
-      assert_response :success, "can't read back the relation: #{relation_id}"
-      check_ordering(doc, @response.body)
+      assert_members_equal_response doc
     end
 
     ##
@@ -881,13 +876,11 @@ module Api
 
       # check the ordering in the current tables:
       get api_relation_path(relation_id)
-      assert_response :success, "can't read back the relation: #{@response.body}"
-      check_ordering(doc, @response.body)
+      assert_members_equal_response doc
 
       # check the ordering in the history tables:
       get api_relation_version_path(relation_id, 1)
-      assert_response :success, "can't read back version 1 of the relation: #{@response.body}"
-      check_ordering(doc, @response.body)
+      assert_members_equal_response doc, "can't read back version 1 of the relation"
     end
 
     ##
@@ -1036,10 +1029,11 @@ module Api
     private
 
     ##
-    # checks that the XML document and the string arguments have
+    # checks that the XML document and the response have
     # members in the same order.
-    def check_ordering(doc, xml)
-      new_doc = XML::Parser.string(xml).parse
+    def assert_members_equal_response(doc, response_message = "can't read back the relation")
+      assert_response :success, "#{response_message}: #{@response.body}"
+      new_doc = XML::Parser.string(@response.body).parse
 
       doc_members = doc.find("//osm/relation/member").collect do |m|
         [m["ref"].to_i, m["type"].to_sym, m["role"]]
@@ -1049,7 +1043,7 @@ module Api
         [m["ref"].to_i, m["type"].to_sym, m["role"]]
       end
 
-      assert_equal doc_members, new_members, "members are not equal - ordering is wrong? (#{doc}, #{xml})"
+      assert_equal doc_members, new_members, "members are not equal - ordering is wrong? (#{doc}, #{@response.body})"
     end
 
     ##

From 4a37700b23422f5d543406bede91c1e8dd65abad Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 07:26:02 +0300
Subject: [PATCH 11/14] Move relation-changeset integration tests out of
 controller tests

---
 .../api/relations_controller_test.rb          | 170 ---------------
 test/integration/changeset_bbox_test.rb       | 194 +++++++++++++++++-
 2 files changed, 191 insertions(+), 173 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 843904f5c..792dd33c4 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -656,101 +656,6 @@ module Api
       assert_response :not_found
     end
 
-    ##
-    # when a relation's tag is modified then it should put the bounding
-    # box of all its members into the changeset.
-    def test_tag_modify_bounding_box
-      relation = create(:relation)
-      node1 = create(:node, :lat => 0.3, :lon => 0.3)
-      node2 = create(:node, :lat => 0.5, :lon => 0.5)
-      way = create(:way)
-      create(:way_node, :way => way, :node => node1)
-      create(:relation_member, :relation => relation, :member => way)
-      create(:relation_member, :relation => relation, :member => node2)
-      # the relation contains nodes1 and node2 (node1
-      # indirectly via the way), so the bbox should be [0.3,0.3,0.5,0.5].
-      check_changeset_modify(BoundingBox.new(0.3, 0.3, 0.5, 0.5)) do |changeset_id, auth_header|
-        # add a tag to an existing relation
-        relation_xml = xml_for_relation(relation)
-        relation_element = relation_xml.find("//osm/relation").first
-        new_tag = XML::Node.new("tag")
-        new_tag["k"] = "some_new_tag"
-        new_tag["v"] = "some_new_value"
-        relation_element << new_tag
-
-        # update changeset ID to point to new changeset
-        update_changeset(relation_xml, changeset_id)
-
-        # upload the change
-        put api_relation_path(relation), :params => relation_xml.to_s, :headers => auth_header
-        assert_response :success, "can't update relation for tag/bbox test"
-      end
-    end
-
-    ##
-    # add a member to a relation and check the bounding box is only that
-    # element.
-    def test_add_member_bounding_box
-      relation = create(:relation)
-      node1 = create(:node, :lat => 4, :lon => 4)
-      node2 = create(:node, :lat => 7, :lon => 7)
-      way1 = create(:way)
-      create(:way_node, :way => way1, :node => create(:node, :lat => 8, :lon => 8))
-      way2 = create(:way)
-      create(:way_node, :way => way2, :node => create(:node, :lat => 9, :lon => 9), :sequence_id => 1)
-      create(:way_node, :way => way2, :node => create(:node, :lat => 10, :lon => 10), :sequence_id => 2)
-
-      [node1, node2, way1, way2].each do |element|
-        bbox = element.bbox.to_unscaled
-        check_changeset_modify(bbox) do |changeset_id, auth_header|
-          relation_xml = xml_for_relation(Relation.find(relation.id))
-          relation_element = relation_xml.find("//osm/relation").first
-          new_member = XML::Node.new("member")
-          new_member["ref"] = element.id.to_s
-          new_member["type"] = element.class.to_s.downcase
-          new_member["role"] = "some_role"
-          relation_element << new_member
-
-          # update changeset ID to point to new changeset
-          update_changeset(relation_xml, changeset_id)
-
-          # upload the change
-          put api_relation_path(relation), :params => relation_xml.to_s, :headers => auth_header
-          assert_response :success, "can't update relation for add #{element.class}/bbox test: #{@response.body}"
-
-          # get it back and check the ordering
-          get api_relation_path(relation)
-          assert_members_equal_response relation_xml
-        end
-      end
-    end
-
-    ##
-    # remove a member from a relation and check the bounding box is
-    # only that element.
-    def test_remove_member_bounding_box
-      relation = create(:relation)
-      node1 = create(:node, :lat => 3, :lon => 3)
-      node2 = create(:node, :lat => 5, :lon => 5)
-      create(:relation_member, :relation => relation, :member => node1)
-      create(:relation_member, :relation => relation, :member => node2)
-
-      check_changeset_modify(BoundingBox.new(5, 5, 5, 5)) do |changeset_id, auth_header|
-        # remove node 5 (5,5) from an existing relation
-        relation_xml = xml_for_relation(relation)
-        relation_xml
-          .find("//osm/relation/member[@type='node'][@ref='#{node2.id}']")
-          .first.remove!
-
-        # update changeset ID to point to new changeset
-        update_changeset(relation_xml, changeset_id)
-
-        # upload the change
-        put api_relation_path(relation), :params => relation_xml.to_s, :headers => auth_header
-        assert_response :success, "can't update relation for remove node/bbox test"
-      end
-    end
-
     ##
     # check that relations are ordered
     def test_relation_member_ordering
@@ -883,38 +788,6 @@ module Api
       assert_members_equal_response doc, "can't read back version 1 of the relation"
     end
 
-    ##
-    # remove all the members from a relation. the result is pretty useless, but
-    # still technically valid.
-    def test_remove_all_members
-      relation = create(:relation)
-      node1 = create(:node, :lat => 0.3, :lon => 0.3)
-      node2 = create(:node, :lat => 0.5, :lon => 0.5)
-      way = create(:way)
-      create(:way_node, :way => way, :node => node1)
-      create(:relation_member, :relation => relation, :member => way)
-      create(:relation_member, :relation => relation, :member => node2)
-
-      check_changeset_modify(BoundingBox.new(0.3, 0.3, 0.5, 0.5)) do |changeset_id, auth_header|
-        relation_xml = xml_for_relation(relation)
-        relation_xml
-          .find("//osm/relation/member")
-          .each(&:remove!)
-
-        # update changeset ID to point to new changeset
-        update_changeset(relation_xml, changeset_id)
-
-        # upload the change
-        put api_relation_path(relation), :params => relation_xml.to_s, :headers => auth_header
-        assert_response :success, "can't update relation for remove all members test"
-        checkrelation = Relation.find(relation.id)
-        assert_not_nil(checkrelation,
-                       "uploaded relation not found in database after upload")
-        assert_equal(0, checkrelation.members.length,
-                     "relation contains members but they should have all been deleted")
-      end
-    end
-
     ##
     # test initial rate limit
     def test_initial_rate_limit
@@ -1046,49 +919,6 @@ module Api
       assert_equal doc_members, new_members, "members are not equal - ordering is wrong? (#{doc}, #{@response.body})"
     end
 
-    ##
-    # create a changeset and yield to the caller to set it up, then assert
-    # that the changeset bounding box is +bbox+.
-    def check_changeset_modify(bbox)
-      ## First test with the private user to check that you get a forbidden
-      auth_header = bearer_authorization_header create(:user, :data_public => false)
-
-      # create a new changeset for this operation, so we are assured
-      # that the bounding box will be newly-generated.
-      with_controller(Api::ChangesetsController.new) do
-        xml = "<osm><changeset/></osm>"
-        post api_changesets_path, :params => xml, :headers => auth_header
-        assert_response :forbidden, "shouldn't be able to create changeset for modify test, as should get forbidden"
-      end
-
-      ## Now do the whole thing with the public user
-      auth_header = bearer_authorization_header
-
-      # create a new changeset for this operation, so we are assured
-      # that the bounding box will be newly-generated.
-      changeset_id = with_controller(Api::ChangesetsController.new) do
-        xml = "<osm><changeset/></osm>"
-        post api_changesets_path, :params => xml, :headers => auth_header
-        assert_response :success, "couldn't create changeset for modify test"
-        @response.body.to_i
-      end
-
-      # go back to the block to do the actual modifies
-      yield changeset_id, auth_header
-
-      # now download the changeset to check its bounding box
-      with_controller(Api::ChangesetsController.new) do
-        get api_changeset_path(changeset_id)
-        assert_response :success, "can't re-read changeset for modify test"
-        assert_select "osm>changeset", 1, "Changeset element doesn't exist in #{@response.body}"
-        assert_select "osm>changeset[id='#{changeset_id}']", 1, "Changeset id=#{changeset_id} doesn't exist in #{@response.body}"
-        assert_select "osm>changeset[min_lon='#{format('%<lon>.7f', :lon => bbox.min_lon)}']", 1, "Changeset min_lon wrong in #{@response.body}"
-        assert_select "osm>changeset[min_lat='#{format('%<lat>.7f', :lat => bbox.min_lat)}']", 1, "Changeset min_lat wrong in #{@response.body}"
-        assert_select "osm>changeset[max_lon='#{format('%<lon>.7f', :lon => bbox.max_lon)}']", 1, "Changeset max_lon wrong in #{@response.body}"
-        assert_select "osm>changeset[max_lat='#{format('%<lat>.7f', :lat => bbox.max_lat)}']", 1, "Changeset max_lat wrong in #{@response.body}"
-      end
-    end
-
     ##
     # returns a k->v hash of tags from an xml doc
     def get_tags_as_hash(a)
diff --git a/test/integration/changeset_bbox_test.rb b/test/integration/changeset_bbox_test.rb
index 4774fe5e3..151c91aab 100644
--- a/test/integration/changeset_bbox_test.rb
+++ b/test/integration/changeset_bbox_test.rb
@@ -61,18 +61,206 @@ class ChangesetBboxTest < ActionDispatch::IntegrationTest
     assert_dom "osm>changeset[max_lat='0.3000000']", 1
   end
 
+  ##
+  # when a relation's tag is modified then it should put the bounding
+  # box of all its members into the changeset.
+  def test_relation_tag_modify_bounding_box
+    relation = create(:relation)
+    node1 = create(:node, :lat => 0.3, :lon => 0.3)
+    node2 = create(:node, :lat => 0.5, :lon => 0.5)
+    way = create(:way)
+    create(:way_node, :way => way, :node => node1)
+    create(:relation_member, :relation => relation, :member => way)
+    create(:relation_member, :relation => relation, :member => node2)
+    # the relation contains nodes1 and node2 (node1
+    # indirectly via the way), so the bbox should be [0.3,0.3,0.5,0.5].
+    check_changeset_modify(BoundingBox.new(0.3, 0.3, 0.5, 0.5)) do |changeset_id, auth_header|
+      # add a tag to an existing relation
+      relation_xml = xml_for_relation(relation)
+      relation_element = relation_xml.find("//osm/relation").first
+      new_tag = XML::Node.new("tag")
+      new_tag["k"] = "some_new_tag"
+      new_tag["v"] = "some_new_value"
+      relation_element << new_tag
+
+      # update changeset ID to point to new changeset
+      update_changeset(relation_xml, changeset_id)
+
+      # upload the change
+      put api_relation_path(relation), :params => relation_xml.to_s, :headers => auth_header
+      assert_response :success, "can't update relation for tag/bbox test"
+    end
+  end
+
+  ##
+  # add a member to a relation and check the bounding box is only that
+  # element.
+  def test_relation_add_member_bounding_box
+    relation = create(:relation)
+    node1 = create(:node, :lat => 4, :lon => 4)
+    node2 = create(:node, :lat => 7, :lon => 7)
+    way1 = create(:way)
+    create(:way_node, :way => way1, :node => create(:node, :lat => 8, :lon => 8))
+    way2 = create(:way)
+    create(:way_node, :way => way2, :node => create(:node, :lat => 9, :lon => 9), :sequence_id => 1)
+    create(:way_node, :way => way2, :node => create(:node, :lat => 10, :lon => 10), :sequence_id => 2)
+
+    [node1, node2, way1, way2].each do |element|
+      bbox = element.bbox.to_unscaled
+      check_changeset_modify(bbox) do |changeset_id, auth_header|
+        relation_xml = xml_for_relation(Relation.find(relation.id))
+        relation_element = relation_xml.find("//osm/relation").first
+        new_member = XML::Node.new("member")
+        new_member["ref"] = element.id.to_s
+        new_member["type"] = element.class.to_s.downcase
+        new_member["role"] = "some_role"
+        relation_element << new_member
+
+        # update changeset ID to point to new changeset
+        update_changeset(relation_xml, changeset_id)
+
+        # upload the change
+        put api_relation_path(relation), :params => relation_xml.to_s, :headers => auth_header
+        assert_response :success, "can't update relation for add #{element.class}/bbox test: #{@response.body}"
+
+        # get it back and check the ordering
+        get api_relation_path(relation)
+        assert_members_equal_response relation_xml
+      end
+    end
+  end
+
+  ##
+  # remove a member from a relation and check the bounding box is
+  # only that element.
+  def test_relation_remove_member_bounding_box
+    relation = create(:relation)
+    node1 = create(:node, :lat => 3, :lon => 3)
+    node2 = create(:node, :lat => 5, :lon => 5)
+    create(:relation_member, :relation => relation, :member => node1)
+    create(:relation_member, :relation => relation, :member => node2)
+
+    check_changeset_modify(BoundingBox.new(5, 5, 5, 5)) do |changeset_id, auth_header|
+      # remove node 5 (5,5) from an existing relation
+      relation_xml = xml_for_relation(relation)
+      relation_xml
+        .find("//osm/relation/member[@type='node'][@ref='#{node2.id}']")
+        .first.remove!
+
+      # update changeset ID to point to new changeset
+      update_changeset(relation_xml, changeset_id)
+
+      # upload the change
+      put api_relation_path(relation), :params => relation_xml.to_s, :headers => auth_header
+      assert_response :success, "can't update relation for remove node/bbox test"
+    end
+  end
+
+  ##
+  # remove all the members from a relation. the result is pretty useless, but
+  # still technically valid.
+  def test_relation_remove_all_members
+    relation = create(:relation)
+    node1 = create(:node, :lat => 0.3, :lon => 0.3)
+    node2 = create(:node, :lat => 0.5, :lon => 0.5)
+    way = create(:way)
+    create(:way_node, :way => way, :node => node1)
+    create(:relation_member, :relation => relation, :member => way)
+    create(:relation_member, :relation => relation, :member => node2)
+
+    check_changeset_modify(BoundingBox.new(0.3, 0.3, 0.5, 0.5)) do |changeset_id, auth_header|
+      relation_xml = xml_for_relation(relation)
+      relation_xml
+        .find("//osm/relation/member")
+        .each(&:remove!)
+
+      # update changeset ID to point to new changeset
+      update_changeset(relation_xml, changeset_id)
+
+      # upload the change
+      put api_relation_path(relation), :params => relation_xml.to_s, :headers => auth_header
+      assert_response :success, "can't update relation for remove all members test"
+      checkrelation = Relation.find(relation.id)
+      assert_not_nil(checkrelation,
+                     "uploaded relation not found in database after upload")
+      assert_equal(0, checkrelation.members.length,
+                   "relation contains members but they should have all been deleted")
+    end
+  end
+
   private
 
   ##
-  # update the changeset_id of a way element
+  # create a changeset and yield to the caller to set it up, then assert
+  # that the changeset bounding box is +bbox+.
+  def check_changeset_modify(bbox)
+    ## First test with the private user to check that you get a forbidden
+    auth_header = bearer_authorization_header create(:user, :data_public => false)
+
+    # create a new changeset for this operation, so we are assured
+    # that the bounding box will be newly-generated.
+    with_controller(Api::ChangesetsController.new) do
+      xml = "<osm><changeset/></osm>"
+      post api_changesets_path, :params => xml, :headers => auth_header
+      assert_response :forbidden, "shouldn't be able to create changeset for modify test, as should get forbidden"
+    end
+
+    ## Now do the whole thing with the public user
+    auth_header = bearer_authorization_header
+
+    # create a new changeset for this operation, so we are assured
+    # that the bounding box will be newly-generated.
+    changeset_id = with_controller(Api::ChangesetsController.new) do
+      xml = "<osm><changeset/></osm>"
+      post api_changesets_path, :params => xml, :headers => auth_header
+      assert_response :success, "couldn't create changeset for modify test"
+      @response.body.to_i
+    end
+
+    # go back to the block to do the actual modifies
+    yield changeset_id, auth_header
+
+    # now download the changeset to check its bounding box
+    with_controller(Api::ChangesetsController.new) do
+      get api_changeset_path(changeset_id)
+      assert_response :success, "can't re-read changeset for modify test"
+      assert_select "osm>changeset", 1, "Changeset element doesn't exist in #{@response.body}"
+      assert_select "osm>changeset[id='#{changeset_id}']", 1, "Changeset id=#{changeset_id} doesn't exist in #{@response.body}"
+      assert_select "osm>changeset[min_lon='#{format('%<lon>.7f', :lon => bbox.min_lon)}']", 1, "Changeset min_lon wrong in #{@response.body}"
+      assert_select "osm>changeset[min_lat='#{format('%<lat>.7f', :lat => bbox.min_lat)}']", 1, "Changeset min_lat wrong in #{@response.body}"
+      assert_select "osm>changeset[max_lon='#{format('%<lon>.7f', :lon => bbox.max_lon)}']", 1, "Changeset max_lon wrong in #{@response.body}"
+      assert_select "osm>changeset[max_lat='#{format('%<lat>.7f', :lat => bbox.max_lat)}']", 1, "Changeset max_lat wrong in #{@response.body}"
+    end
+  end
+
+  ##
+  # checks that the XML document and the response have
+  # members in the same order.
+  def assert_members_equal_response(doc, response_message = "can't read back the relation")
+    assert_response :success, "#{response_message}: #{@response.body}"
+    new_doc = XML::Parser.string(@response.body).parse
+
+    doc_members = doc.find("//osm/relation/member").collect do |m|
+      [m["ref"].to_i, m["type"].to_sym, m["role"]]
+    end
+
+    new_members = new_doc.find("//osm/relation/member").collect do |m|
+      [m["ref"].to_i, m["type"].to_sym, m["role"]]
+    end
+
+    assert_equal doc_members, new_members, "members are not equal - ordering is wrong? (#{doc}, #{@response.body})"
+  end
+
+  ##
+  # update the changeset_id of an element
   def update_changeset(xml, changeset_id)
     xml_attr_rewrite(xml, "changeset", changeset_id)
   end
 
   ##
-  # update an attribute in a way element
+  # update an attribute in an element
   def xml_attr_rewrite(xml, name, value)
-    xml.find("//osm/way").first[name] = value.to_s
+    xml.find("//osm/*[self::node or self::way or self::relation]").first[name] = value.to_s
     xml
   end
 end

From d71d6f540922c68d312977cbc35b00223849bcd7 Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 07:30:39 +0300
Subject: [PATCH 12/14] Add extra members test for history table

---
 test/controllers/api/relations_controller_test.rb | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index 792dd33c4..b52274a07 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -689,6 +689,10 @@ module Api
       get api_relation_path(relation_id)
       assert_members_equal_response doc
 
+      # check the ordering in the history tables:
+      get api_relation_version_path(relation_id, 1)
+      assert_members_equal_response doc, "can't read back version 2 of the relation"
+
       # insert a member at the front
       new_member = XML::Node.new "member"
       new_member["ref"] = node3.id.to_s
@@ -750,6 +754,10 @@ module Api
       # get it back and check the ordering
       get api_relation_path(relation_id)
       assert_members_equal_response doc
+
+      # check the ordering in the history tables:
+      get api_relation_version_path(relation_id, 1)
+      assert_members_equal_response doc, "can't read back version 1 of the relation"
     end
 
     ##

From f6b2283c933446a846045e9b735b24e9a461565f Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 07:38:20 +0300
Subject: [PATCH 13/14] Move tests comparing relation members with response to
 integration tests

---
 .../api/relations_controller_test.rb          | 158 -----------------
 test/integration/relation_versions_test.rb    | 163 ++++++++++++++++++
 2 files changed, 163 insertions(+), 158 deletions(-)
 create mode 100644 test/integration/relation_versions_test.rb

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index b52274a07..d9fb435f1 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -656,146 +656,6 @@ module Api
       assert_response :not_found
     end
 
-    ##
-    # check that relations are ordered
-    def test_relation_member_ordering
-      user = create(:user)
-      changeset = create(:changeset, :user => user)
-      node1 = create(:node)
-      node2 = create(:node)
-      node3 = create(:node)
-      way1 = create(:way_with_nodes, :nodes_count => 2)
-      way2 = create(:way_with_nodes, :nodes_count => 2)
-
-      auth_header = bearer_authorization_header user
-
-      doc_str = <<~OSM
-        <osm>
-         <relation changeset='#{changeset.id}'>
-          <member ref='#{node1.id}' type='node' role='first'/>
-          <member ref='#{node2.id}' type='node' role='second'/>
-          <member ref='#{way1.id}' type='way' role='third'/>
-          <member ref='#{way2.id}' type='way' role='fourth'/>
-         </relation>
-        </osm>
-      OSM
-      doc = XML::Parser.string(doc_str).parse
-
-      post api_relations_path, :params => doc.to_s, :headers => auth_header
-      assert_response :success, "can't create a relation: #{@response.body}"
-      relation_id = @response.body.to_i
-
-      # get it back and check the ordering
-      get api_relation_path(relation_id)
-      assert_members_equal_response doc
-
-      # check the ordering in the history tables:
-      get api_relation_version_path(relation_id, 1)
-      assert_members_equal_response doc, "can't read back version 2 of the relation"
-
-      # insert a member at the front
-      new_member = XML::Node.new "member"
-      new_member["ref"] = node3.id.to_s
-      new_member["type"] = "node"
-      new_member["role"] = "new first"
-      doc.find("//osm/relation").first.child.prev = new_member
-      # update the version, should be 1?
-      doc.find("//osm/relation").first["id"] = relation_id.to_s
-      doc.find("//osm/relation").first["version"] = 1.to_s
-
-      # upload the next version of the relation
-      put api_relation_path(relation_id), :params => doc.to_s, :headers => auth_header
-      assert_response :success, "can't update relation: #{@response.body}"
-      assert_equal 2, @response.body.to_i
-
-      # get it back again and check the ordering again
-      get api_relation_path(relation_id)
-      assert_members_equal_response doc
-
-      # check the ordering in the history tables:
-      get api_relation_version_path(relation_id, 2)
-      assert_members_equal_response doc, "can't read back version 2 of the relation"
-    end
-
-    ##
-    # check that relations can contain duplicate members
-    def test_relation_member_duplicates
-      private_user = create(:user, :data_public => false)
-      user = create(:user)
-      changeset = create(:changeset, :user => user)
-      node1 = create(:node)
-      node2 = create(:node)
-
-      doc_str = <<~OSM
-        <osm>
-         <relation changeset='#{changeset.id}'>
-          <member ref='#{node1.id}' type='node' role='forward'/>
-          <member ref='#{node2.id}' type='node' role='forward'/>
-          <member ref='#{node1.id}' type='node' role='forward'/>
-          <member ref='#{node2.id}' type='node' role='forward'/>
-         </relation>
-        </osm>
-      OSM
-      doc = XML::Parser.string(doc_str).parse
-
-      ## First try with the private user
-      auth_header = bearer_authorization_header private_user
-
-      post api_relations_path, :params => doc.to_s, :headers => auth_header
-      assert_response :forbidden
-
-      ## Now try with the public user
-      auth_header = bearer_authorization_header user
-
-      post api_relations_path, :params => doc.to_s, :headers => auth_header
-      assert_response :success, "can't create a relation: #{@response.body}"
-      relation_id = @response.body.to_i
-
-      # get it back and check the ordering
-      get api_relation_path(relation_id)
-      assert_members_equal_response doc
-
-      # check the ordering in the history tables:
-      get api_relation_version_path(relation_id, 1)
-      assert_members_equal_response doc, "can't read back version 1 of the relation"
-    end
-
-    ##
-    # test that the ordering of elements in the history is the same as in current.
-    def test_history_ordering
-      user = create(:user)
-      changeset = create(:changeset, :user => user)
-      node1 = create(:node)
-      node2 = create(:node)
-      node3 = create(:node)
-      node4 = create(:node)
-
-      doc_str = <<~OSM
-        <osm>
-         <relation changeset='#{changeset.id}'>
-          <member ref='#{node1.id}' type='node' role='forward'/>
-          <member ref='#{node4.id}' type='node' role='forward'/>
-          <member ref='#{node3.id}' type='node' role='forward'/>
-          <member ref='#{node2.id}' type='node' role='forward'/>
-         </relation>
-        </osm>
-      OSM
-      doc = XML::Parser.string(doc_str).parse
-      auth_header = bearer_authorization_header user
-
-      post api_relations_path, :params => doc.to_s, :headers => auth_header
-      assert_response :success, "can't create a relation: #{@response.body}"
-      relation_id = @response.body.to_i
-
-      # check the ordering in the current tables:
-      get api_relation_path(relation_id)
-      assert_members_equal_response doc
-
-      # check the ordering in the history tables:
-      get api_relation_version_path(relation_id, 1)
-      assert_members_equal_response doc, "can't read back version 1 of the relation"
-    end
-
     ##
     # test initial rate limit
     def test_initial_rate_limit
@@ -909,24 +769,6 @@ module Api
 
     private
 
-    ##
-    # checks that the XML document and the response have
-    # members in the same order.
-    def assert_members_equal_response(doc, response_message = "can't read back the relation")
-      assert_response :success, "#{response_message}: #{@response.body}"
-      new_doc = XML::Parser.string(@response.body).parse
-
-      doc_members = doc.find("//osm/relation/member").collect do |m|
-        [m["ref"].to_i, m["type"].to_sym, m["role"]]
-      end
-
-      new_members = new_doc.find("//osm/relation/member").collect do |m|
-        [m["ref"].to_i, m["type"].to_sym, m["role"]]
-      end
-
-      assert_equal doc_members, new_members, "members are not equal - ordering is wrong? (#{doc}, #{@response.body})"
-    end
-
     ##
     # returns a k->v hash of tags from an xml doc
     def get_tags_as_hash(a)
diff --git a/test/integration/relation_versions_test.rb b/test/integration/relation_versions_test.rb
new file mode 100644
index 000000000..c01534832
--- /dev/null
+++ b/test/integration/relation_versions_test.rb
@@ -0,0 +1,163 @@
+require "test_helper"
+
+class RelationVersionsTest < ActionDispatch::IntegrationTest
+  ##
+  # check that relations are ordered
+  def test_relation_member_ordering
+    user = create(:user)
+    changeset = create(:changeset, :user => user)
+    node1 = create(:node)
+    node2 = create(:node)
+    node3 = create(:node)
+    way1 = create(:way_with_nodes, :nodes_count => 2)
+    way2 = create(:way_with_nodes, :nodes_count => 2)
+
+    auth_header = bearer_authorization_header user
+
+    doc_str = <<~OSM
+      <osm>
+        <relation changeset='#{changeset.id}'>
+        <member ref='#{node1.id}' type='node' role='first'/>
+        <member ref='#{node2.id}' type='node' role='second'/>
+        <member ref='#{way1.id}' type='way' role='third'/>
+        <member ref='#{way2.id}' type='way' role='fourth'/>
+        </relation>
+      </osm>
+    OSM
+    doc = XML::Parser.string(doc_str).parse
+
+    post api_relations_path, :params => doc.to_s, :headers => auth_header
+    assert_response :success, "can't create a relation: #{@response.body}"
+    relation_id = @response.body.to_i
+
+    # get it back and check the ordering
+    get api_relation_path(relation_id)
+    assert_members_equal_response doc
+
+    # check the ordering in the history tables:
+    get api_relation_version_path(relation_id, 1)
+    assert_members_equal_response doc, "can't read back version 2 of the relation"
+
+    # insert a member at the front
+    new_member = XML::Node.new "member"
+    new_member["ref"] = node3.id.to_s
+    new_member["type"] = "node"
+    new_member["role"] = "new first"
+    doc.find("//osm/relation").first.child.prev = new_member
+    # update the version, should be 1?
+    doc.find("//osm/relation").first["id"] = relation_id.to_s
+    doc.find("//osm/relation").first["version"] = 1.to_s
+
+    # upload the next version of the relation
+    put api_relation_path(relation_id), :params => doc.to_s, :headers => auth_header
+    assert_response :success, "can't update relation: #{@response.body}"
+    assert_equal 2, @response.body.to_i
+
+    # get it back again and check the ordering again
+    get api_relation_path(relation_id)
+    assert_members_equal_response doc
+
+    # check the ordering in the history tables:
+    get api_relation_version_path(relation_id, 2)
+    assert_members_equal_response doc, "can't read back version 2 of the relation"
+  end
+
+  ##
+  # check that relations can contain duplicate members
+  def test_relation_member_duplicates
+    private_user = create(:user, :data_public => false)
+    user = create(:user)
+    changeset = create(:changeset, :user => user)
+    node1 = create(:node)
+    node2 = create(:node)
+
+    doc_str = <<~OSM
+      <osm>
+        <relation changeset='#{changeset.id}'>
+        <member ref='#{node1.id}' type='node' role='forward'/>
+        <member ref='#{node2.id}' type='node' role='forward'/>
+        <member ref='#{node1.id}' type='node' role='forward'/>
+        <member ref='#{node2.id}' type='node' role='forward'/>
+        </relation>
+      </osm>
+    OSM
+    doc = XML::Parser.string(doc_str).parse
+
+    ## First try with the private user
+    auth_header = bearer_authorization_header private_user
+
+    post api_relations_path, :params => doc.to_s, :headers => auth_header
+    assert_response :forbidden
+
+    ## Now try with the public user
+    auth_header = bearer_authorization_header user
+
+    post api_relations_path, :params => doc.to_s, :headers => auth_header
+    assert_response :success, "can't create a relation: #{@response.body}"
+    relation_id = @response.body.to_i
+
+    # get it back and check the ordering
+    get api_relation_path(relation_id)
+    assert_members_equal_response doc
+
+    # check the ordering in the history tables:
+    get api_relation_version_path(relation_id, 1)
+    assert_members_equal_response doc, "can't read back version 1 of the relation"
+  end
+
+  ##
+  # test that the ordering of elements in the history is the same as in current.
+  def test_history_ordering
+    user = create(:user)
+    changeset = create(:changeset, :user => user)
+    node1 = create(:node)
+    node2 = create(:node)
+    node3 = create(:node)
+    node4 = create(:node)
+
+    doc_str = <<~OSM
+      <osm>
+        <relation changeset='#{changeset.id}'>
+        <member ref='#{node1.id}' type='node' role='forward'/>
+        <member ref='#{node4.id}' type='node' role='forward'/>
+        <member ref='#{node3.id}' type='node' role='forward'/>
+        <member ref='#{node2.id}' type='node' role='forward'/>
+        </relation>
+      </osm>
+    OSM
+    doc = XML::Parser.string(doc_str).parse
+    auth_header = bearer_authorization_header user
+
+    post api_relations_path, :params => doc.to_s, :headers => auth_header
+    assert_response :success, "can't create a relation: #{@response.body}"
+    relation_id = @response.body.to_i
+
+    # check the ordering in the current tables:
+    get api_relation_path(relation_id)
+    assert_members_equal_response doc
+
+    # check the ordering in the history tables:
+    get api_relation_version_path(relation_id, 1)
+    assert_members_equal_response doc, "can't read back version 1 of the relation"
+  end
+
+  private
+
+  ##
+  # checks that the XML document and the response have
+  # members in the same order.
+  def assert_members_equal_response(doc, response_message = "can't read back the relation")
+    assert_response :success, "#{response_message}: #{@response.body}"
+    new_doc = XML::Parser.string(@response.body).parse
+
+    doc_members = doc.find("//osm/relation/member").collect do |m|
+      [m["ref"].to_i, m["type"].to_sym, m["role"]]
+    end
+
+    new_members = new_doc.find("//osm/relation/member").collect do |m|
+      [m["ref"].to_i, m["type"].to_sym, m["role"]]
+    end
+
+    assert_equal doc_members, new_members, "members are not equal - ordering is wrong? (#{doc}, #{@response.body})"
+  end
+end

From a04b667601a48613a8eee4547cacc0cc11d405e4 Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 7 Jul 2025 07:48:42 +0300
Subject: [PATCH 14/14] Move tests comparing relation tags with response to
 integration tests

---
 .../api/relations_controller_test.rb          | 120 +---------------
 test/integration/relation_versions_test.rb    | 130 ++++++++++++++++++
 2 files changed, 131 insertions(+), 119 deletions(-)

diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb
index d9fb435f1..e2f6ca1c2 100644
--- a/test/controllers/api/relations_controller_test.rb
+++ b/test/controllers/api/relations_controller_test.rb
@@ -373,95 +373,6 @@ module Api
     # Test updating relations
     # ------------------------------------
 
-    ##
-    # test that, when tags are updated on a relation, the correct things
-    # happen to the correct tables and the API gives sensible results.
-    # this is to test a case that gregory marler noticed and posted to
-    # josm-dev.
-    ## FIXME Move this to an integration test
-    def test_update_relation_tags
-      user = create(:user)
-      changeset = create(:changeset, :user => user)
-      relation = create(:relation)
-      create_list(:relation_tag, 4, :relation => relation)
-
-      auth_header = bearer_authorization_header user
-
-      get api_relation_path(relation)
-      assert_response :success
-      rel = xml_parse(@response.body)
-      rel_id = rel.find("//osm/relation").first["id"].to_i
-
-      # alter one of the tags
-      tag = rel.find("//osm/relation/tag").first
-      tag["v"] = "some changed value"
-      update_changeset(rel, changeset.id)
-      put api_relation_path(rel_id), :params => rel.to_s, :headers => auth_header
-      assert_response :success, "can't update relation: #{@response.body}"
-      new_version = @response.body.to_i
-
-      # check that the downloaded tags are the same as the uploaded tags...
-      get api_relation_path(rel_id)
-      assert_tags_equal_response rel
-
-      # check the original one in the current_* table again
-      get api_relation_path(relation)
-      assert_tags_equal_response rel
-
-      # now check the version in the history
-      get api_relation_version_path(relation, new_version)
-      assert_tags_equal_response rel
-    end
-
-    ##
-    # test that, when tags are updated on a relation when using the diff
-    # upload function, the correct things happen to the correct tables
-    # and the API gives sensible results. this is to test a case that
-    # gregory marler noticed and posted to josm-dev.
-    def test_update_relation_tags_via_upload
-      user = create(:user)
-      changeset = create(:changeset, :user => user)
-      relation = create(:relation)
-      create_list(:relation_tag, 4, :relation => relation)
-
-      auth_header = bearer_authorization_header user
-
-      get api_relation_path(relation)
-      assert_response :success
-      rel = xml_parse(@response.body)
-      rel_id = rel.find("//osm/relation").first["id"].to_i
-
-      # alter one of the tags
-      tag = rel.find("//osm/relation/tag").first
-      tag["v"] = "some changed value"
-      update_changeset(rel, changeset.id)
-      new_version = nil
-      with_controller(Api::ChangesetsController.new) do
-        doc = OSM::API.new.xml_doc
-        change = XML::Node.new "osmChange"
-        doc.root = change
-        modify = XML::Node.new "modify"
-        change << modify
-        modify << doc.import(rel.find("//osm/relation").first)
-
-        post api_changeset_upload_path(changeset), :params => doc.to_s, :headers => auth_header
-        assert_response :success, "can't upload diff relation: #{@response.body}"
-        new_version = xml_parse(@response.body).find("//diffResult/relation").first["new_version"].to_i
-      end
-
-      # check that the downloaded tags are the same as the uploaded tags...
-      get api_relation_path(rel_id)
-      assert_tags_equal_response rel
-
-      # check the original one in the current_* table again
-      get api_relation_path(relation)
-      assert_tags_equal_response rel
-
-      # now check the version in the history
-      get api_relation_version_path(relation, new_version)
-      assert_tags_equal_response rel
-    end
-
     def test_update_wrong_id
       user = create(:user)
       changeset = create(:changeset, :user => user)
@@ -471,7 +382,7 @@ module Api
       auth_header = bearer_authorization_header user
       get api_relation_path(relation)
       assert_response :success
-      rel = xml_parse(@response.body)
+      rel = XML::Parser.string(@response.body).parse
 
       update_changeset(rel, changeset.id)
       put api_relation_path(other_relation), :params => rel.to_s, :headers => auth_header
@@ -769,28 +680,6 @@ module Api
 
     private
 
-    ##
-    # returns a k->v hash of tags from an xml doc
-    def get_tags_as_hash(a)
-      a.find("//osm/relation/tag").to_h do |tag|
-        [tag["k"], tag["v"]]
-      end
-    end
-
-    ##
-    # assert that tags on relation document +rel+
-    # are equal to tags in response
-    def assert_tags_equal_response(rel)
-      assert_response :success
-      response_xml = xml_parse(@response.body)
-
-      # turn the XML doc into tags hashes
-      rel_tags = get_tags_as_hash(rel)
-      response_tags = get_tags_as_hash(response_xml)
-
-      assert_equal rel_tags, response_tags, "Tags should be identical."
-    end
-
     ##
     # update the changeset_id of a node element
     def update_changeset(xml, changeset_id)
@@ -803,12 +692,5 @@ module Api
       xml.find("//osm/relation").first[name] = value.to_s
       xml
     end
-
-    ##
-    # parse some xml
-    def xml_parse(xml)
-      parser = XML::Parser.string(xml)
-      parser.parse
-    end
   end
 end
diff --git a/test/integration/relation_versions_test.rb b/test/integration/relation_versions_test.rb
index c01534832..7a60946c0 100644
--- a/test/integration/relation_versions_test.rb
+++ b/test/integration/relation_versions_test.rb
@@ -1,6 +1,94 @@
 require "test_helper"
 
 class RelationVersionsTest < ActionDispatch::IntegrationTest
+  ##
+  # test that, when tags are updated on a relation, the correct things
+  # happen to the correct tables and the API gives sensible results.
+  # this is to test a case that gregory marler noticed and posted to
+  # josm-dev.
+  def test_update_relation_tags
+    user = create(:user)
+    changeset = create(:changeset, :user => user)
+    relation = create(:relation)
+    create_list(:relation_tag, 4, :relation => relation)
+
+    auth_header = bearer_authorization_header user
+
+    get api_relation_path(relation)
+    assert_response :success
+    rel = xml_parse(@response.body)
+    rel_id = rel.find("//osm/relation").first["id"].to_i
+
+    # alter one of the tags
+    tag = rel.find("//osm/relation/tag").first
+    tag["v"] = "some changed value"
+    update_changeset(rel, changeset.id)
+    put api_relation_path(rel_id), :params => rel.to_s, :headers => auth_header
+    assert_response :success, "can't update relation: #{@response.body}"
+    new_version = @response.body.to_i
+
+    # check that the downloaded tags are the same as the uploaded tags...
+    get api_relation_path(rel_id)
+    assert_tags_equal_response rel
+
+    # check the original one in the current_* table again
+    get api_relation_path(relation)
+    assert_tags_equal_response rel
+
+    # now check the version in the history
+    get api_relation_version_path(relation, new_version)
+    assert_tags_equal_response rel
+  end
+
+  ##
+  # test that, when tags are updated on a relation when using the diff
+  # upload function, the correct things happen to the correct tables
+  # and the API gives sensible results. this is to test a case that
+  # gregory marler noticed and posted to josm-dev.
+  def test_update_relation_tags_via_upload
+    user = create(:user)
+    changeset = create(:changeset, :user => user)
+    relation = create(:relation)
+    create_list(:relation_tag, 4, :relation => relation)
+
+    auth_header = bearer_authorization_header user
+
+    get api_relation_path(relation)
+    assert_response :success
+    rel = xml_parse(@response.body)
+    rel_id = rel.find("//osm/relation").first["id"].to_i
+
+    # alter one of the tags
+    tag = rel.find("//osm/relation/tag").first
+    tag["v"] = "some changed value"
+    update_changeset(rel, changeset.id)
+    new_version = nil
+    with_controller(Api::ChangesetsController.new) do
+      doc = OSM::API.new.xml_doc
+      change = XML::Node.new "osmChange"
+      doc.root = change
+      modify = XML::Node.new "modify"
+      change << modify
+      modify << doc.import(rel.find("//osm/relation").first)
+
+      post api_changeset_upload_path(changeset), :params => doc.to_s, :headers => auth_header
+      assert_response :success, "can't upload diff relation: #{@response.body}"
+      new_version = xml_parse(@response.body).find("//diffResult/relation").first["new_version"].to_i
+    end
+
+    # check that the downloaded tags are the same as the uploaded tags...
+    get api_relation_path(rel_id)
+    assert_tags_equal_response rel
+
+    # check the original one in the current_* table again
+    get api_relation_path(relation)
+    assert_tags_equal_response rel
+
+    # now check the version in the history
+    get api_relation_version_path(relation, new_version)
+    assert_tags_equal_response rel
+  end
+
   ##
   # check that relations are ordered
   def test_relation_member_ordering
@@ -143,6 +231,20 @@ class RelationVersionsTest < ActionDispatch::IntegrationTest
 
   private
 
+  ##
+  # assert that tags on relation document +rel+
+  # are equal to tags in response
+  def assert_tags_equal_response(rel)
+    assert_response :success
+    response_xml = xml_parse(@response.body)
+
+    # turn the XML doc into tags hashes
+    rel_tags = get_tags_as_hash(rel)
+    response_tags = get_tags_as_hash(response_xml)
+
+    assert_equal rel_tags, response_tags, "Tags should be identical."
+  end
+
   ##
   # checks that the XML document and the response have
   # members in the same order.
@@ -160,4 +262,32 @@ class RelationVersionsTest < ActionDispatch::IntegrationTest
 
     assert_equal doc_members, new_members, "members are not equal - ordering is wrong? (#{doc}, #{@response.body})"
   end
+
+  ##
+  # returns a k->v hash of tags from an xml doc
+  def get_tags_as_hash(a)
+    a.find("//osm/relation/tag").to_h do |tag|
+      [tag["k"], tag["v"]]
+    end
+  end
+
+  ##
+  # update the changeset_id of a node element
+  def update_changeset(xml, changeset_id)
+    xml_attr_rewrite(xml, "changeset", changeset_id)
+  end
+
+  ##
+  # update an attribute in the node element
+  def xml_attr_rewrite(xml, name, value)
+    xml.find("//osm/relation").first[name] = value.to_s
+    xml
+  end
+
+  ##
+  # parse some xml
+  def xml_parse(xml)
+    parser = XML::Parser.string(xml)
+    parser.parse
+  end
 end