<p>I can confirm this. The problem is around the building a <a href="https://github.com/openstreetmap/openstreetmap-website/blob/9ed1fe2d781d50d2fb76124b488958f43b4c2f4e/app/controllers/trace_controller.rb#L266">new_trace from the supplied xml</a>. The code for this sets the <a href="https://github.com/openstreetmap/openstreetmap-website/blob/9ed1fe2d781d50d2fb76124b488958f43b4c2f4e/app/models/trace.rb#L197">id of the temporary trace to be the same as the existing trace</a>, and then <a href="https://github.com/openstreetmap/openstreetmap-website/blob/9ed1fe2d781d50d2fb76124b488958f43b4c2f4e/app/models/trace.rb#L212-L214">builds the tags</a>. But the first call to trace.tags.build causes a load of the existing tags, before adding all the tags again from the xml.</p>
<p>To replicate in the rails console, using an existing trace:</p>
<pre><code>irb(main):061:0> t
=> #<Trace id: 2, user_id: 1, visible: true, name: "trace 1", size: nil, latitude: nil, longitude: nil, timestamp: "2017-08-09 10:29:39", description: "2014-02-15 1001 עדינות צוקים__20140215_1001.gpx", inserted: false, visibility: "private">

irb(main):062:0> t.tags
  Tracetag Load (0.8ms)  SELECT "gpx_file_tags".* FROM "gpx_file_tags" WHERE "gpx_file_tags"."gpx_id" = $1  [["gpx_id", 2]]
=> #<ActiveRecord::Associations::CollectionProxy [#<Tracetag gpx_id: 2, tag: "foo", id: 8>]>

irb(main):063:0> new = Trace.new
=> #<Trace id: nil, user_id: nil, visible: true, name: "", size: nil, latitude: nil, longitude: nil, timestamp: nil, description: "", inserted: nil, visibility: "public">

irb(main):064:0> new.id = 2
=> 2

irb(main):065:0> new.tags
  Tracetag Load (0.8ms)  SELECT "gpx_file_tags".* FROM "gpx_file_tags" WHERE "gpx_file_tags"."gpx_id" = $1  [["gpx_id", 2]]
=> #<ActiveRecord::Associations::CollectionProxy [#<Tracetag gpx_id: 2, tag: "foo", id: 8>]>
</code></pre>
<p>So even though we haven't added any tags to <code>new</code> yet, it picks up the existing tags from <code>t</code>.</p>
<p>It seems a bit dangerous creating these trace objects from XML and setting the id of the temporary object to match an existing db record. We need to consider if there's a better approach.</p>
<p>For example, when updating a rails object normally, you load the existing one and update the attributes from the form data (e.g. <code>@diary_entry.update_attributes(entry_params)</code>), then run validations on the result. We don't normally create a temporary DiaryEntry object. The way the trace works currently is quite different, where we build a temporary Trace object, run validations as we parse the XML, and then copy the attributes over from the temporary object to the real one. I'm not sure if this approach has any benefits.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/issues/1600#issuecomment-321226714">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABWnLU_nD10GbrE1X8_4dMwhFpatZ2CFks5sWZW-gaJpZM4Oo-aV">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABWnLUlrshlWu9s3bTiuolZ-eVwgq-sZks5sWZW-gaJpZM4Oo-aV.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
  <link itemprop="url" href="https://github.com/openstreetmap/openstreetmap-website/issues/1600#issuecomment-321226714"></link>
  <meta itemprop="name" content="View Issue"></meta>
</div>
<meta itemprop="description" content="View this Issue on GitHub"></meta>
</div>

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/openstreetmap/openstreetmap-website","title":"openstreetmap/openstreetmap-website","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/openstreetmap/openstreetmap-website"}},"updates":{"snippets":[{"icon":"PERSON","message":"@gravitystorm in #1600: I can confirm this. The problem is around the building a [new_trace from the supplied xml](https://github.com/openstreetmap/openstreetmap-website/blob/9ed1fe2d781d50d2fb76124b488958f43b4c2f4e/app/controllers/trace_controller.rb#L266). The code for this sets the [id of the temporary trace to be the same as the existing trace](https://github.com/openstreetmap/openstreetmap-website/blob/9ed1fe2d781d50d2fb76124b488958f43b4c2f4e/app/models/trace.rb#L197), and then [builds the tags](https://github.com/openstreetmap/openstreetmap-website/blob/9ed1fe2d781d50d2fb76124b488958f43b4c2f4e/app/models/trace.rb#L212-L214). But the first call to trace.tags.build causes a load of the existing tags, before adding all the tags again from the xml.\r\n\r\nTo replicate in the rails console, using an existing trace:\r\n\r\n```\r\nirb(main):061:0\u003e t\r\n=\u003e #\u003cTrace id: 2, user_id: 1, visible: true, name: \"trace 1\", size: nil, latitude: nil, longitude: nil, timestamp: \"2017-08-09 10:29:39\", description: \"2014-02-15 1001 עדינות צוקים__20140215_1001.gpx\", inserted: false, visibility: \"private\"\u003e\r\n\r\nirb(main):062:0\u003e t.tags\r\n  Tracetag Load (0.8ms)  SELECT \"gpx_file_tags\".* FROM \"gpx_file_tags\" WHERE \"gpx_file_tags\".\"gpx_id\" = $1  [[\"gpx_id\", 2]]\r\n=\u003e #\u003cActiveRecord::Associations::CollectionProxy [#\u003cTracetag gpx_id: 2, tag: \"foo\", id: 8\u003e]\u003e\r\n\r\nirb(main):063:0\u003e new = Trace.new\r\n=\u003e #\u003cTrace id: nil, user_id: nil, visible: true, name: \"\", size: nil, latitude: nil, longitude: nil, timestamp: nil, description: \"\", inserted: nil, visibility: \"public\"\u003e\r\n\r\nirb(main):064:0\u003e new.id = 2\r\n=\u003e 2\r\n\r\nirb(main):065:0\u003e new.tags\r\n  Tracetag Load (0.8ms)  SELECT \"gpx_file_tags\".* FROM \"gpx_file_tags\" WHERE \"gpx_file_tags\".\"gpx_id\" = $1  [[\"gpx_id\", 2]]\r\n=\u003e #\u003cActiveRecord::Associations::CollectionProxy [#\u003cTracetag gpx_id: 2, tag: \"foo\", id: 8\u003e]\u003e\r\n```\r\nSo even though we haven't added any tags to `new` yet, it picks up the existing tags from `t`.\r\n\r\n\r\nIt seems a bit dangerous creating these trace objects from XML and setting the id of the temporary object to match an existing db record. We need to consider if there's a better approach.\r\n\r\nFor example, when updating a rails object normally, you load the existing one and update the attributes from the form data (e.g. `@diary_entry.update_attributes(entry_params)`), then run validations on the result. We don't normally create a temporary DiaryEntry object. The way the trace works currently is quite different, where we build a temporary Trace object, run validations as we parse the XML, and then copy the attributes over from the temporary object to the real one. I'm not sure if this approach has any benefits.\r\n"}],"action":{"name":"View Issue","url":"https://github.com/openstreetmap/openstreetmap-website/issues/1600#issuecomment-321226714"}}}</script>