Skip to content

Fix export crash for models with JSON field#3056

Closed
blaze182 wants to merge 4 commits into
railsadminteam:masterfrom
blaze182:fix-json-export-crash
Closed

Fix export crash for models with JSON field#3056
blaze182 wants to merge 4 commits into
railsadminteam:masterfrom
blaze182:fix-json-export-crash

Conversation

@blaze182

@blaze182 blaze182 commented Aug 18, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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