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

Using unsupported database features leads to incomplete schema.rb files without error or warning #36359

Closed
searls opened this issue May 29, 2019 · 8 comments
Labels

Comments

@searls
Copy link
Contributor

searls commented May 29, 2019

Steps to reproduce

When running migrations and using the default schema.rb dump strategy, if a table cannot be dumped, migrations will not raise an error or log a warning, but rather leave a comment.

For example, given this migration:

class AddTableWithEnum < ActiveRecord::Migration[5.2]
  def change
    execute "create type conditions as enum ('sunny', 'rainy')"

    create_table :afternoons do |t|
      t.column :weather, :conditions, null: false, default: "sunny"
    end
  end
end

Running migrations will result in this schema.rb file:

ActiveRecord::Schema.define(version: 2019_05_29_142121) do

  # These are extensions that must be enabled in order to support this database
  enable_extension "plpgsql"

# Could not dump table "afternoons" because of following StandardError
#   Unknown type 'conditions' for column 'weather'

end

Here's a repro-repo demonstrating the issue: https://github.com/searls/rails-repro-schema-squelch

Here's the specific commit of interest.

Expected behavior

I expected Rails to raise an error in this case, and prompt the user to set config.active_record.schema_format = :sql, explaining that they need to switch to structure.sql to have a reproducible schema dump.

As it stands, the only moment someone will discover their dump is incomplete is later—potentially much later—when running something like rake db:test:prepare or rake db:schema:load on a new environment that depends on the schema being complete. Worse, the error they see (from a table not existing) may take users a while to trace back to an incomplete schema, since (in my experience) relatively few Rails developers have a firm understanding of when and how db/schema.rb is used by Rails.

Actual behavior

As described above, the actual behavior is:

$ bin/rake db:migrate
== 20190529142121 AddTableWithEnum: migrating =================================
-- execute("create type conditions as enum ('sunny', 'rainy')")
   -> 0.0040s
-- create_table(:afternoons)
   -> 0.0102s
== 20190529142121 AddTableWithEnum: migrated (0.0143s) ========================

$ echo $?
0

System configuration

Rails version: 5.2.3

Ruby version: ruby 2.6.1p33 (2019-01-30 revision 66950) [x86_64-darwin18]

@itsWill
Copy link
Contributor

itsWill commented May 29, 2019

Oh wow, agreed this is a nasty gotcha.

This is really old behaviour introduced in 87535f5

I wonder how many people are relying on this? Otherwise it feels like we can fix this by removing the rescue and raising with the desired error and message, which seems like the better option to me.

@searls
Copy link
Contributor Author

searls commented May 29, 2019

I wonder how many people are relying on this?

I suspect zero people are wittingly relying on this, but that a very large number of projects will experience an error on very old migrations when running db:migrate. Introducing an error to a thing that's "worked" for years is likely going to lead to a lot of attention and search traffic.

Fortunately, the solution won't be "change your old migrations", since forward-compatibility of migrations is critical. Rather the message can contain the solution: just change the dump format in your application config.

As far as timing, this seems like the perfect kind of thing to include in a major version bump. Not a functional change, just one that'll break a bunch of builds but offer a <1 minute fix.

@itsWill
Copy link
Contributor

itsWill commented May 29, 2019

I suspect zero people are wittingly relying on this

👍 agreed, I meant along the lines of old migrations as you pointed out.

As for the timing I think it makes sense to loop in @rafaelfranca who's the release manager of rails and see his thoughts on including it.

@itsWill
Copy link
Contributor

itsWill commented May 30, 2019

I talked to @rafaelfranca and we were all of the same mind that this makes sense to put in for rails 6. I'll put up a PR for this shortly.

itsWill pushed a commit to itsWill/rails that referenced this issue Jun 24, 2019
Previoulsy the schema dumper when it encountered a failure it would
rescue the exception and leave a comment on the schema.rb. Now we raise
an exception and let the developer be aware that the schema was not
dumped sucessfuly. If an error is raised the `db/schema.rb` file remains
the same, we only modify it if the dump was successful.

Fixes: rails#36359
@rails-bot
Copy link

rails-bot bot commented Aug 28, 2019

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-0-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Aug 28, 2019
@rails-bot rails-bot bot closed this as completed Sep 4, 2019
@kurt-mueller-osumc
Copy link

kurt-mueller-osumc commented Jun 5, 2020

I can confirm that this is still an issue, even on rails 6, and that no exception was raised - only a comment is made in schema.rb.

@robocopkaka
Copy link

Can also confirm that this still happens on 6.0.2.2. Took a while to figure out why tests were failing

@tdelam
Copy link

tdelam commented Mar 30, 2021

Can also confirm that this still happens in 6.0.3 as well. Only a comment made in schema.rb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants