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.

Sign up for GitHub

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

Jump to bottom

[Bug 1.10.0] Issue with frozen strings #53

Closed
airled opened this issue Apr 1, 2020 · 10 comments
Closed

[Bug 1.10.0] Issue with frozen strings #53

airled opened this issue Apr 1, 2020 · 10 comments

Comments

@airled
Copy link

airled commented Apr 1, 2020 •
edited
Loading

In 1.10.0 there is an issue with models that have enum attribute. For example:

class Reservation < ApplicationRecord
  strip_attributes allow_empty: true
  ...
  enum status: [:visited, :cancelled, :fraud]
  ...

IRB:

reservation = Reservation.create!(date: Date.new, time: Time.now)
Traceback (most recent call last):
        2: from (irb):1
        1: from (irb):2:in `rescue in irb_binding'
FrozenError (can't modify frozen String: "visited")

Rails_admin controller log:

Started POST "/admin/reservation/new" for 127.0.0.1 at 2020-04-01 12:57:45  0600
Processing by RailsAdmin::MainController#new as HTML
  ...
Completed 500 Internal Server Error in 13ms (ActiveRecord: 1.0ms | Allocations: 4925)
  
FrozenError (can't modify frozen String: "visited"):
  
strip_attributes (1.10.0) lib/strip_attributes.rb:64:in `gsub!'
strip_attributes (1.10.0) lib/strip_attributes.rb:64:in `strip_string'
strip_attributes (1.10.0) lib/strip_attributes.rb:46:in `block in strip_record'
strip_attributes (1.10.0) lib/strip_attributes.rb:44:in `each'
strip_attributes (1.10.0) lib/strip_attributes.rb:44:in `strip_record'
strip_attributes (1.10.0) lib/strip_attributes.rb:35:in `strip'
strip_attributes (1.10.0) lib/strip_attributes.rb:9:in `block in strip_attributes'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:429:in `instance_exec'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:429:in `block in make_lambda'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:201:in `block (2 levels) in halting'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:607:in `block (2 levels) in default_terminator'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:606:in `catch'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:606:in `block in default_terminator'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:202:in `block in halting'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:514:in `block in invoke_before'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:514:in `each'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:514:in `invoke_before'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:134:in `run_callbacks'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:827:in `_run_validation_callbacks'
activemodel (6.0.2.2) lib/active_model/validations/callbacks.rb:118:in `run_validations!'
activemodel (6.0.2.2) lib/active_model/validations.rb:337:in `valid?'
activerecord (6.0.2.2) lib/active_record/validations.rb:68:in `valid?'
activerecord (6.0.2.2) lib/active_record/validations.rb:85:in `perform_validations'
activerecord (6.0.2.2) lib/active_record/validations.rb:47:in `save'
activerecord (6.0.2.2) lib/active_record/transactions.rb:315:in `block in save'
activerecord (6.0.2.2) lib/active_record/transactions.rb:375:in `block in with_transaction_returning_status'
activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/database_statements.rb:281:in `block in transaction'
activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/transaction.rb:280:in `block in within_new_transaction'
activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/transaction.rb:278:in `synchronize'
activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/transaction.rb:278:in `within_new_transaction'
activerecord (6.0.2.2) lib/active_record/connection_adapters/abstract/database_statements.rb:281:in `transaction'
activerecord (6.0.2.2) lib/active_record/transactions.rb:212:in `transaction'
activerecord (6.0.2.2) lib/active_record/transactions.rb:366:in `with_transaction_returning_status'
activerecord (6.0.2.2) lib/active_record/transactions.rb:315:in `save'
activerecord (6.0.2.2) lib/active_record/suppressor.rb:44:in `save'
rails_admin (2.0.2) lib/rails_admin/adapters/active_record/abstract_object.rb:23:in `save'
rails_admin (2.0.2) lib/rails_admin/config/actions/new.rb:41:in `block (2 levels) in <class:New>'
rails_admin (2.0.2) app/controllers/rails_admin/main_controller.rb:22:in `instance_eval'
rails_admin (2.0.2) app/controllers/rails_admin/main_controller.rb:22:in `new'
actionpack (6.0.2.2) lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action'
actionpack (6.0.2.2) lib/abstract_controller/base.rb:196:in `process_action'
actionpack (6.0.2.2) lib/action_controller/metal/rendering.rb:30:in `process_action'
actionpack (6.0.2.2) lib/abstract_controller/callbacks.rb:42:in `block in process_action'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:135:in `run_callbacks'
actionpack (6.0.2.2) lib/abstract_controller/callbacks.rb:41:in `process_action'
actionpack (6.0.2.2) lib/action_controller/metal/rescue.rb:22:in `process_action'
actionpack (6.0.2.2) lib/action_controller/metal/instrumentation.rb:33:in `block in process_action'
activesupport (6.0.2.2) lib/active_support/notifications.rb:180:in `block in instrument'
activesupport (6.0.2.2) lib/active_support/notifications/instrumenter.rb:24:in `instrument'
activesupport (6.0.2.2) lib/active_support/notifications.rb:180:in `instrument'
actionpack (6.0.2.2) lib/action_controller/metal/instrumentation.rb:32:in `process_action'
actionpack (6.0.2.2) lib/action_controller/metal/params_wrapper.rb:245:in `process_action'
searchkick (4.3.0) lib/searchkick/logging.rb:212:in `process_action'
activerecord (6.0.2.2) lib/active_record/railties/controller_runtime.rb:27:in `process_action'
actionpack (6.0.2.2) lib/abstract_controller/base.rb:136:in `process'
actionview (6.0.2.2) lib/action_view/rendering.rb:39:in `process'
actionpack (6.0.2.2) lib/action_controller/metal.rb:191:in `dispatch'
actionpack (6.0.2.2) lib/action_controller/metal.rb:252:in `dispatch'
actionpack (6.0.2.2) lib/action_dispatch/routing/route_set.rb:51:in `dispatch'
actionpack (6.0.2.2) lib/action_dispatch/routing/route_set.rb:33:in `serve'
actionpack (6.0.2.2) lib/action_dispatch/journey/router.rb:49:in `block in serve'
actionpack (6.0.2.2) lib/action_dispatch/journey/router.rb:32:in `each'
actionpack (6.0.2.2) lib/action_dispatch/journey/router.rb:32:in `serve'
actionpack (6.0.2.2) lib/action_dispatch/routing/route_set.rb:837:in `call'
railties (6.0.2.2) lib/rails/engine.rb:526:in `call'
railties (6.0.2.2) lib/rails/railtie.rb:190:in `public_send'
railties (6.0.2.2) lib/rails/railtie.rb:190:in `method_missing'
actionpack (6.0.2.2) lib/action_dispatch/routing/mapper.rb:19:in `block in <class:Constraints>'
actionpack (6.0.2.2) lib/action_dispatch/routing/mapper.rb:48:in `serve'
actionpack (6.0.2.2) lib/action_dispatch/journey/router.rb:49:in `block in serve'
actionpack (6.0.2.2) lib/action_dispatch/journey/router.rb:32:in `each'
actionpack (6.0.2.2) lib/action_dispatch/journey/router.rb:32:in `serve'
actionpack (6.0.2.2) lib/action_dispatch/routing/route_set.rb:837:in `call'
rack-pjax (1.1.0) lib/rack/pjax.rb:12:in `call'
remotipart (1.4.4) lib/remotipart/middleware.rb:32:in `call'
warden (1.2.8) lib/warden/manager.rb:36:in `block in call'
warden (1.2.8) lib/warden/manager.rb:34:in `catch'
warden (1.2.8) lib/warden/manager.rb:34:in `call'
rack (2.2.2) lib/rack/tempfile_reaper.rb:15:in `call'
rack (2.2.2) lib/rack/etag.rb:27:in `call'
rack (2.2.2) lib/rack/conditional_get.rb:40:in `call'
rack (2.2.2) lib/rack/head.rb:12:in `call'
actionpack (6.0.2.2) lib/action_dispatch/http/content_security_policy.rb:18:in `call'
rack (2.2.2) lib/rack/session/abstract/id.rb:266:in `context'
rack (2.2.2) lib/rack/session/abstract/id.rb:260:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/cookies.rb:648:in `call'
activerecord (6.0.2.2) lib/active_record/migration.rb:567:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'
activesupport (6.0.2.2) lib/active_support/callbacks.rb:101:in `run_callbacks'
actionpack (6.0.2.2) lib/action_dispatch/middleware/callbacks.rb:26:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/actionable_exceptions.rb:17:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/debug_exceptions.rb:32:in `call'
web-console (4.0.1) lib/web_console/middleware.rb:132:in `call_app'
web-console (4.0.1) lib/web_console/middleware.rb:28:in `block in call'
web-console (4.0.1) lib/web_console/middleware.rb:17:in `catch'
web-console (4.0.1) lib/web_console/middleware.rb:17:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/show_exceptions.rb:33:in `call'
railties (6.0.2.2) lib/rails/rack/logger.rb:38:in `call_app'
railties (6.0.2.2) lib/rails/rack/logger.rb:26:in `block in call'
activesupport (6.0.2.2) lib/active_support/tagged_logging.rb:80:in `block in tagged'
activesupport (6.0.2.2) lib/active_support/tagged_logging.rb:28:in `tagged'
activesupport (6.0.2.2) lib/active_support/tagged_logging.rb:80:in `tagged'
railties (6.0.2.2) lib/rails/rack/logger.rb:26:in `call'
sprockets-rails (3.2.1) lib/sprockets/rails/quiet_assets.rb:13:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/remote_ip.rb:81:in `call'
request_store (1.5.0) lib/request_store/middleware.rb:19:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/request_id.rb:27:in `call'
rack (2.2.2) lib/rack/method_override.rb:24:in `call'
rack (2.2.2) lib/rack/runtime.rb:22:in `call'
activesupport (6.0.2.2) lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/static.rb:126:in `call'
rack (2.2.2) lib/rack/sendfile.rb:110:in `call'
actionpack (6.0.2.2) lib/action_dispatch/middleware/host_authorization.rb:83:in `call'
webpacker (5.0.1) lib/webpacker/dev_server_proxy.rb:23:in `perform_request'
rack-proxy (0.6.5) lib/rack/proxy.rb:57:in `call'
railties (6.0.2.2) lib/rails/engine.rb:526:in `call'
puma (4.3.3) lib/puma/configuration.rb:228:in `call'
puma (4.3.3) lib/puma/server.rb:682:in `handle_request'
puma (4.3.3) lib/puma/server.rb:472:in `process_client'
puma (4.3.3) lib/puma/server.rb:328:in `block in run'
puma (4.3.3) lib/puma/thread_pool.rb:134:in `block in spawn_thread'

If I downgrade to 1.9.2 the issue is gone.

1.9.2 had this code

value = value.strip if value.respond_to?(:strip)

that automatically created new unfrozen value.
So possible solution might be to add here

value = value.dup
@airled airled changed the title [Bug 1.10.0] Error saving models with enum [Bug 1.10.0] Can't save models with enum Apr 1, 2020
@rmm5t
Copy link
Owner

rmm5t commented Apr 1, 2020

Thanks for the report. Was this not a problem in v1.9.0 as well, or was this problem introduced in v1.10.0?

@kivanio
Copy link

kivanio commented Apr 1, 2020

Hi,
It might not be related with enum.
I have got a lot of errors and it is not in enum code.

I am cleaning a string:

template = StripAttributes.strip(template, collapse_spaces: true)
      
      FrozenError:
        can't modify frozen String

It might happens because # frozen_string_literal: true at file.

And it works in old version.

@airled
Copy link
Author

airled commented Apr 1, 2020

Thanks for the report. Was this not a problem in v1.9.0 as well, or was this problem introduced in v1.10.0?

When I downgrade to 1.9.2 then problem is gone. The problem is that string value can be frozen and bang methods raise.

@airled
Copy link
Author

airled commented Apr 1, 2020 •
edited
Loading

Hi,
It might not be related with enum.
I have got a lot of errors and it is not in enum code.

I am cleaning a string:

template = StripAttributes.strip(template, collapse_spaces: true)

  FrozenError:
    can't modify frozen String

It might happens because # frozen_string_literal: true at file.

And it works in old version.

I didn't look up in rails code for enums, but I think, that rails freezes all enum values. So yes, problem is not with enums itself but with frozen strings generally.

@airled airled changed the title [Bug 1.10.0] Can't save models with enum [Bug 1.10.0] Issue with frozen strings Apr 1, 2020
@airled
Copy link
Author

airled commented Apr 1, 2020 •
edited
Loading

@rmm5t As I mentioned earlier, when I add

value = value.dup

right after

return value unless value.is_a?(String)

at

def self.strip_string(value, options = {})

the problem is gone.

@kivan Can you please do the same and confirm that your issue is gone too?

@rmm5t
Copy link
Owner

rmm5t commented Apr 1, 2020

Hi all, I'm inclined to think that the desired behavior here would be to ignore frozen strings. i.e. Short-circuit with a guard clause if the string is frozen. Thoughts?

Aside, I'm a little confused why this behavior exists in 1.10.0, but not 1.9.x, but I'll dig in with some tests.

@zarqman
Copy link
Contributor

zarqman commented Apr 1, 2020 •
edited
Loading

We're seeing this too, with frozen strings coming from a different source than enums.

The behavior in 1.9.x would have been equivalent to adding value = value.dup after L55.

Short-circuiting on .frozen? in some way makes sense, but it is a definite breaking behavior change from 1.9, and so in my opinion should be reserved for a 2.0 release.

For 1.10.x, I recommend adding value = value.dup as proposed. If you believe short-circuiting is indeed preferred behavior, please release a separate 2.0 and declare it as a breaking change. (Edit to add: And, still make the short-circuit optional. There can be valid reasons to want to dup/strip frozen strings.)

Looks like you may already be on this, but if you want me to prepare a PR to fix 1.10.x, let me know.

@airled
Copy link
Author

airled commented Apr 1, 2020 •
edited
Loading

Hi all, I'm inclined to think that the desired behavior here would be to ignore frozen strings. i.e. Short-circuit with a guard clause if the string is frozen. Thoughts?

Aside, I'm a little confused why this behavior exists in 1.10.0, but not 1.9.x, but I'll dig in with some tests.

1.9 does not have bug because of

value = value.strip if value.respond_to?(:strip)

strip method creates new unfrozen string and all bang methods below do not raise because they can mutate it.

I think a guard for a frozen string is the best solution, but I agree with @zarqman that for now it is best to just dup string.

@rmm5t
Copy link
Owner

rmm5t commented Apr 2, 2020

I'm going to do two things here.

  1. I'm going to release a v1.10.1 release to dup the value before anything else. This will mimick v1.9 behavior.
  2. I'm going to release a v1.11.0 release to add a guard clause and perform a no-op on frozen strings. This will be somewhat backwards incompatible, but not quite worth a v2 release in my opinion, because I don't think anyone using frozen strings would expect those values to be modified by strip_attributes.

@rmm5t
Copy link
Owner

rmm5t commented Apr 2, 2020

Fixed by c87721f

Enhanced and closed by 622699f

v1.10.1 and v1.11.0 both released as well.

@rmm5t rmm5t closed this as completed Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@rmm5t @kivanio @zarqman @airled