From fe5a25522cbbc237ae86806367162353283c24ea Mon Sep 17 00:00:00 2001 From: vignesh-chaturvedi Date: Thu, 15 Oct 2026 05:20:12 +0000 Subject: [PATCH] Don't save trace tags until the trace is saved Assigning a new set of tags to an existing trace saved them immediately, so an invalid tag raised ActiveRecord::RecordNotSaved instead of failing validation. The API returned a 500 error and the edit form showed an error page. Build the new tags instead so they are validated and saved along with the trace, and update the trace through the API in a transaction so the old tags are kept if the update fails. --- app/controllers/api/traces_controller.rb | 6 ++-- app/models/trace.rb | 32 +++++++++---------- .../controllers/api/traces_controller_test.rb | 17 ++++++++++ test/controllers/traces_controller_test.rb | 11 ++++++- test/models/trace_test.rb | 16 ++++++++++ 5 files changed, 63 insertions(+), 19 deletions(-) diff --git a/app/controllers/api/traces_controller.rb b/app/controllers/api/traces_controller.rb index a0892ed1c..f61dfbf15 100644 --- a/app/controllers/api/traces_controller.rb +++ b/app/controllers/api/traces_controller.rb @@ -48,8 +48,10 @@ module Api trace = Trace.visible.find(params.expect(:id)) if trace.user == current_user - trace.update_from_xml(request.raw_post) - trace.save! + Trace.transaction do + trace.update_from_xml(request.raw_post) + trace.save! + end head :ok else diff --git a/app/models/trace.rb b/app/models/trace.rb index 216eb3735..55efe175c 100644 --- a/app/models/trace.rb +++ b/app/models/trace.rb @@ -87,20 +87,18 @@ class Trace < ApplicationRecord end def tagstring=(s) - self.tags = if s.include? "," - s.split(",").map(&:strip).reject(&:empty?).collect do |tag| - tt = Tracetag.new - tt.tag = tag - tt - end - else - # do as before for backwards compatibility: - s.split.collect do |tag| - tt = Tracetag.new - tt.tag = tag - tt - end - end + names = if s.include? "," + s.split(",").map(&:strip).reject(&:empty?) + else + # do as before for backwards compatibility: + s.split + end + + tags.clear + + names.each do |name| + tags.build(:tag => name) + end end def file=(attachable) @@ -190,8 +188,10 @@ class Trace < ApplicationRecord self.description = description.content - self.tags = pt.find("tag").collect do |tag| - Tracetag.new(:tag => tag.content) + tags.clear + + pt.find("tag").each do |tag| + tags.build(:tag => tag.content) end end diff --git a/test/controllers/api/traces_controller_test.rb b/test/controllers/api/traces_controller_test.rb index 71ce695f6..946208818 100644 --- a/test/controllers/api/traces_controller_test.rb +++ b/test/controllers/api/traces_controller_test.rb @@ -264,6 +264,23 @@ module Api assert_equal trace.tagstring, updated.tagstring end + # Check that updating a trace with an invalid tag is rejected + def test_update_invalid_tags + tracetag = create(:tracetag) + trace = tracetag.trace + auth_header = bearer_authorization_header trace.user + + updated = Trace.find(trace.id) + updated.tags.build(:tag => "invalid/tag") + + put api_trace_path(trace), :params => create_trace_xml(updated), :headers => auth_header + assert_response :bad_request + assert_match "tags: is invalid", @response.body + + # Ensure the existing tags have not been changed + assert_equal [tracetag.tag], trace.reload.tags.collect(&:tag) + end + # Check deleting a trace through the api def test_destroy public_trace_file = create(:trace, :visibility => "identifiable") diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb index afc57e945..402169f50 100644 --- a/test/controllers/traces_controller_test.rb +++ b/test/controllers/traces_controller_test.rb @@ -568,13 +568,22 @@ class TracesControllerTest < ActionDispatch::IntegrationTest # Test invalid updates def test_update_invalid - trace = create(:trace) + tracetag = create(:tracetag) + trace = tracetag.trace # Invalid visibility session_for(trace.user) put trace_path(trace, :trace => { :description => "Changed description", :tagstring => "new_tag", :visibility => "wrong" }) assert_response :success assert_select "title", :text => /^Editing Trace/ + + # Invalid tag + put trace_path(trace, :trace => { :description => "Changed description", :tagstring => "invalid/tag", :visibility => "trackable" }) + assert_response :success + assert_select "title", :text => /^Editing Trace/ + + # Ensure the existing tags have not been changed + assert_equal [tracetag.tag], trace.reload.tags.collect(&:tag) end # Test destroying a trace diff --git a/test/models/trace_test.rb b/test/models/trace_test.rb index 59fb62bf4..b694a23db 100644 --- a/test/models/trace_test.rb +++ b/test/models/trace_test.rb @@ -140,6 +140,22 @@ class TraceTest < ActiveSupport::TestCase assert_equal "zero, one, two", trace.tagstring end + def test_tagstring_invalid_tag + trace = create(:trace) + create(:tracetag, :trace => trace, :tag => "old") + + assert_not trace.update(:tagstring => "invalid/tag, valid") + assert_equal %w[old], trace.reload.tags.collect(&:tag) + end + + def test_update_from_xml_invalid_tag + trace = create(:trace) + + trace.update_from_xml("Testinvalid/tag") + assert_predicate trace.tags.first, :new_record? + assert_not trace.save + end + def test_public? assert_predicate build(:trace, :visibility => "public"), :public? assert_not_predicate build(:trace, :visibility => "private"), :public? -- 2.47.3