Introduce versions formatter for the schema dumper by fatkodima · Pull Request #53797 · rails/rails

fatkodima · 2024-12-01T19:14:53Z

Previously, the versions were sorted in decreasing order and new added to the top of the list. Within large-enough teams, this can cause many merge conflicts near the top of this list. Within my company, we have lots of developers pushing lots of schema changes and half of the time someone merges a PR, lots of existing PRs now have conflicts with the master.

Now, the versions are sorted by their reverse representations (20241201183002 -> 20038110214202), in decreasing (can be increasing, does not matter, but to be consistent with the previous implementation) order. This will greatly reduce the likelihood of merge conflicts as now new versions can be inserted at any place in the existing list. I did a simple calculation and while before we had a likelihood of conflict within a list of, say 5 top elements (20%), now - within a whole list of 600+ items (0.1%).

I also added a comment with reversed version representation for each version within dumped structure.sql file for people to more easily solve the merge conflicts. But we can remove this and add a comment at the top of the INSERT describing the algorithm.

For existing apps, there will be a large diff the next time structure.sql is generated.

ghiculescu · 2024-12-01T21:39:31Z

I love this idea!

IMO the comment is a bit confusing unless you already know about the algorithm.

fatkodima · 2024-12-01T23:02:50Z

Agree, changed to print only a comment at the top of the list.

byroot · 2024-12-02T08:36:07Z

I'm not too sure how I feel about that.

I'm sympathetic to the desire of reducing conflicts, but it feels weird to have these versions ordered in a seemingly arbitrary fashion. I'll try to seek a second opinion.

fatkodima · 2024-12-02T10:00:16Z

Another alternative is the ability for users to provide a custom proc, which will be used for sorting these versions.

jjb · 2024-12-02T14:10:54Z

activerecord/test/cases/schema_dumper_test.rb


    schema_info = ActiveRecord::Base.lease_connection.dump_schema_information
    expected = <<~STR
+    -- Versions are sorted by reverse representations of numbers (abc -> cba) in descending order.


Suggested change

-- Versions are sorted by reverse representations of numbers (abc -> cba) in descending order.

-- Versions are sorted by reverse representations of numbers (20241225000000 -> 00000052214202) in descending order.

eileencodes · 2024-12-02T16:14:19Z

I'm sympathetic to the problem but I really don't like this solution. Reversing the strings is just confusing and I can imagine both beginners and seasoned Rails devs getting stuck on this. If we merge this we'll get endless bug reports and complaints.

fatkodima · 2024-12-02T19:00:47Z

Wdyt about my alternative idea

Another alternative is the ability for users to provide a custom proc, which will be used for sorting these versions.

so people can be able to configure their custom logic? And we preserve the original implementation by default.

ghiculescu · 2024-12-02T23:40:00Z

If we merge this we'll get endless bug reports and complaints.

I'm skeptical. I am going to monkey patch this into our codebase, and see if anyone (~50 devs) notices or asks.

ghiculescu · 2024-12-09T03:26:06Z

^ A week later nobody has asked about it (as far as I'm aware) and we're definitely seeing less merge conflicts.

ghiculescu · 2024-12-12T00:48:42Z

byroot · 2024-12-12T08:20:38Z

This was the reactions when the order was reversed: #44363

fatkodima · 2024-12-12T09:51:20Z

If this PR is implemented in alternative way, for example, providing users a proc to configure, for example config.active_record.structure_versions_sort = ->(a, b) { b <=> a }, we can preserve the current behaviour, for not happy people from the linked PR to use ->(a, b) { a <=> b } to return to the previous behavior, and for the proposal in this PR to use the behaviour we want ->(a, b) { b.to_s.reverse <=> a.to_s.reverse }.

byroot · 2024-12-12T10:02:55Z

If this PR is implemented in alternative way, for example, providing users a proc to configure,

@eileencodes @matthewd thoughts? I'm not against it, but it's yet another super obscure config.

byroot · 2024-12-19T09:25:40Z

for example, providing users a proc to configure, for example config.active_record.structure_versions_sort = ->(a, b) { b <=> a }

I can be convinced to offer some way to customize this, but I wonder if rather than just exposing a sort proc, we might not as well refactor insert_versions_sql out so it's implemented by a class that you can substitue, e.g (untested pseudo-code).

def insert_versions_sql
  self.class.schema_version_inserter.generate(pool.schema_migration)
end

Then you get the flexibility to order however you want, but also include some comments, and even fetch the list of versions differently.

fatkodima · 2024-12-20T00:57:01Z

Updated with the suggestion for a separate class.

chaadow · 2024-12-27T13:08:46Z

guides/source/configuring.md

+to change the default behavior:
+
+```ruby
+class CustomSchemaVersionsFormatter


I've just tried this snippet and it's missing

private attr_reader :connection def initialize(connection) @connection = connection end

cc @fatkodima

oops, #54054

rails-bot bot added the activerecord label Dec 1, 2024

fatkodima force-pushed the change-structure-dump-versions-ordering branch from 15dfdcb to 081880b Compare December 1, 2024 23:01

jjb reviewed Dec 2, 2024

View reviewed changes

fatkodima force-pushed the change-structure-dump-versions-ordering branch from 081880b to 6c18dc7 Compare December 20, 2024 00:55

rails-bot bot added the docs label Dec 20, 2024

fatkodima changed the title ~~Change the order of INSERT statements in structure.sql dumps~~ Introduce versions formatter for the schema dumper Dec 20, 2024

fatkodima force-pushed the change-structure-dump-versions-ordering branch from 6c18dc7 to 7bdd0df Compare December 20, 2024 01:02

Introduce versions formatter for the schema dumper

0cc1842

fatkodima force-pushed the change-structure-dump-versions-ordering branch from 7bdd0df to 0cc1842 Compare December 20, 2024 01:10

byroot merged commit 0bded31 into rails:main Dec 27, 2024
1 check passed

chaadow reviewed Dec 27, 2024

View reviewed changes

fatkodima mentioned this pull request Dec 27, 2024

Fix config.active_record.schema_versions_formatter example [skip ci] #54054

Merged

fatkodima deleted the change-structure-dump-versions-ordering branch December 27, 2024 13:18

jduff mentioned this pull request Jan 3, 2025

Sort table columns by name when dumping schema #53281

Merged

4 tasks

rwojsznis mentioned this pull request Mar 2, 2025

Add sql_filesystem_versions schema format #54670

Closed

4 tasks

Sebmono mentioned this pull request Jan 19, 2026

[PR] Sort table columns by name when dumping schema Sandgarden-Demo/rails#106

Closed

4 tasks

	-- Versions are sorted by reverse representations of numbers (abc -> cba) in descending order.
	-- Versions are sorted by reverse representations of numbers (20241225000000 -> 00000052214202) in descending order.

Comments

Conversation

fatkodima commented Dec 1, 2024

Uh oh!

ghiculescu commented Dec 1, 2024

Uh oh!

fatkodima commented Dec 1, 2024

Uh oh!

byroot commented Dec 2, 2024

Uh oh!

fatkodima commented Dec 2, 2024

Uh oh!

jjb Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

eileencodes commented Dec 2, 2024

Uh oh!

fatkodima commented Dec 2, 2024

Uh oh!

ghiculescu commented Dec 2, 2024

Uh oh!

ghiculescu commented Dec 9, 2024

Uh oh!

ghiculescu commented Dec 12, 2024

Uh oh!

byroot commented Dec 12, 2024

Uh oh!

fatkodima commented Dec 12, 2024

Uh oh!

byroot commented Dec 12, 2024

Uh oh!

byroot commented Dec 19, 2024

Uh oh!

fatkodima commented Dec 20, 2024

Uh oh!

Uh oh!

chaadow Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

chaadow Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

fatkodima Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants