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

Introduce a context for rendering fixtures ERB. #13022

Merged
merged 1 commit into from Dec 3, 2013

Conversation

@pwnall
Copy link
Contributor

@pwnall pwnall commented Nov 24, 2013

I ran into this issue while trying to use binary data in my fixtures. The following guides motivated me to write this patch.

http://realityforge.org/code/rails/2006/04/06/loading-binary-data-into-rails-fixtures.html
http://www.tamingthemindmonkey.com/2011/08/18/rails-binary-data-in-fixtures
These define a new method right in the fixture file. I'm sure other people do this every once in a while too. With the current code, method definitions leak to other fixtures via the main object. This introduces subtle test inder-dependencies. Methods won't leak after this patch. (proven by new test case)

http://stackoverflow.com/questions/12644057/how-to-use-binary-data-in-rails-fixtures
This is a nice approach that's amenable to being packaged into a gem, except there is no clean place for adding this sort of functionality.

I look forward to your feedback.

@senny
Copy link
Member

@senny senny commented Nov 26, 2013

Could this break applications that rely on the fact that currently methods are leaked?

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Nov 26, 2013

It depends on how you define "break".

This only impacts fixture code, so it won't break the folks who don't have tests at all.

Assuming you have tests, and you run them when you upgrade Rails, this will turn flaky tests into definite failures.

I think the fixtures load order is currently decided by this call to Dir.[]. That method has no guarantee for the order in which it returns directory entries. One would hope it will return sorted entries, but in my tests this was not the case. StackOverflow agrees with me.

For some restricted definition of "break", this is a breaking change. I am willing to write the documentation needed for devs who run into this to understand what's happening. At the same time, I consider that the change turns ticking bombs into problems that are immediately visible.

For context, imagine this exploding all of a sudden in a continuous integration / continuous deployment environment. 💣 😢

@senny
Copy link
Member

@senny senny commented Nov 27, 2013

of course by breaking I meant the tests and not production code. I don't see a reasonable way to issue depreciation warnings only in these cases so we need to compensate with docs.

/cc @rafaelfranca

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Nov 27, 2013

@senny I figured you already knew the answer, and were asking for documentation purposes, or to have the answer spelled out in the PR log. Sorry, I misunderstood and my answer was weird :/

I didn't see a 4_1_release_notes.md in rails/guides/source. What is a good place to document the change?

@senny
Copy link
Member

@senny senny commented Nov 28, 2013

@pwnall sorry for being vague. It's good to reflect these thoughts in the PR so we can later link to it.

I'll prepare the release notes and the upgrading guide today. I'll let you know when things are ready.
In the meantime you could polish the rdocs. To make sure people find render_context we should add a section to the main fixture documentation.

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Nov 30, 2013

@senny I added a Changelog entry and a paragraph in the fixtures documentation. Can I do anything else to help document this?

@senny
Copy link
Member

@senny senny commented Nov 30, 2013

You can add a section to the upgrading ruby on rails guide

I pushed a first draft of the 4.1 release notes. You can add an entry in the notable changes section and even link to the upgrading guide.

@senny
senny reviewed Dec 1, 2013
View changes
activerecord/CHANGELOG.md Outdated
main object, and potentially leading to unexpected dependencies between
tests. Now each fixture is evaluated in the context of unique
`ActiveRecord::FixtureSet::RenderContext` subclass, so method definitions
are not shared between fixtures. Helper methods inteded to be used by

This comment has been minimized.

@senny

senny Dec 1, 2013
Member

typo inteded => intended

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

Fixed.

@senny
senny reviewed Dec 1, 2013
View changes
activerecord/CHANGELOG.md Outdated
`ActiveRecord::FixtureSet::RenderContext` subclass, so method definitions
are not shared between fixtures. Helper methods inteded to be used by
multiple fixtures can be defined on
`ActiveRecord::FixtureSet::RenderContext`, in an initializer.

This comment has been minimized.

@senny

senny Dec 1, 2013
Member

is an initializer the right place? Genereally they should be used for application specific stuff. This is strictly for tests. Could we define them in test_helper?

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

You're right. Thank you!

@senny
senny reviewed Dec 1, 2013
View changes
activerecord/lib/active_record/fixture_set/render_context.rb Outdated
# The context used by the ERB fixture renderer.
#
# Make helper functions available in your fixtures by defining them in a
# module and including it in ActiveRecord::FixtureSet::RenderContext.

This comment has been minimized.

@senny

senny Dec 1, 2013
Member

can you add a simple example?

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

My example is not too difficult, but it's non-trivial, because I didn't want to come across as advocating to move all logic in fixture helpers.

I can make it simpler or more complex, please let me know what you think!

This comment has been minimized.

@senny

senny Dec 2, 2013
Member

I think the example is spot on 👍 . It's short, shows a real world use-case and not too complex.

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 2, 2013

@senny Thank you! I added notes in the upgrade guide and 4.1 release notes. I look forward to your feedback!

@senny
senny reviewed Dec 2, 2013
View changes
activerecord/CHANGELOG.md Outdated
`ActiveRecord::FixtureSet::RenderContext` subclass, so method definitions
are not shared between fixtures. Helper methods intended to be used by
multiple fixtures can be defined on
`ActiveRecord::FixtureSet::RenderContext`, in `test_helper.rb`.

This comment has been minimized.

@senny

senny Dec 2, 2013
Member

Let's use the short version from the release notes here as well. If people want to find out more then RenderContext and the upgrade guide is the place to look:

  • The ERB in fixture files is no longer evaluated in the context of the main object. Helper methods used by multiple fixtures should be defined on the ActiveRecord::FixtureSet::RenderContext class.

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

Done. Thank you!

@senny
senny reviewed Dec 2, 2013
View changes
activerecord/test/cases/fixture_set/file_test.rb Outdated
end
end

def test_render_context_scope

This comment has been minimized.

@senny

senny Dec 2, 2013
Member

Does this test confirm that RenderContext does not have FixtureSet and ActiveRecord in it's lookup path? Just by looking at it and reading the title it's not that obvious.

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

I tried to make the test more explicit. What do you think?

@senny
Copy link
Member

@senny senny commented Dec 2, 2013

I added some minor comments. This is looking good.

@rafaelfranca
rafaelfranca reviewed Dec 2, 2013
View changes
activerecord/lib/active_record/fixture_set/render_context.rb Outdated
# A new subclass of this class is created each time the ERB renderer is
# called so that methods defined in ERB templates do not leak into other
# templates' context.
class ActiveRecord::FixtureSet::RenderContext

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 2, 2013
Member

This construction is very confusing. It only work because users will reopen a framework class to inject behavior. Could not we simplify this and make users create their own context and configure the fixture to use it? Or even better we can expose the context class and let people inject their own modules there. Something like:

Fixtures.context.include MyHelpers

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

@rafaelfranca You can currently do

ActiveRecord::FixtureSet::RenderContext.include MyHelpers

I think we can get to where you want by

class ActiveRecord::FixtureSet
  def self.context
    ActiveRecord::FixtureSet::RenderContext
  end
end

If I read this right, Fixtures is deprecated, so what you wrote would work and output a deprecation warning. The line below is slightly longer, but would work as is.

FixtureSet.context.include MyHelpers

What do you think?

@senny
Copy link
Member

@senny senny commented Dec 2, 2013

I discussed this change with @rafaelfranca. We came to the conclusion that we should:

  1. use an anonymous class as the context. This will get rid of the lookup hack and the render_context.rb file.
class Fixture
  def self.context
    @context ||= Class.new do
      def get_binding
        binding()
      end
    end
  end
end
  1. We should expose a simple API for third party code to provide helpers for that context. ActiveRecord::FixtureSet.context will return the anonymous context class. Third party code can add helpers like so: ActiveRecord::FixtureSet.context.send :include, Paperclip::FixtureHelpers
  2. The docs should only talk about ActiveRecord::FixtureSet.context
  3. We still create a subclass of the context to isolate each file.
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 2, 2013

👍

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 2, 2013

This doesn't quite work :(

I tried adding this to fixtures.rb

class ActiveRecord::FixtureSet
  def self.context
    @context ||= Class.new do
      def get_binding
        binding()
      end
    end
  end
end

One problem is that FixtureSet is now in the lookup scope of the binding, so using File.read in a helper will actually break, because File will get resolved as ActiveRecord::FixtureSet::File.

The other problem is that unless I define get_binding on the class is that is used as a context, test_independent_render_contexts fails, implying that methods are actually getting defined on the shared superclass.

Thoughts?

@senny
Copy link
Member

@senny senny commented Dec 2, 2013

We can define get_binding in the created subclass.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 2, 2013

About the File problem it is more about class clash than lookup scope. I have the feeling that ActiveRecord::FixtureSet::File should not be called File

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 2, 2013

@rafaelfranca In the end, we'll have a namespace where we have to pay a lot of attention when we add new names. I think that ActiveRecord::FixtureSet is less suitable than a namespace that is specifically designed for this purpose, like ActiveRecord::FixtureSet::RenderContext.

@senny I think that is correct. I also think it has to be defined in a "clean" namespace, because that namespace will be in lookup scope.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 2, 2013

I'm fine with a clean namespace but I don't want to see that module hack. It is ugly and hard to understand.

So if there is a way to make a clean namespace without it 👍

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 2, 2013

@rafaelfranca What do you think about this version?

ActiveRecord::FixtureSet::RenderContext does not show up in the hierarchy of the ERB evaluation context, and it's just a private namespace that is an implementation detail.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 2, 2013

Very good ❤️

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 2, 2013

@rafaelfranca The other alternative I have is along the lines of

ActiveRecord::FixtureSet::File.define_singleton_method(:render_context) do
  Class.new ActiveRecord::FixtureSet.context do
    def get_binding
      binding()
    end
  end
end
@rafaelfranca
rafaelfranca reviewed Dec 2, 2013
View changes
activerecord/lib/active_record/fixtures.rb Outdated
# Digest::SHA2.hexdigest(File.read(Rails.root.join('test/fixtures', path)))
# end
# end
# ActiveRecord::FixtureSet.context.include FixtureFileHelpers

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 2, 2013
Member

We will have to use send here since include is not public on Ruby 2.0

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

Good point! Thank you!

@rafaelfranca
rafaelfranca reviewed Dec 2, 2013
View changes
activerecord/lib/active_record/fixtures.rb Outdated
@@ -529,6 +546,11 @@ def self.identify(label)
Zlib.crc32(label.to_s) % MAX_ID
end

# Superclass for the evaluation contexts used by ERB fixtures.
def self.context

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 2, 2013
Member

Maybe we could call this context_class to make explicit it is a class. WDYT?

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

If you're fine with the extra verbosity, I'll make the change.

This comment has been minimized.

@senny

senny Dec 3, 2013
Member

@pwnall looks like you renamed all occurrences of context to context_class but the method and the ivar is still called context.

@rafaelfranca
rafaelfranca reviewed Dec 2, 2013
View changes
guides/source/4_1_release_notes.md Outdated
@@ -348,6 +348,10 @@ for detailed changes.
ActiveRecord will now translate aliased attribute names to the actual column
name used in the database. ([Pull Request](https://github.com/rails/rails/pull/7839))

* The ERB in fixture files is no longer evaluated in the context of the main
object. Helper methods used by multiple fixtures should be defined on the
`ActiveRecord::FixtureSet::RenderContext` class. ([Pull Request](https://github.com/rails/rails/pull/13022))

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 2, 2013
Member

We should update this

This comment has been minimized.

@pwnall

pwnall Dec 2, 2013
Author Contributor

Done.

@rafaelfranca
rafaelfranca reviewed Dec 2, 2013
View changes
guides/source/upgrading_ruby_on_rails.md Outdated
`test_helper.rb`.

```ruby
class ActiveRecord::FixtureSet::RenderContext

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 2, 2013
Member

This too

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 2, 2013

@rafaelfranca Thank you! I'll go through the docs and update everything.

@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 2, 2013

@rafaelfranca I updated the docs and commit message.

Fixture files are passed through an ERB renderer before being read as
YAML. The rendering is currently done in the context of the main object,
so method definitons leak into other fixtures, and there is no clean
place to define fixture helpers.

After this commit, the ERB renderer will use a new subclass of
ActiveRecord::FixtureSet.context_class each time a fixture is rendered.
@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 3, 2013

@senny Sorry, and thank you for catching that! I used git grep to make sure I didn't miss any other occurrence.

@senny
Copy link
Member

@senny senny commented Dec 3, 2013

@pwnall 👍 I think we are ready to go. Thank you very much for all the experimentation and updates, the speedy replies and the contribution. ❤️

senny added a commit that referenced this pull request Dec 3, 2013
Introduce a context for rendering fixtures ERB.
@senny senny merged commit b6f189e into rails:master Dec 3, 2013
@pwnall
Copy link
Contributor Author

@pwnall pwnall commented Dec 3, 2013

@senny Thank you and @rafaelfranca for the patience and guidance!

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 5, 2013

Great we could make this in time to 4.1, thank you so much for the work

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.