[openstreetmap/openstreetmap-website] Adds note tags support (PR #5344)

[Tom Hughes]

@tomhughes requested changes on this pull request.



> @@ -9,6 +9,13 @@ SET xmloption = content;
 SET client_min_messages = warning;
 SET row_security = off;
 
+--
+-- Name: public; Type: SCHEMA; Schema: -; Owner: -
+--
+
+-- *not* creating schema, since initdb creates it
+
+

This block is noise created by the version of `pg_dump` you're using and has nothing to do with the actual changes in this PR so should be removed.

> @@ -0,0 +1,13 @@
+class CreateNoteTags < ActiveRecord::Migration[7.2]
+  def change
+    # Create a table and primary key
+    create_table :note_tags, :primary_key => [:note_id, :k] do |t|
+      t.column "note_id", :bigint, :null => false
+      t.column "k",  :string, :default => "", :null => false
+      t.column "v",  :string, :default => "", :null => false
+    end
+
+    # Add foreign key without validation
+    add_foreign_key :note_tags, :notes, :column => :note_id, :name => "note_tags_id_fkey", :validate => false

Would moving this inside the `create_table` as `t.foreign_key` avoid the need to create it with validation disabled? Not sure if the strong migrations is clever enough to know that is safe?

If not then you can just add a second migration to enable validation as it's safe to do on an empty table.

> @@ -50,7 +50,8 @@ OSM.NewNote = function (map) {
       data: {
         lat: location.lat,
         lon: location.lng,
-        text: $(form.text).val()
+        text: $(form.text).val(),
+        tags: "created_by:OpenStreetMaps-Website"

There is no `s` on the end of OpenStreetMap ;-) I'm not sure what is the best value for the tag here but it definitely shouldn't have the trailing s.

> @@ -83,12 +85,30 @@ def create
       lat = OSM.parse_float(params[:lat], OSM::APIBadUserInput, "lat was not a number")
       comment = params[:text]
 
+      # Extract the tags parameter (if it exists)
+      tags = []
+      if params[:tags].present?
+        # Split by commas to get individual key-value pairs
+        pairs = params[:tags].split(",")
+
+        # For each pair in parameters, store it in tags variable
+        pairs.each do |pair|
+          key, value = pair.split(":", 2)
+          tags << { :k => sanitize(key), :v => sanitize(value) } if key && value
+        end
+      end

Encoding all the tags in a string like this is nasty and creates all sorts of problems if people use colons or commas in tag names or values - colons in particular are historically common in tag names for other object types.

I'm not sure what the solution is as this API is unusual in using form parameters rather than an XML or JSON body to create an object.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5344#pullrequestreview-2456745482
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/5344/review/2456745482 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20241124/1a5b698e/attachment-0001.htm>