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

Remove safe_yaml. #2397

Merged
merged 2 commits into from
Jun 7, 2016
Merged

Remove safe_yaml. #2397

merged 2 commits into from
Jun 7, 2016

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Sep 4, 2015

I don't think it's really this gem's job, but besides that, I was getting a failure
when starting zeus, so I just made it optional.

The below stacktrace is from a fork of v0.6.7 with a commit on top of it:

https://github.com/swipesense/rails_admin/commits/0.6.7-eager_loading

Here's the stacktrace:

$ zeus rake

     ~/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/psych/parser.rb:33:in `<class:Parser>': superclass mismatch for class Mark (TypeError)
from ~/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/psych/parser.rb:32:in `<module:Psych>'
from ~/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/psych/parser.rb:1:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/psych.rb:7:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/safe_yaml-1.0.4/lib/safe_yaml/psych_handler.rb:1:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/safe_yaml-1.0.4/lib/safe_yaml/load.rb:130:in `<module:SafeYAML>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/safe_yaml-1.0.4/lib/safe_yaml/load.rb:26:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/safe_yaml-1.0.4/lib/safe_yaml.rb:1:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/bundler/gems/rails_admin-975bb5075824/lib/rails_admin/engine.rb:10:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/bundler/gems/rails_admin-975bb5075824/lib/rails_admin.rb:1:in `<top (required)>'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:76:in `require'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:76:in `block (2 levels) in require'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:72:in `each'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:72:in `block in require'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:61:in `each'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:61:in `require'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler.rb:134:in `require'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus/rails.rb:97:in `default_bundle'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:200:in `run_action'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:74:in `block (2 levels) in boot_steps'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus/load_tracking.rb:7:in `features_loaded_by'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:73:in `block in boot_steps'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:56:in `catch'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:56:in `boot_steps'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:48:in `go'
from -e:1:in `<main>'

@bf4 bf4 force-pushed the remove_safe_yaml branch from d0b0411 to e78eaf3 Compare October 21, 2015 16:28
@bf4
Copy link
Contributor Author

bf4 commented Oct 21, 2015

Rebased against current master

end
end
else
warn "Safe-loading of YAML is not available. Please install 'safe_yaml' or install Psych 2.0+"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An error should be raised here, not warning. We shouldn't let users choose unsafe state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change

B mobile phone

On Nov 3, 2015, at 3:08 AM, Mitsuhiro Shibuya [email protected] wrote:

In lib/rails_admin/engine.rb:

module RailsAdmin

  • Backwards-compatible RailsAdmin::SafeYAML when SafeYAML isn't available

  • begin
  • require 'safe_yaml/load'
  • rescue LoadError
  • if YAML.respond_to?(:safe_load)
  •  SafeYAML = Module.new do
    
  •    def self.load(yaml)
    
  •      YAML.safe_load(yaml)
    
  •    end
    
  •  end
    
  • else
  •  warn "Safe-loading of YAML is not available. Please install 'safe_yaml' or install Psych 2.0+"
    
    An error should be raised here, not warning. We shouldn't let users choose unsafe state.


Reply to this email directly or view it on GitHub.

@bf4 bf4 force-pushed the remove_safe_yaml branch from e78eaf3 to 16ffd04 Compare November 3, 2015 18:40
rescue LoadError
unless YAML.respond_to?(:safe_yaml)
raise LoadError, "Safe-loading of YAML is not available. Please install 'safe_yaml' or install Psych 2.0+"
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guard clause raises same exception but with better message when there's no fallback

@bbenezech
Copy link
Collaborator

@bf4 can we merge this?

@bf4 bf4 force-pushed the remove_safe_yaml branch 2 times, most recently from 07c9f0b to 830e99b Compare June 6, 2016 17:08
bf4 and others added 2 commits June 6, 2016 12:08
I don't think it's really this gem's job, but besides that, I was getting a failure
when starting zeus, so I just made it optional.

This one a fork of v0.6.7 with a commit on top of it:

https://github.com/swipesense/rails_admin/commits/0.6.7-eager_loading

Here's the stacktrace:

$ zeus rake

     ~/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/psych/parser.rb:33:in `<class:Parser>': superclass mismatch for class Mark (TypeError)
from ~/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/psych/parser.rb:32:in `<module:Psych>'
from ~/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/psych/parser.rb:1:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/psych.rb:7:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/safe_yaml-1.0.4/lib/safe_yaml/psych_handler.rb:1:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/safe_yaml-1.0.4/lib/safe_yaml/load.rb:130:in `<module:SafeYAML>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/safe_yaml-1.0.4/lib/safe_yaml/load.rb:26:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/safe_yaml-1.0.4/lib/safe_yaml.rb:1:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/bundler/gems/rails_admin-975bb5075824/lib/rails_admin/engine.rb:10:in `<top (required)>'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `block in require'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:240:in `load_dependency'
from ~/projects/rails_server/bundle/ruby/2.2.0/gems/activesupport-4.2.3/lib/active_support/dependencies.rb:274:in `require'
from ~/projects/rails_server/bundle/ruby/2.2.0/bundler/gems/rails_admin-975bb5075824/lib/rails_admin.rb:1:in `<top (required)>'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:76:in `require'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:76:in `block (2 levels) in require'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:72:in `each'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:72:in `block in require'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:61:in `each'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler/runtime.rb:61:in `require'
from ~/.rvm/gems/ruby-2.2.2/gems/bundler-1.10.6/lib/bundler.rb:134:in `require'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus/rails.rb:97:in `default_bundle'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:200:in `run_action'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:74:in `block (2 levels) in boot_steps'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus/load_tracking.rb:7:in `features_loaded_by'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:73:in `block in boot_steps'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:56:in `catch'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:56:in `boot_steps'
from ~/.rvm/gems/ruby-2.2.2/gems/zeus-0.15.4/lib/zeus.rb:48:in `go'
from -e:1:in `<main>'
Failure running today June 6, 2016 at 10:47 am CST

```
1) RailsAdmin::Adapters::ActiveRecord #build_statement supports datetime type query
   Failure/Error: expect(abstract_model.send(:filter_scope, scope,  'datetime_field' => {'1' => {v: ['', 'February 01, 2012 00:00', 'March 01, 2012 00:00'], o: 'between'}}).where_values).to eq(scope.where(['(field_tests.datetime_field BETWEEN ? AND ?)', Time.local(2012, 2, 1), Time.local(2012, 3, 1).end_of_day]).where_values)

       expected: ["(field_tests.datetime_field BETWEEN '2012-02-01 06:00:00.000000' AND '2012-03-02 05:59:59.999999')"]
            got: ["(field_tests.datetime_field BETWEEN '2012-01-31 06:00:00.000000' AND '2012-03-01 05:59:59.999999')"]

2) RailsAdmin::AbstractModel filters on datetimes with :en locale lists elements within outbound limits
   Failure/Error: expect(@abstract_model.all(filters: {'datetime_field' => {'1' => {v: ['', 'January 03, 2012 00:00', ''], o: 'between'}}}).count).to eq(2)

        expected: 2
             got: 3
```
@bf4 bf4 force-pushed the remove_safe_yaml branch from 830e99b to 5c14d48 Compare June 6, 2016 17:08
@bf4
Copy link
Contributor Author

bf4 commented Jun 7, 2016

@bbenezech I think it can be merged, and is actually better since you last asked the question.

require 'safe_yaml/load'
def self.yaml_load(yaml)
SafeYAML.load(yaml)
end
Copy link
Contributor Author

@bf4 bf4 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define self.yaml_load to use SafeYAML.load in non-monkey-patching mode, when available

@bbenezech bbenezech merged commit 9819122 into railsadminteam:master Jun 7, 2016
@bbenezech
Copy link
Collaborator

Thanks a lot. It's good to see safe_yaml go. (with psych 2 at least)

@bf4
Copy link
Contributor Author

bf4 commented Jun 8, 2016

Awesome!

@bf4 bf4 deleted the remove_safe_yaml branch June 8, 2016 03:14
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

Successfully merging this pull request may close these issues.

3 participants