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

Fix export crash for models with JSON field #3056

Closed

Conversation

blaze182
Copy link
Contributor

@blaze182 blaze182 commented Aug 18, 2018

Hi everyone,

I experienced a crash when was exporting a model with a JSON field:

NoMethodError:
undefined method `content_tag' for nil:NilClass'
# ./lib/rails_admin/config/fields/types/json.rb:17:in `block in
<class:Json>'

Failing lines:

# lib/rails_admin/config/fields/types/json.rb
# ...
  register_instance_option :pretty_value do
    bindings[:view].content_tag(:pre) { formatted_value }.html_safe
  end
# ...

Apparently bindings[:view] is nil within export context.

Here's a proposed fix for that, I specified an export_value for json field.

Additionally, another test failed for model querying if JSON field is included, so I suppose it's more practical to return nil instead of raising JSON::ParserError in this case.

Thanks for an awesome product!

@blaze182 blaze182 changed the title Fix JSON export crash Fix export crash for models with JSON field Aug 18, 2018
@blaze182 blaze182 force-pushed the fix-json-export-crash branch 3 times, most recently from 9c26555 to 8291903 Compare August 18, 2018 23:13
Failed examples:

rspec
./spec/integration/basic/export/rails_admin_basic_export_spec.rb:31 #
RailsAdmin Export POST /admin/players/export (prompt) allows to export
to CSV with associations and default schema, containing properly
translated header and follow configuration
Failure/Error: bindings[:view].content_tag(:pre) { formatted_value
}.html_safe

NoMethodError:
undefined method `content_tag' for nil:NilClass
./lib/rails_admin/config/fields/types/json.rb:17:in `block in
<class:Json>'
rspec ./spec/rails_admin/adapters/active_record_spec.rb:117 #
RailsAdmin::Adapters::ActiveRecord data access methods #all supports
querying
Failure/Error: value.present? ? JSON.parse(value) : nil

JSON::ParserError:
765: unexpected token at 'Player 211'
./lib/rails_admin/config/fields/types/json.rb:25
@mshibuya
Copy link
Member

Thanks for the PR, fix for export_value looks good. Merged into master as 9fd8d03.

But for the change in parse_input, I don't think silently swallowing parse error is always useful for everybody, and it'a breaking change anyway so not merging in this time.

@mshibuya mshibuya closed this Aug 19, 2018
@blaze182
Copy link
Contributor Author

@mshibuya you can try adding json field to Player model and running specs, it will fail on existing query spec. Same happens in query user interface for any model containing json field, it unexpectedly crashes and needs an explicit queryable: false setting for json field. That’s why I consider it as a bug, probably you have better ideas to handle it (at least mentioning in docs)

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.

2 participants