Issues around ActiveType.cast and the association cache

[kratob]

In https://github.com/makandra/active_type/commit/6a623c5c003fc5bb414ba58ee320b2a53fe9b399 we introduced a change to ActiveType.cast that is causing a lot if issues. The change added these lines: https://github.com/makandra/active_type/blob/6a623c5c003fc5bb414ba58ee320b2a53fe9b399/lib/active_type/util.rb#L37-L39

Basically, when casting an object, we now copy all loaded associations to the new object.

This is the cause for issue #147.

Here the user is confused that a cast objects retains associated objects, instead of reloading them. It is relevant, because the load would have casted the associated objects.

Worse, it's also the cause for #146.

Here, a casted object is saved, and through a chain of autosave and inverse-of associations, this also causes the original uncasted object to be saved at the same time, which leads to duplicate database entries (or actually an even weirder crash).

It's generally super unsafe do do anything with the original object after it has been used for a cast, since the casted object and the uncasted object share state.

One possible fix is to revert the copying of the @association_cache. The biggest consequence would be that this loses changes to associations.

We have seen one legitimate use case that did a cast after mutating associations. Code was of the form

class ComplicatedRecord < ApplicationRecord
  def do_some_complex_validation
    # called after some assocations were changed
    ActiveType.cast(self, ComplicatedRecord::ComplexValidator).validate
  end
end

class ComplicatedRecord::ComplexValidator < ActiveType::Record[ComplicatedRecord]
  # bunch of validation methods
end

Not that this is also super dangerous, because the code absolutely does use the uncasted record after casting. It also relies on the fact that errors are shared between casted and uncasted object.

Proposal

• Revert the copying of the @association_cache, which would fix #146 and #147.
• Have ActiveType.cast detect if there are unsaved changes in an assocation. If so, raise an error telling the user those would be lost.
• Maybe allow the user to either
  • opt in to the current behavior with a ActiveType.cast(..., copy_associations: true)
  • or just allow them to do the cast despite the fact that changes will be lost with a ActiveType.cast(..., force: true). I would prefer this solution, as it is the least surprising. The user could manually copy the associations if they wanted to.

[kratob]

@judithroth

[triskweline]

In almost every case it is possible to cast the record into the final subtype before mutation. Let's limit the scope of what ActiveType.cast can handle, and no longer try to cover everyone's edge case.

I could work with the following limitations:

1. You cannot cast a record that has unsaved modifications in its associations (proposal by @kratob).
2. You cannot mutate the original record after casting. For this we could take away the original record's @association_cache and @attributes. This way we no longer share state between original and casted object.
3. You cannot cast a record that has loaded associations . This way the old record cannot be referenced in an inverse-of association. If the user needs to preload associations, they can cast the entire scope/relation before fetching records from the database (ActiveType.cast works on scopes).
4. You cannot cast a record that has unsaved modifications in its own attributes. Maybe that's redundant if we also do (2) and no longer share state.

[judithroth]

Thanks for the summary Tobias! I see that trying to fix the associations for a casted record would be quite hard to do and probably a lot of work.

Not that this is also super dangerous, because the code absolutely does use the uncasted record after casting.

As you pointed out when we were talking, this comes from copying references to complex objects from the original to the casted records - this way they stay coupled forever, changes to one of them will affect the other (as we saw with the example of issue #146 where the casted record gets the ID of the original record, although it had no ID yet when the cast was done).

I agree with both of you that it's not good that with the current implementation of the shared state the library does not complain when you continue to use the old record. But I am not sure if we shoot ourselves in the foot when we are too restrictive on when to allow casting and with disabling the original record. On the other hand, this would prevent lots of strange edge cases.

146 can also be fixed when deep_duping everything from the original record instead of the shallow copy we're doing now. I would like to understand for what reason we're keeping that coupling of the two records?

It also relies on the fact that errors are shared between casted and uncasted object.

This is probably not relevant for the discussion here, but I have the urge to clarify this: The errors are not shared between the casted and uncasted object. The errors seem to be the only thing that is duped by ActiveType before copying them to the casted record :) Having the errors decoupled was the major goal of what I did when introducing the Validator you mentioned.

[judithroth]

Tobias and I talked about this again today. We agreed on the following:

• The changes that introduced copying the association_cache will be reverted
• We want to restrict ActiveType.cast so that it's harder for users to run into edge cases. Since we're not 100% sure where to draw the line to not be too restrictive, we want to test the changes (locally) in several of our projects before releasing.

[judithroth]

Active Type now implements 1. and 2. of @triskweline's suggestions:

1. Casting is prevented when the base record has changes in its already loaded associations, because those would be lost.

2. After casting, the base record will not be usable any more. The base record and the newly created casted record share state which is unexpected.

There is an option (force: true) to override the new behaviour in cases where it's necessary (it's not recommended to do that).