Skip to content

Fix monkey patch of ActiveRecord::Base for records without primary key #282

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

Merged

Conversation

atranson-electra
Copy link
Contributor

Context

On the project I am working on, we are relying on TimescaleDB PostgreSQL extension.
The corresponding hypertables and record implementation does not expose a primary_key attribute on the record class nor does it have an id at the instance level.

This means that all calls to attributes_for_super_diff result with NoMethodError: undefined method to_sym' for nil`.

Fix

This PR proposes to edit the monkey patch to handle this specific case.

@atranson-electra atranson-electra changed the title Fix monkey patch of ActiveRecord::Base for records without primary keys Fix monkey patch of ActiveRecord::Base for records without primary key Mar 7, 2025
Copy link
Collaborator

@jas14 jas14 left a comment

Choose a reason for hiding this comment

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

Thanks, @atranson-electra ! Please add a test – other than that, looks great.

If you're feeling inspired, there are other parts of the code base that rely on the presence of a primary_key. Feel free to address those in a separate PR, or to leave them for someone else.

@atranson-electra
Copy link
Contributor Author

@jas14 I updated the other parts using the primary key and added test on the way 👍

I believe this covers it.
With the extra changes, diff reports of records without primary key won't have : nil and id: nil strings hanging around anymore.

Copy link
Collaborator

@jas14 jas14 left a comment

Choose a reason for hiding this comment

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

Fantastic!

@jas14 jas14 merged commit bb367ea into splitwise:main Mar 9, 2025
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants