[openstreetmap/openstreetmap-website] PUT on gpx_file in API does not remove old tags (#1600)

[Andy Allan]

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.

To replicate in the rails console, using an existing trace:

```
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>]>
```
So even though we haven't added any tags to `new` yet, it picks up the existing tags from `t`.


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.

For 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.


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/issues/1600#issuecomment-321226714
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20170809/7e80930d/attachment.html>