Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compact migrations into a single one #5030

Open
kennyadsl opened this issue Apr 19, 2023 · 5 comments
Open

Compact migrations into a single one #5030

kennyadsl opened this issue Apr 19, 2023 · 5 comments
Labels
good first issue Wanted, but lacking time to work on it and might be a good fit for a first contribution type:enhancement Proposed or newly added feature

Comments

@kennyadsl
Copy link
Member

Similarly to what we have done with https://github.com/solidusio/solidus/blob/5111949aef306ab0ad2c0b9b61af2e1e26fd2f0d/core/db/migrate/20160101010000_solidus_one_four.rb, we can compact the majority of our migrations into a single one.

This will reduce the number of migrations we copy over the host application.

@kennyadsl kennyadsl added the type:enhancement Proposed or newly added feature label Apr 19, 2023
@kennyadsl
Copy link
Member Author

After some investigations, I don't think we are there yet. The main blocker is that we can't compact all migrations in a single one easily because they are executed with different versions of Rails (some with Rails 4.2, some with 5.0, some with 5.1, etc).

If we compact all of them in a single migration, we need to specify with which Rails version that single migration will be executed and at that point, we will potentially have stores that executed the same migrations with different versions, not being anymore able to give for granted the shape of the stores' databases.

Eg:

  • old stores executed the old migrations with mixed versions (4.2, 5.0, 5.1, etc)
  • new stores will execute the single migration with a single version, let's say 5.1

I have two possible solutions in mind for this:

  1. Create multiple migrations divided by version. This is doable but there are a couple of problems. First of all, the migrations don't have a consecutive pattern in terms of the version used. For example, the first one is executed with Rails 5.0, and the second one with 4.2. This means that we should first check if the first one is 4.2 compatible, and only at that point we can merge them together. The second problem is that, at the moment, I couldn't find a tool that splits migrations into multiple ones. The only tool I found (squasher) merges the migrations from a specific time up to the first one.
  2. Align the majority of migrations to a specific Rails version and do the merge later: We could create a new migration that changes the database to have the same output as if we run the migrations with a specific Rails version (eg. Rails 7). At that point, we could split all the migration from the first one to that "alignment" migration safely, using the Rails version of choice as our executor.

I'm personally more in favor of option 2, it will also improve everyone's database, bringing the new Rails migrations feature and solving the problem also presented here.

@kennyadsl kennyadsl mentioned this issue Apr 20, 2023
3 tasks
@waiting-for-dev
Copy link
Contributor

Thanks for the investigation and the complete report, @kennyadsl

not being anymore able to give for granted the shape of the stores' databases.

To understand, are we expecting any substantial difference that can make a difference at the application layer, or it's just an unknown at this point?

@kennyadsl
Copy link
Member Author

To understand, are we expecting any substantial difference that can make a difference at the application layer, or it's just an unknown at this point?

To be honest, I don't know yet. There are differences, which are visible in the Rails Migration Compatibility class as pointed out by @adammathys here, but maybe we have no impact at the application layer. Also, even if we don't have today, a different shape of the database might bring some impact later, as we change it and the application code.

One quick experiment we could do is to change all migrations to a specific target version and compare the schema. Maybe there are just a few differences that we can cover with a small migration.

@kennyadsl
Copy link
Member Author

I made the test above, and these are the differences running all migrations with the Rails 7 executor:

diff --git a/db/schema.rb b/db/schema.rb
index 26615df..8c1e7bd 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -137,7 +137,7 @@
     t.string "attachment_content_type"
     t.string "attachment_file_name"
     t.string "type", limit: 75
-    t.datetime "attachment_updated_at", precision: nil
+    t.datetime "attachment_updated_at"
     t.text "alt"
     t.datetime "created_at"
     t.datetime "updated_at"
@@ -163,7 +163,7 @@
     t.integer "address_id"
     t.integer "shipping_method_id"
     t.string "tracking"
-    t.datetime "shipped_at", precision: nil
+    t.datetime "shipped_at"
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer "imported_from_shipment_id"
@@ -310,7 +310,7 @@
     t.string "state"
     t.decimal "adjustment_total", precision: 10, scale: 2, default: "0.0", null: false
     t.integer "user_id"
-    t.datetime "completed_at", precision: nil
+    t.datetime "completed_at"
     t.integer "bill_address_id"
     t.integer "ship_address_id"
     t.decimal "payment_total", precision: 10, scale: 2, default: "0.0"
@@ -330,10 +330,10 @@
     t.decimal "included_tax_total", precision: 10, scale: 2, default: "0.0", null: false
     t.integer "item_count", default: 0
     t.integer "approver_id"
-    t.datetime "approved_at", precision: nil
+    t.datetime "approved_at"
     t.boolean "confirmation_delivered", default: false
     t.string "guest_token"
-    t.datetime "canceled_at", precision: nil
+    t.datetime "canceled_at"
     t.integer "canceler_id"
     t.integer "store_id"
     t.string "approver_name"
@@ -372,7 +372,7 @@
     t.string "name"
     t.text "description"
     t.boolean "active", default: true
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.datetime "created_at"
     t.datetime "updated_at"
     t.boolean "auto_capture"
@@ -417,7 +417,7 @@
     t.integer "variant_id", null: false
     t.decimal "amount", precision: 10, scale: 2, null: false
     t.string "currency"
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.datetime "created_at"
     t.datetime "updated_at"
     t.string "country_iso", limit: 2
@@ -460,8 +460,8 @@
   create_table "spree_products", force: :cascade do |t|
     t.string "name", default: "", null: false
     t.text "description"
-    t.datetime "available_on", precision: nil
-    t.datetime "deleted_at", precision: nil
+    t.datetime "available_on"
+    t.datetime "deleted_at"
     t.string "slug"
     t.text "meta_description"
     t.string "meta_keywords"
@@ -471,7 +471,7 @@
     t.datetime "updated_at"
     t.boolean "promotionable", default: true
     t.string "meta_title"
-    t.datetime "discontinue_on", precision: nil
+    t.datetime "discontinue_on"
     t.index ["available_on"], name: "index_spree_products_on_available_on"
     t.index ["deleted_at"], name: "index_spree_products_on_deleted_at"
     t.index ["name"], name: "index_spree_products_on_name"
@@ -503,7 +503,7 @@
     t.integer "promotion_id"
     t.integer "position"
     t.string "type"
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.text "preferences"
     t.datetime "created_at"
     t.datetime "updated_at"
@@ -567,8 +567,8 @@
   create_table "spree_promotion_rules_stores", force: :cascade do |t|
     t.integer "store_id", null: false
     t.integer "promotion_rule_id", null: false
-    t.datetime "created_at", precision: nil, null: false
-    t.datetime "updated_at", precision: nil, null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
     t.index ["promotion_rule_id"], name: "index_spree_promotion_rules_stores_on_promotion_rule_id"
     t.index ["store_id"], name: "index_spree_promotion_rules_stores_on_store_id"
   end
@@ -584,8 +584,8 @@

   create_table "spree_promotions", force: :cascade do |t|
     t.string "description"
-    t.datetime "expires_at", precision: nil
-    t.datetime "starts_at", precision: nil
+    t.datetime "expires_at"
+    t.datetime "starts_at"
     t.string "name"
     t.string "type"
     t.integer "usage_limit"
@@ -751,7 +751,7 @@
     t.string "tracking"
     t.string "number"
     t.decimal "cost", precision: 10, scale: 2, default: "0.0"
-    t.datetime "shipped_at", precision: nil
+    t.datetime "shipped_at"
     t.integer "order_id"
     t.integer "deprecated_address_id"
     t.string "state"
@@ -801,7 +801,7 @@

   create_table "spree_shipping_methods", force: :cascade do |t|
     t.string "name"
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.datetime "created_at"
     t.datetime "updated_at"
     t.string "tracking_url"
@@ -865,7 +865,7 @@
     t.datetime "created_at"
     t.datetime "updated_at"
     t.boolean "backorderable", default: false
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.index ["deleted_at"], name: "index_spree_stock_items_on_deleted_at"
     t.index ["stock_location_id", "variant_id"], name: "stock_item_by_loc_and_var_id"
     t.index ["stock_location_id"], name: "index_spree_stock_items_on_stock_location_id"
@@ -921,7 +921,7 @@
     t.decimal "amount", precision: 8, scale: 2
     t.decimal "user_total_amount", precision: 8, scale: 2, default: "0.0", null: false
     t.string "authorization_code", null: false
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.string "originator_type"
     t.integer "originator_id"
     t.datetime "created_at"
@@ -935,8 +935,8 @@
   create_table "spree_store_credit_reasons", force: :cascade do |t|
     t.string "name"
     t.boolean "active", default: true
-    t.datetime "created_at", precision: nil, null: false
-    t.datetime "updated_at", precision: nil, null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
   end

   create_table "spree_store_credit_types", force: :cascade do |t|
@@ -956,11 +956,11 @@
     t.decimal "amount_authorized", precision: 8, scale: 2, default: "0.0", null: false
     t.string "currency"
     t.text "memo"
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer "type_id"
-    t.datetime "invalidated_at", precision: nil
+    t.datetime "invalidated_at"
     t.index ["deleted_at"], name: "index_spree_store_credits_on_deleted_at"
     t.index ["type_id"], name: "index_spree_store_credits_on_type_id"
     t.index ["user_id"], name: "index_spree_store_credits_on_user_id"
@@ -1007,7 +1007,7 @@
     t.string "name"
     t.string "description"
     t.boolean "is_default", default: false
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.datetime "created_at"
     t.datetime "updated_at"
     t.string "tax_code"
@@ -1028,9 +1028,9 @@
     t.datetime "updated_at"
     t.string "name"
     t.boolean "show_rate_in_label", default: true
-    t.datetime "deleted_at", precision: nil
-    t.datetime "starts_at", precision: nil
-    t.datetime "expires_at", precision: nil
+    t.datetime "deleted_at"
+    t.datetime "starts_at"
+    t.datetime "expires_at"
     t.integer "level", default: 0, null: false
     t.index ["deleted_at"], name: "index_spree_tax_rates_on_deleted_at"
     t.index ["zone_id"], name: "index_spree_tax_rates_on_zone_id"
@@ -1055,7 +1055,7 @@
     t.string "icon_file_name"
     t.string "icon_content_type"
     t.integer "icon_file_size"
-    t.datetime "icon_updated_at", precision: nil
+    t.datetime "icon_updated_at"
     t.text "description"
     t.datetime "created_at"
     t.datetime "updated_at"
@@ -1112,9 +1112,9 @@
     t.string "perishable_token"
     t.integer "login_count", default: 0, null: false
     t.integer "failed_login_count", default: 0, null: false
-    t.datetime "last_request_at", precision: nil
-    t.datetime "current_login_at", precision: nil
-    t.datetime "last_login_at", precision: nil
+    t.datetime "last_request_at"
+    t.datetime "current_login_at"
+    t.datetime "last_login_at"
     t.string "current_login_ip"
     t.string "last_login_ip"
     t.string "login"
@@ -1158,7 +1158,7 @@
     t.decimal "height", precision: 8, scale: 2
     t.decimal "width", precision: 8, scale: 2
     t.decimal "depth", precision: 8, scale: 2
-    t.datetime "deleted_at", precision: nil
+    t.datetime "deleted_at"
     t.boolean "is_master", default: false
     t.integer "product_id"
     t.decimal "cost_price", precision: 10, scale: 2
@@ -1184,6 +1184,7 @@
     t.boolean "default", default: false, null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.index ["payment_source_type", "payment_source_id"], name: "index_spree_wallet_payment_sources_on_payment_source"
     t.index ["user_id", "payment_source_id", "payment_source_type"], name: "index_spree_wallet_payment_sources_on_source_and_user", unique: true
     t.index ["user_id"], name: "index_spree_wallet_payment_sources_on_user_id"
   end

@kennyadsl kennyadsl added the good first issue Wanted, but lacking time to work on it and might be a good fit for a first contribution label May 25, 2023
@kennyadsl
Copy link
Member Author

The steps to complete this issues are:

  1. create a migration that makes the database identical if we run it with the current versions specified in each migration file or if we run it with the latest rails version. To get the differences, it's possible to compare the schema in a fresh application, destroy the database, change all the versions to 7 (or above), regenerate the database and compare.
  2. release the migration that aligns the database in a new Solidus version.
  3. compact old migrations in the next major.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Wanted, but lacking time to work on it and might be a good fit for a first contribution type:enhancement Proposed or newly added feature
Projects
None yet
Development

No branches or pull requests

2 participants