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

Migration fails and more problems due to method_missing #15

Closed
DrTom opened this issue Apr 19, 2013 · 12 comments
Closed

Migration fails and more problems due to method_missing #15

DrTom opened this issue Apr 19, 2013 · 12 comments

Comments

@DrTom
Copy link

DrTom commented Apr 19, 2013

We had a several problems using textacular, i.e. migrations would fail. It turned out to be the implementation of method_missing in textacular.

I forked the project and simply removed method_missing which fixes the issues for us:
https://github.com/DrTom/textacular

It is up to you how to fix the problem but I kindly suggest not to overwrite method_missing as the troubles that come with it are often not worth the benefits.

@benhamill
Copy link
Contributor

The method_missing implementation is there to support dynamic search method names. Those kind of method names are falling out of favor, I think, in Rails, which I believe to be the inspiration for them in the first place. I don't want to just rip them out, however, without discussing among the other maintainers (I, personally, have never found them to be much use).

However, I did just change method_missing a bit in master, and I wonder if you could see if it fixed this issue for you? Or if you could help me reproduce, I could test it myself.

Also: Sorry for the long response time.

@DrTom
Copy link
Author

DrTom commented Jun 12, 2013

Yes, I am aware that the dynamic finders that method_missing are falling out of favor. Imho named arguments turn out to be more readable than find_by_this_and_than_and_y_and_z. Without the trouble that overwriting method missing brings along.

At any rate. I tried the current commit of textacular on github and our project still fails when running the migrations with it. I created a branch from the failing commit if you want too look in to it: https://github.com/zhdk/madek/tree/textacular
The config/database_developer_example.yml shows an example how we connect. As mentioned, the failing task is rake db:migrate.

@benhamill
Copy link
Contributor

Hmm. I checked it out and had a look at the stack trace. It didn't look like textacular was specifically implicated:

PG::Error: ERROR:  current transaction is aborted, commands ignored until end of transaction block
: CREATE TABLE "zencoder_jobs" ("id" uuid NOT NULL, "media_file_id" integer NOT NULL, "zencoder_id" integer, "comment" text, "state" character varying(255) DEFAULT 'initialized' NOT NULL, "error" text, "notification" text, "request" text, "response" text, "created_at" timestamp NOT NULL, "updated_at" timestamp NOT NULL) /home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rack-mini-profiler-0.1.26/Ruby/lib/patches/sql_patches.rb:155:in `exec'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rack-mini-profiler-0.1.26/Ruby/lib/patches/sql_patches.rb:155:in `async_exec'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/connection_adapters/postgresql_adapter.rb:650:in `block in execute'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract_adapter.rb:280:in `block in log'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activesupport-3.2.13/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract_adapter.rb:275:in `log'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/connection_adapters/postgresql_adapter.rb:649:in `execute'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract/schema_statements.rb:170:in `create_table'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/foreigner-1.4.1/lib/foreigner/connection_adapters/abstract/schema_statements.rb:14:in `create_table'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:466:in `block in method_missing'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:438:in `block in say_with_time'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/2.0.0/benchmark.rb:281:in `measure'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:438:in `say_with_time'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:458:in `method_missing'
/home/ben/src/madek/db/migrate/20130205144924_create_zencoder_jobs.rb:16:in `up'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:410:in `block (2 levels) in migrate'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/2.0.0/benchmark.rb:281:in `measure'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:410:in `block in migrate'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract/connection_pool.rb:129:in `with_connection'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:389:in `migrate'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:528:in `migrate'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:720:in `block (2 levels) in migrate'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:775:in `call'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:775:in `block in ddl_transaction'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract/database_statements.rb:192:in `transaction'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/transactions.rb:208:in `transaction'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:775:in `ddl_transaction'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:719:in `block in migrate'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:700:in `each'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:700:in `migrate'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:570:in `up'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/migration.rb:551:in `migrate'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/activerecord-3.2.13/lib/active_record/railties/databases.rake:193:in `block (2 levels) in <top (required)>'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/task.rb:246:in `call'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/task.rb:246:in `block in execute'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/task.rb:241:in `each'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/task.rb:241:in `execute'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/task.rb:184:in `block in invoke_with_call_chain'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/2.0.0/monitor.rb:211:in `mon_synchronize'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/task.rb:177:in `invoke_with_call_chain'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/task.rb:170:in `invoke'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:143:in `invoke_task'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:101:in `block (2 levels) in top_level'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:101:in `each'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:101:in `block in top_level'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:110:in `run_with_threads'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:95:in `top_level'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:73:in `block in run'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:160:in `standard_exception_handling'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/lib/rake/application.rb:70:in `run'
/home/ben/.rbenv/versions/2.0.0-p195/lib/ruby/gems/2.0.0/gems/rake-10.0.4/bin/rake:33:in `<top (required)>'
./bin/rake:16:in `load'
./bin/rake:16:in `<main>'
Tasks: TOP => db:migrate

However, if removing this gem fixes it, then that's telling. I'm not sure how to go about figuring out how to fix it, so I'll open a new Issue specifically to discuss removing the dynamic method feature. Obviously, that would require a major version bump, too, so we're not looking at the next release for this.

@gregmolnar
Copy link
Member

@DrTom I cloned that repo and ran the migrations with the master of textacular without any issue.
I was on the wrong branch. Testing it again.

@benhamill
Copy link
Contributor

Oh. @DrTom: What version of Ruby are you using? That might matter, too.

@gregmolnar
Copy link
Member

It is failing for me too. I use the same patchlevel of ruby 2 as you. It is a very weird error. I will try to find where it is originated from.

@gregmolnar
Copy link
Member

I tried to get to the bottom of this but no luck so far. It brakes at the override of respond_to?

@gregmolnar
Copy link
Member

I might be wrong but at this line https://github.com/textacular/textacular/blob/master/lib/textacular.rb#L56 should we check if it is an ActiveRecord::Base instance and return super if not? If change the code like that than the migrations are not breaking and the dynamic search methods are working too. Although the spec for this is broken but I guess it is an issue with the test.
This what I would put in that line instead:

return super if self != ActiveRecord::Base

@benhamill do you think this would be a good fix? I can commit the changes if you think it is ok.

@benhamill
Copy link
Contributor

That abstract_class? call went in 2 days ago in this commit. And this issue predates it...

Ignoring the test for the moment, if the class in question is ActiveRecord::Base, then we don't want to check if it's a dynamic helper method, because AR::Base doesn't have columns out of which to build helpers (or, to bring the test back in, any other abstract class).

But I feel like I might just be misunderstanding something about what you're saying.

@gregmolnar
Copy link
Member

You are right it was late and I haven't think it through. Oddly enough if I do the change above to textacular in DrTom's app the migrations are working and even the dynamic helpers. Not sure how :) .
So I guess there id another gem which overwrites respond_to? in that app which conflicts with textacular. I will have free time tomorrow and I will investigate this further.

@DrTom
Copy link
Author

DrTom commented Jun 14, 2013

respond_to? is overwritten at least 55 times from various gems which madek uses (I searched with def\s*respond_to\? but there are more subtle ways to overwrite things in ruby, so I might have missed some).

I eliminated a few suspicious gems but the migration still fails. So, we know that there is some unfortunate interaction between textacular and some other gem that overwrites respond_to? as well.

I will use my patched version of textacular without the overwritten method_missing and respond_to? for the time being. I will prepare Madek for Rails 4 in the next month and also remove some further gems I want to get rid of anyways. I can test the original version of textacular after the refactoring again.

@benhamill
Copy link
Contributor

I'm gonna go ahead and close this. I just merged #16, which deprecates the dynamic helpers. In the next major version bump, we'll rip 'em out.

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

No branches or pull requests

3 participants