rails / globalid

Identify app models with a URI
MIT License
1.22k stars 124 forks source link

SignedGlobalID: Consider re-adding the ability to use the old serialization format #171

Open intrip opened 1 year ago

intrip commented 1 year ago

https://github.com/rails/globalid/commit/12f76297c803c658ea2fc684d4336db347a90901 removed the ability to keep using the old format when serializing via SignedGlobalID. This causes the following issues:

I understand that "Rollbacks" can be considered a "no-issue" since the upgrade is one-way but I'm still afraid of the rolling-deployment issue. Additionally, Version "1.2.1" is the new version pointed by Rails "7.1", which makes rolling back a Rails upgrade more complicated because it forces you to upgrade GlobalID before upgrading Rails in order to not encounter the "unexpected" rollback issue.

#!/usr/bin/env ruby

require "bundler/inline"

globalid_version = ARGV[0]
$sgid = ARGV[1]

# true = install gems so this is fast on repeat invocations
gemfile(true, quiet: true) do
  source "https://rubygems.org"

  gem "globalid", globalid_version
  gem "activerecord", "~> 7.1"
  gem "sqlite3"
end

require "active_record"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

ActiveRecord::Schema.define do
  create_table "people" do |t|
    t.string "name"
  end
end
class Person < ActiveRecord::Base; end

SignedGlobalID.verifier = ActiveSupport::MessageVerifier.new("secret")
person = Person.create!(name: "John Doe")

if $sgid
  require "minitest/autorun"
  class SignedGlobalIdTest < Minitest::Test
    def test_cant_locate_new_format_sgid_with_old_version
      assert GlobalID::Locator.locate_signed($sgid), "Can't locate by SGID"
    end
  end
else
  puts SignedGlobalID.create(person, app: "test")
end
jacopo-37s-mb 3.3.0-preview2 ~ ./signed_globalid_serializarion_issue.rb 1.2.0 | tail -n 1 | xargs ./signed_globalid_serializarion_issue.rb 1.2.0
-- create_table("people")
   -> 0.0081s
Run options: --seed 25563

# Running:

.

Finished in 0.014660s, 68.2128 runs/s, 68.2128 assertions/s.

1 runs, 1 assertions, 0 failures, 0 errors, 0 skips
jacopo-37s-mb 3.3.0-preview2 ~ ./signed_globalid_serializarion_issue.rb 1.2.0 | tail -n 1 | xargs ./signed_globalid_serializarion_issue.rb 1.1.0
-- create_table("people")
   -> 0.0093s
Run options: --seed 3475

# Running:

F

Finished in 0.000923s, 1083.4238 runs/s, 1083.4238 assertions/s.

  1) Failure:
SignedGlobalIdTest#test_cant_locate_new_format_sgid_with_old_version [./signed_globalid_serializarion_issue.rb:36]:
Can't locate by SGID

1 runs, 1 assertions, 1 failures, 0 errors, 0 skips
jacopo-37s-mb 3.3.0-preview2 ~
intrip commented 12 months ago

Let me know your take on this, I'll be happy to create a PR to re-add the ability to use the old format.

rwc9u commented 2 weeks ago

This is affecting us as well, because the signed global ids are created in workers and passed along as part of a link. The links don't expire for awhile. If we upgrade the gem, the links can no longer be parsed.

To expand on this a little more, our case was an edge case where we use bootboot for upgrading rails. In this case the DEPENDENCIES_NEXT Gemfile_next.lock had version 1.2.1 and the default had 1.0.0 set. We upgraded workers first, the urls were created with the new signed global id format. Our web processes were still running on the older version of globalid, and didn't know how to handle the new format. If we had updated both web and workers at the same time we wouldn't have had an issue.