Fix #14: attribute setting on loading from DB

[zverok]

Fixes the problem with attr_readonly setting by avoiding any assignment on loading of persisted models.

I am not 100% sure this is a proper solution, but no tests fail 🤷

[ankane]

Hi @zverok, thanks for the PR! I think it'd be better to use if default_value instead of if !persisted? to fix this while keeping the existing behavior / preserving backwards compatibility.

[ankane]

To expand on this, I wouldn't want writes to start failing due to validation errors if str_enum is added an existing column with NULL values (and allow_nil: true, default: nil is not specified).

[zverok]

To expand on this, I wouldn't want writes to start failing due to validation errors if str_enum is added an existing column with NULL values (and allow_nil: true, default: nil is not specified).

It is an interesting case. What behavior do you see as reasonable in such cases?

Because I intended to bring this case as an argument towards my solution :)

1. Imagine we have (historically) records with kind=NULL in DB
2. Imagine we add str_enum :kind, [:guest, :vip] definition to the column (without allow_nil)
3. Now, we load one of such entries (with status=NULL) from the DB, and...
   a. without str_enum: we have a model object with status == nil. It is not valid, but it faithfully represents what the DB stores.
   b. with str_enum: it has kind == :guest

I believe 3b is basically incorrect behavior. Only the application's author knows the semantics of an old record with kind == nil. All of those might be true:

• They are implicitly "guest" (we didn't have "vip" kind in the past; it is a new thing for new customers);
• They are implicitly "vip" (that's old customers, we value them all and treat them specially);
• A funny third option (that's records that require special handling, like sending old users the reminder emails that they should choose their plan/affiliation status).

With str_enum, we are basically losing the information about the real DB status on loading.

Moreover, if the field is attr_readonly (which is a useful tool especially for financial subsystems: you can't change the existing payment's kind or payment gateway after creation), with str_enum we can't even LOAD such records (they will fail on loading with kind= being prohibited, like in my original ticket).

I believe str_enum's defaults should behave as close to ActiveRecord's native ones as possible--and those are never set on loading of existing records, only on initializing the new ones.

(BTW, another difference from AR default's handling--but purely theoretical for me--is that str_enum can't distinguish between "column value is not provided, set it to default" and "column value is provided explicitly and is nil, leave it be".)

[ankane]

Thanks for the response, but it sounds like we have different opinions on this. Happy to support read-only attributes without changing the existing behavior (just skipping the nil assignment), or you could use a fork with different behavior for your application.