Skip to content

Conversation

@mdayaram
Copy link

Previously, registered store attributes could show inconsistent values between the store and Rails' attribute system, causing validation failures and incorrect behavior with methods like attributes, *_before_type_cast, and dirty tracking. This fix ensures proper synchronization between both systems.

This PR introduces an AttributesSync module that maintains proper synchronization between store values and registered attributes by:

  • Overriding attribute accessors to ensure both systems stay in sync
  • Adding lifecycle callbacks to sync values after initialization and database loads
  • Fixing the attributes method to return actual store values instead of defaults
  • Ensuring proper type casting in both the store and attribute systems

Fixes #52
Fixes #54

Checklist

  • I've added tests for this change
  • I've added a Changelog entry
  • I've updated a documentation (Readme)

…d store attributes

Previously, registered store attributes could show inconsistent values between the store and Rails' attribute system, causing validation failures and incorrect behavior with methods like `attributes`, `*_before_type_cast`, and dirty tracking. This fix ensures proper synchronization between both systems.

This PR introduces an `AttributesSync` module that maintains proper synchronization between store values and registered attributes by:

- Overriding attribute accessors to ensure both systems stay in sync
- Adding lifecycle callbacks to sync values after initialization and database loads
- Fixing the `attributes` method to return actual store values instead of defaults
- Ensuring proper type casting in both the store and attribute systems

Fixes palkan#52
Fixes palkan#54
Copy link
Owner

@palkan palkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes!

The PR confirms my original concerns regarding this feature.

The amount of hacks grows, and I want to be sure we do not interfere in any way with the stable functionality (no registered attributes). So, I'd like to have all the related functionality isolated as much as possible and only included when users opt-in for it.

See the comments for the details.

_local_typed_stored_attributes[store_name][:types][name] = [type, options]

if store_attribute_register_attributes
_ensure_attributes_sync_module_included
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do this only if the feature is enabled (we probably can move the inclusion into the accessor method).

end

def _define_store_attribute_accessors(store_name, name)
# Override getter to read from store
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to override the getter? It's already define by the built-in store-accessor.

As I understand, we need to hack the writer to keep things in sync. For that, I would suggest having a dedicated module prepended with the sync code defined and a super call instead.

And this module should only be included if the feature is enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants