From 3d240ef75fc8ea35355233c3d3f947edf30f6499 Mon Sep 17 00:00:00 2001
From: Anton Khorev <tony29@yandex.ru>
Date: Sat, 3 May 2025 06:09:55 +0300
Subject: [PATCH] Move api changeset upload create tests

---
 .../api/changesets/uploads_controller_test.rb | 288 +++++++++++++++++
 .../api/changesets_controller_test.rb         | 300 ------------------
 2 files changed, 288 insertions(+), 300 deletions(-)

diff --git a/test/controllers/api/changesets/uploads_controller_test.rb b/test/controllers/api/changesets/uploads_controller_test.rb
index 57bef30f3..f6075d3a7 100644
--- a/test/controllers/api/changesets/uploads_controller_test.rb
+++ b/test/controllers/api/changesets/uploads_controller_test.rb
@@ -125,6 +125,294 @@ module Api
         assert_equal 1 * GeoRecord::SCALE, node.longitude
       end
 
+      # -------------------------------------
+      # Test creating elements.
+      # -------------------------------------
+
+      def test_upload_create_elements
+        user = create(:user)
+        changeset = create(:changeset, :user => user)
+        node = create(:node)
+        way = create(:way_with_nodes, :nodes_count => 2)
+        relation = create(:relation)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <node id='-1' lon='0' lat='0' changeset='#{changeset.id}'>
+                <tag k='foo' v='bar'/>
+                <tag k='baz' v='bat'/>
+              </node>
+              <way id='-1' changeset='#{changeset.id}'>
+                <nd ref='#{node.id}'/>
+              </way>
+            </create>
+            <create>
+              <relation id='-1' changeset='#{changeset.id}'>
+                <member type='way' role='some' ref='#{way.id}'/>
+                <member type='node' role='some' ref='#{node.id}'/>
+                <member type='relation' role='some' ref='#{relation.id}'/>
+              </relation>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header user
+
+        post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+        assert_response :success
+
+        new_node_id, new_way_id, new_rel_id = nil
+        assert_dom "diffResult[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 do
+          # inspect the response to find out what the new element IDs are
+          # check the old IDs are all present and negative one
+          # check the versions are present and equal one
+          assert_dom "> node", 1 do |(node_el)|
+            new_node_id = node_el["new_id"].to_i
+            assert_dom "> @old_id", "-1"
+            assert_dom "> @new_version", "1"
+          end
+          assert_dom "> way", 1 do |(way_el)|
+            new_way_id = way_el["new_id"].to_i
+            assert_dom "> @old_id", "-1"
+            assert_dom "> @new_version", "1"
+          end
+          assert_dom "> relation", 1 do |(rel_el)|
+            new_rel_id = rel_el["new_id"].to_i
+            assert_dom "> @old_id", "-1"
+            assert_dom "> @new_version", "1"
+          end
+        end
+
+        assert_equal 2, Node.find(new_node_id).tags.size, "new node should have two tags"
+        assert_equal 0, Way.find(new_way_id).tags.size, "new way should have no tags"
+        assert_equal 0, Relation.find(new_rel_id).tags.size, "new relation should have no tags"
+      end
+
+      ##
+      # upload an element with a really long tag value
+      def test_upload_create_node_with_tag_too_long
+        changeset = create(:changeset)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <node id='-1' lon='0' lat='0' changeset='#{changeset.id}'>
+                <tag k='foo' v='#{'x' * 256}'/>
+              </node>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        assert_no_difference "Node.count" do
+          post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+          assert_response :bad_request
+        end
+      end
+
+      def test_upload_create_nodes_with_invalid_placeholder_reuse_in_one_action_block
+        changeset = create(:changeset)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <node id='-1' lon='0' lat='0' changeset='#{changeset.id}' version='1'/>
+              <node id='-1' lon='1' lat='1' changeset='#{changeset.id}' version='1'/>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        assert_no_difference "Node.count" do
+          post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+          assert_response :bad_request
+        end
+      end
+
+      def test_upload_create_nodes_with_invalid_placeholder_reuse_in_two_action_blocks
+        changeset = create(:changeset)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <node id='-1' lon='0' lat='0' changeset='#{changeset.id}' version='1'/>
+            </create>
+            <create>
+              <node id='-1' lon='1' lat='1' changeset='#{changeset.id}' version='1'/>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        assert_no_difference "Node.count" do
+          post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+          assert_response :bad_request
+        end
+      end
+
+      def test_upload_create_way_referring_node_placeholder_defined_later
+        changeset = create(:changeset)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <way id="-1" changeset="#{changeset.id}">
+                <nd ref="-1"/>
+              </way>
+              <node id="-1" lat="1" lon="2" changeset="#{changeset.id}"/>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        assert_no_difference "Node.count" do
+          assert_no_difference "Way.count" do
+            post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+            assert_response :bad_request
+          end
+        end
+        assert_equal "Placeholder node not found for reference -1 in way -1", @response.body
+      end
+
+      def test_upload_create_way_referring_undefined_node_placeholder
+        changeset = create(:changeset)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <way id="-1" changeset="#{changeset.id}">
+                <nd ref="-1"/>
+              </way>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        assert_no_difference "Way.count" do
+          post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+          assert_response :bad_request
+        end
+        assert_equal "Placeholder node not found for reference -1 in way -1", @response.body
+      end
+
+      def test_upload_create_existing_way_referring_undefined_node_placeholder
+        changeset = create(:changeset)
+        way = create(:way)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <way id="#{way.id}" changeset="#{changeset.id}" version="1">
+                <nd ref="-1"/>
+              </way>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+        assert_response :bad_request
+        assert_equal "Placeholder node not found for reference -1 in way #{way.id}", @response.body
+
+        way.reload
+        assert_equal 1, way.version
+      end
+
+      def test_upload_create_relation_referring_undefined_node_placeholder
+        changeset = create(:changeset)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <relation id="-1" changeset="#{changeset.id}" version="1">
+                <member type="node" role="foo" ref="-1"/>
+              </relation>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        assert_no_difference "Relation.count" do
+          post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+          assert_response :bad_request
+        end
+        assert_equal "Placeholder Node not found for reference -1 in relation -1.", @response.body
+      end
+
+      def test_upload_create_existing_relation_referring_undefined_way_placeholder
+        changeset = create(:changeset)
+        relation = create(:relation)
+
+        diff = <<~CHANGESET
+          <osmChange>
+            <create>
+              <relation id="#{relation.id}" changeset="#{changeset.id}" version="1">
+                <member type="way" role="bar" ref="-1"/>
+              </relation>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
+
+        assert_response :bad_request
+        assert_equal "Placeholder Way not found for reference -1 in relation #{relation.id}.", @response.body
+
+        relation.reload
+        assert_equal 1, relation.version
+      end
+
+      def test_upload_create_relations_with_circular_references
+        changeset = create(:changeset)
+
+        diff = <<~CHANGESET
+          <osmChange version='0.6'>
+            <create>
+              <relation id='-2' version='0' changeset='#{changeset.id}'>
+                <member type='relation' role='' ref='-4' />
+                <tag k='type' v='route' />
+                <tag k='name' v='AtoB' />
+              </relation>
+              <relation id='-3' version='0' changeset='#{changeset.id}'>
+                <tag k='type' v='route' />
+                <tag k='name' v='BtoA' />
+              </relation>
+              <relation id='-4' version='0' changeset='#{changeset.id}'>
+                <member type='relation' role='' ref='-2' />
+                <member type='relation' role='' ref='-3' />
+                <tag k='type' v='route_master' />
+                <tag k='name' v='master' />
+              </relation>
+            </create>
+          </osmChange>
+        CHANGESET
+
+        auth_header = bearer_authorization_header changeset.user
+
+        post api_changeset_upload_path(changeset), :params => diff.to_s, :headers => auth_header
+
+        assert_response :bad_request
+        assert_equal "Placeholder Relation not found for reference -4 in relation -2.", @response.body
+      end
+
       # -------------------------------------
       # Test modifying elements.
       # -------------------------------------
diff --git a/test/controllers/api/changesets_controller_test.rb b/test/controllers/api/changesets_controller_test.rb
index 68e527886..4dcadbedf 100644
--- a/test/controllers/api/changesets_controller_test.rb
+++ b/test/controllers/api/changesets_controller_test.rb
@@ -643,73 +643,6 @@ module Api
       end
     end
 
-    ##
-    # upload something which creates new objects using placeholders
-    def test_upload_create_valid
-      user = create(:user)
-      changeset = create(:changeset, :user => user)
-      node = create(:node)
-      way = create(:way_with_nodes, :nodes_count => 2)
-      relation = create(:relation)
-
-      auth_header = bearer_authorization_header user
-
-      # simple diff to create a node way and relation using placeholders
-      diff = <<~CHANGESET
-        <osmChange>
-         <create>
-          <node id='-1' lon='0' lat='0' changeset='#{changeset.id}'>
-           <tag k='foo' v='bar'/>
-           <tag k='baz' v='bat'/>
-          </node>
-          <way id='-1' changeset='#{changeset.id}'>
-           <nd ref='#{node.id}'/>
-          </way>
-         </create>
-         <create>
-          <relation id='-1' changeset='#{changeset.id}'>
-           <member type='way' role='some' ref='#{way.id}'/>
-           <member type='node' role='some' ref='#{node.id}'/>
-           <member type='relation' role='some' ref='#{relation.id}'/>
-          </relation>
-         </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :success,
-                      "can't upload a simple valid creation to changeset: #{@response.body}"
-
-      # check the returned payload
-      new_node_id, new_way_id, new_rel_id = nil
-      assert_dom "diffResult[version='#{Settings.api_version}'][generator='#{Settings.generator}']", 1 do
-        # inspect the response to find out what the new element IDs are
-        # check the old IDs are all present and negative one
-        # check the versions are present and equal one
-        assert_dom "> node", 1 do |(node_el)|
-          new_node_id = node_el["new_id"].to_i
-          assert_dom "> @old_id", "-1"
-          assert_dom "> @new_version", "1"
-        end
-        assert_dom "> way", 1 do |(way_el)|
-          new_way_id = way_el["new_id"].to_i
-          assert_dom "> @old_id", "-1"
-          assert_dom "> @new_version", "1"
-        end
-        assert_dom "> relation", 1 do |(rel_el)|
-          new_rel_id = rel_el["new_id"].to_i
-          assert_dom "> @old_id", "-1"
-          assert_dom "> @new_version", "1"
-        end
-      end
-
-      # check that the changes made it into the database
-      assert_equal 2, Node.find(new_node_id).tags.size, "new node should have two tags"
-      assert_equal 0, Way.find(new_way_id).tags.size, "new way should have no tags"
-      assert_equal 0, Relation.find(new_rel_id).tags.size, "new relation should have no tags"
-    end
-
     ##
     # test a complex delete where we delete elements which rely on eachother
     # in the same transaction.
@@ -952,30 +885,6 @@ module Api
       assert Relation.find(used_relation.id).visible
     end
 
-    ##
-    # upload an element with a really long tag value
-    def test_upload_invalid_too_long_tag
-      changeset = create(:changeset)
-
-      auth_header = bearer_authorization_header changeset.user
-
-      # simple diff to create a node way and relation using placeholders
-      diff = <<~CHANGESET
-        <osmChange>
-         <create>
-          <node id='-1' lon='0' lat='0' changeset='#{changeset.id}'>
-           <tag k='foo' v='#{'x' * 256}'/>
-          </node>
-         </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :bad_request,
-                      "shouldn't be able to upload too long a tag to changeset: #{@response.body}"
-    end
-
     ##
     # upload something which creates new objects and inserts them into
     # existing containers using placeholders.
@@ -1258,71 +1167,6 @@ module Api
       assert_select "diffResult>node[old_id='-1']", 3
     end
 
-    ##
-    # test what happens if a diff upload re-uses placeholder IDs in an
-    # illegal way.
-    def test_upload_placeholder_invalid
-      changeset = create(:changeset)
-
-      auth_header = bearer_authorization_header changeset.user
-
-      diff = <<~CHANGESET
-        <osmChange>
-         <create>
-          <node id='-1' lon='0' lat='0' changeset='#{changeset.id}' version='1'/>
-          <node id='-1' lon='1' lat='1' changeset='#{changeset.id}' version='1'/>
-          <node id='-1' lon='2' lat='2' changeset='#{changeset.id}' version='2'/>
-         </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :bad_request,
-                      "shouldn't be able to re-use placeholder IDs"
-
-      # placeholder_ids must be unique across all action blocks
-      diff = <<~CHANGESET
-        <osmChange>
-         <create>
-          <node id='-1' lon='0' lat='0' changeset='#{changeset.id}' version='1'/>
-         </create>
-         <create>
-          <node id='-1' lon='1' lat='1' changeset='#{changeset.id}' version='1'/>
-         </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :bad_request,
-                      "shouldn't be able to re-use placeholder IDs"
-    end
-
-    def test_upload_process_order
-      changeset = create(:changeset)
-
-      auth_header = bearer_authorization_header changeset.user
-
-      diff = <<~CHANGESET
-        <osmChange>
-        <create>
-          <node id="-1" lat="1" lon="2" changeset="#{changeset.id}"/>
-          <way id="-1" changeset="#{changeset.id}">
-              <nd ref="-1"/>
-              <nd ref="-2"/>
-          </way>
-          <node id="-2" lat="1" lon="2" changeset="#{changeset.id}"/>
-        </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :bad_request,
-                      "shouldn't refer elements behind it"
-    end
-
     def test_upload_duplicate_delete
       changeset = create(:changeset)
 
@@ -1366,116 +1210,6 @@ module Api
       assert_select "diffResult>node[new_version='2']", 1
     end
 
-    ##
-    # test that uploading a way referencing invalid placeholders gives a
-    # proper error, not a 500.
-    def test_upload_placeholder_invalid_way
-      changeset = create(:changeset)
-      way = create(:way)
-
-      auth_header = bearer_authorization_header changeset.user
-
-      diff = <<~CHANGESET
-        <osmChange>
-         <create>
-          <node id="-1" lon="0.0" lat="0.0" changeset="#{changeset.id}" version="1"/>
-          <node id="-2" lon="0.1" lat="0.1" changeset="#{changeset.id}" version="1"/>
-          <node id="-3" lon="0.2" lat="0.2" changeset="#{changeset.id}" version="1"/>
-          <way id="-1" changeset="#{changeset.id}" version="1">
-           <nd ref="-1"/>
-           <nd ref="-2"/>
-           <nd ref="-3"/>
-           <nd ref="-4"/>
-          </way>
-         </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :bad_request,
-                      "shouldn't be able to use invalid placeholder IDs"
-      assert_equal "Placeholder node not found for reference -4 in way -1", @response.body
-
-      # the same again, but this time use an existing way
-      diff = <<~CHANGESET
-        <osmChange>
-         <create>
-          <node id="-1" lon="0.0" lat="0.0" changeset="#{changeset.id}" version="1"/>
-          <node id="-2" lon="0.1" lat="0.1" changeset="#{changeset.id}" version="1"/>
-          <node id="-3" lon="0.2" lat="0.2" changeset="#{changeset.id}" version="1"/>
-          <way id="#{way.id}" changeset="#{changeset.id}" version="1">
-           <nd ref="-1"/>
-           <nd ref="-2"/>
-           <nd ref="-3"/>
-           <nd ref="-4"/>
-          </way>
-         </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :bad_request,
-                      "shouldn't be able to use invalid placeholder IDs"
-      assert_equal "Placeholder node not found for reference -4 in way #{way.id}", @response.body
-    end
-
-    ##
-    # test that uploading a relation referencing invalid placeholders gives a
-    # proper error, not a 500.
-    def test_upload_placeholder_invalid_relation
-      changeset = create(:changeset)
-      relation = create(:relation)
-
-      auth_header = bearer_authorization_header changeset.user
-
-      diff = <<~CHANGESET
-        <osmChange>
-         <create>
-          <node id="-1" lon="0.0" lat="0.0" changeset="#{changeset.id}" version="1"/>
-          <node id="-2" lon="0.1" lat="0.1" changeset="#{changeset.id}" version="1"/>
-          <node id="-3" lon="0.2" lat="0.2" changeset="#{changeset.id}" version="1"/>
-          <relation id="-1" changeset="#{changeset.id}" version="1">
-           <member type="node" role="foo" ref="-1"/>
-           <member type="node" role="foo" ref="-2"/>
-           <member type="node" role="foo" ref="-3"/>
-           <member type="node" role="foo" ref="-4"/>
-          </relation>
-         </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :bad_request,
-                      "shouldn't be able to use invalid placeholder IDs"
-      assert_equal "Placeholder Node not found for reference -4 in relation -1.", @response.body
-
-      # the same again, but this time use an existing relation
-      diff = <<~CHANGESET
-        <osmChange>
-         <create>
-          <node id="-1" lon="0.0" lat="0.0" changeset="#{changeset.id}" version="1"/>
-          <node id="-2" lon="0.1" lat="0.1" changeset="#{changeset.id}" version="1"/>
-          <node id="-3" lon="0.2" lat="0.2" changeset="#{changeset.id}" version="1"/>
-          <relation id="#{relation.id}" changeset="#{changeset.id}" version="1">
-           <member type="node" role="foo" ref="-1"/>
-           <member type="node" role="foo" ref="-2"/>
-           <member type="node" role="foo" ref="-3"/>
-           <member type="way" role="bar" ref="-1"/>
-          </relation>
-         </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff, :headers => auth_header
-      assert_response :bad_request,
-                      "shouldn't be able to use invalid placeholder IDs"
-      assert_equal "Placeholder Way not found for reference -1 in relation #{relation.id}.", @response.body
-    end
-
     ##
     # test what happens if a diff is uploaded containing only a node
     # move.
@@ -1683,40 +1417,6 @@ module Api
       assert_response :not_found, "Relation should not be deleted"
     end
 
-    def test_upload_relation_placeholder_not_fix
-      changeset = create(:changeset)
-
-      auth_header = bearer_authorization_header changeset.user
-
-      # modify node
-      diff = <<~CHANGESET
-        <osmChange version='0.6'>
-          <create>
-            <relation id='-2' version='0' changeset='#{changeset.id}'>
-              <member type='relation' role='' ref='-4' />
-              <tag k='type' v='route' />
-              <tag k='name' v='AtoB' />
-            </relation>
-            <relation id='-3' version='0' changeset='#{changeset.id}'>
-              <tag k='type' v='route' />
-              <tag k='name' v='BtoA' />
-            </relation>
-            <relation id='-4' version='0' changeset='#{changeset.id}'>
-              <member type='relation' role='' ref='-2' />
-              <member type='relation' role='' ref='-3' />
-              <tag k='type' v='route_master' />
-              <tag k='name' v='master' />
-            </relation>
-          </create>
-        </osmChange>
-      CHANGESET
-
-      # upload it
-      post api_changeset_upload_path(changeset), :params => diff.to_s, :headers => auth_header
-      assert_response :bad_request, "shouldn't be able to use reference -4 in relation -2: #{@response.body}"
-      assert_equal "Placeholder Relation not found for reference -4 in relation -2.", @response.body
-    end
-
     def test_upload_multiple_delete_block
       changeset = create(:changeset)
 
-- 
2.39.5