@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/2475434...@github.com>_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
