Issue with schema dump #2621

Merged
merged 4 commits into from Mar 27, 2012

Projects

None yet

9 participants

@icco

I'll admit, I'm not 100% sure why this fix works, but stream.respond_to?(:external_encoding) was returning true even when stream.external_encoding was nil. The scenario where it would get triggered was when using active record alone with other frameworks, such as Padrino.

@brianmario

👍

@dasch

There may have been a reason for using respond_to?, e.g. that the method may not always be available on the stream object. Have you tested this on Ruby 1.8 and 1.9?

It's probably best to keep the check in place:

if stream.respond_to?(:external_encoding) && stream.external_encoding
  # ...
end
@icco

I didn't test on 1.8, in hindsight, that seems like an intelligent idea. I'll go install 1.8, make the suggested change and retest and get back to you on this. Thanks for the quick reply.

@minikomi

Ah.. thanks for this. Might be a good idea to post to the Padrino mailing list or issues, just so people are aware.

@icco

Alright, assuming I'm doing the tests right, this works in 1.8.7 and 1.9.2

@dasch

Looks good to me!

@knightlabs

Seconded: resolves issue in AR 3.1.0 using AR in a standalone, non-Rails environment.

Thanks!

@dmathieu dmathieu and 1 other commented on an outdated diff Sep 22, 2011
activerecord/lib/active_record/schema_dumper.rb
@@ -40,7 +40,7 @@ module ActiveRecord
def header(stream)
define_params = @version ? ":version => #{@version}" : ""
- if stream.respond_to?(:external_encoding)
+ if stream.respond_to?(:external_encoding) && !stream.external_encoding.nil?
@dmathieu
dmathieu Sep 22, 2011

Doing if stream.respond_to?(:external_encoding) && stream.internal_encoding would be slightely faster.
That's micro-optimization, but there's been several like this in activerecord, which participated in making it much more faster.

@icco
icco Sep 22, 2011

Wow, seriously? That kind of blows my mind. I assumed .nil? was a relatively free operation. I guess I'll make the change.

@icco
icco Sep 22, 2011

Also, question, why are you using .internal_encoding versus .external_encoding?

@dmathieu
dmathieu Sep 22, 2011

My bad, typo in the comment.

@icco
icco Sep 22, 2011

Ahh, alright, I'm not crazy. That's a good sign :p

@dmathieu

Don't misunderstand me. I haven't said "a lot faster". I said slightly.

@icco

My apologies for putting words in your mouth. I tend to use adjectives that blow things out of proportion.

@bascht bascht added a commit to bascht/api that referenced this pull request Dec 3, 2011
@bascht bascht - work around active_record encoding issue (rails/rails#2621) e56562c
@mxswd

+1 to pull this. Just ran into this problem today. No reason to not pull this...

@D1plo1d

+1 to fixing this. It is a constant problem for me with Padrino.

@tenderlove tenderlove merged commit 41dfc46 into rails:master Mar 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment