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

Handle unpersisted primary key values #448

Merged
merged 2 commits into from Feb 22, 2019
Merged

Handle unpersisted primary key values #448

merged 2 commits into from Feb 22, 2019

Conversation

smudge
Copy link
Contributor

@smudge smudge commented Feb 20, 2019

Two changes here:

  1. I renamed Object#primary_key_value to Object#bullet_primary_key_value to make it clear that this core extension is specific to bullet and to avoid potential naming conflicts. (This feels at least to me like a semi-private method that shouldn't be used by external code...)
  2. I added a test case and made code modifications to handle the case when a primary key value has been manually set on an unpersisted record. One might commonly do this, for instance, in a test context (to avoid making unnecessary database round-trips).

Here's a very contrived example:

# Totally make-believe test case:

post = user.posts.build(id: 123)
comment = post.comments.build(text: 'hey, cool post')
email = WeeklySummaryEmail.new(user: user)

expect(email.subject).to eq "1 new comment on post 123!"
# `email.subject` has code to traverse user->posts->comments

Previously, bullet would prevent this test case from passing, even though the model does not get persisted at all, simply because the id value is present. This change allows bullet to additionally rely on persisted? (if it is defined) to decide whether a record might be subject to N+1 issues, etc.

@flyerhzm flyerhzm merged commit c5f1945 into flyerhzm:master Feb 22, 2019
@smudge smudge deleted the primary-key-unpersisted branch February 22, 2019 17:27
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.

None yet

2 participants