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

[Emin Kocan]

@kcne commented on this pull request.



> @@ -14,6 +14,10 @@ xml.note("lon" => note.lon, "lat" => note.lat) do
 
   xml.date_closed note.closed_at if note.closed?
 
+  note.tags.each do |k, v|

This code is duplicated multiple times, consider creating a shared helper method to reuse instead.

> +  validates :note, :associated => true
+  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true
+  validates :k, :uniqueness => { :scope => :note_id }

To ensure stronger data integrity, we can enforce that k and v are always present, within length limits, and unique per note. Additionally, using `presence: true` for note simplifies and improves performance compared to `associated: true`

```ruby
validates :note, presence: true
validates :k, presence: true, length: { maximum: 255 }, uniqueness: { scope: :note_id }
validates :v, presence: true, length: { maximum: 255 }
```

> +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
+
+      t.foreign_key :notes, :column => :note_id, :name => "note_tags_id_fkey"
+    end
+  end
+end

This migration works well for ensuring uniqueness with a composite primary key. However, if querying by tags (e.g., `{k: "created_by", v: "openstreetmap"}`) is expected, a surrogate primary key (`id`) with additional indexes might be more efficient. For example:

```ruby
create_table :note_tags do |t|
  t.bigint :note_id, null: false
  t.string :k, null: false, default: ""
  t.string :v, null: false, default: ""

  t.timestamps
end

add_index :note_tags, [:note_id, :k], unique: true       # Enforce uniqueness
add_index :note_tags, [:k, :v]                          # Optimize tag-based queries
```

This approach aligns with Rails conventions, simplifies queries, and allows adding indexes for tag lookups (`k`/`v`).

> @@ -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

I agree with Tom here. Also independent of implementation, validation here would be necessary in my opinion.

 Two alternative approaches that could be useful here is to:
- use nested form parameters:
```
tags[created_by]=OpenStreetMap-Website&tags[editor]=JOSM
```
- using a prefix for tag related properties:
```
tag_created_by=OpenStreetMap-Website&tag_editor=JOSM
```

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

Message ID: <openstreetmap/openstreetmap-website/pull/5344/review/2475434346 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20241203/41fe5d17/attachment.htm>