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.

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

Fix Person trying to serialize non-existing methods in User #10575

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gregorbg
Copy link
Member

@gregorbg gregorbg commented Jan 9, 2025

For some reason, we have the following code snippet in the serializable_hash method of Person:

# If there's a user for this Person, merge in all their data,
# the Person's data takes priority, though.
(user || User.new).serializable_hash(user_override_options).merge(json)

The code comment explains what the intention is, but the implementation is problematic because it directly passes down the serialization options from Person down to User. But Person has plenty of methods and properties (which the developer might want to serialize) that don't exist on User at all.

The only reason why this didn't break so far is because nobody ever happened to request (i.e. try and serialize) a property of Person that isn't available from User.

The monkey-patched method is essentially a short-hand for:

{
  only: USER_PERSON_COMMON_OPTIONS[:only] & options[:only],
  methods: USER_PERSON_COMMON_OPTIONS[:methods] & options[:methods],
  includes: USER_PERSON_COMMON_OPTIONS[:includes] & options[:includes],
}

with a little bit of null-safety built in. The intention is to merge these two arrays together, but only keep the union (i.e. common values shared between two arrays) of the value params. That way, we can effectively restrict which params should be passed down to User.

Copy link
Member

@danieljames-dj danieljames-dj left a comment

Choose a reason for hiding this comment

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

I understood almost everything, the monkey_patches code is bit confusing for me, but I think I understood it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants