AR .persisted? throws SystemStackError for an unsaved model with a custom primary_key that didn't save due to validation error #14393

Closed
wants to merge 12 commits into
from

Projects

None yet

4 participants

Contributor

See this for a mini test:
https://gist.github.com/chrisfinne/9564807

While just looking at the test, it doesn't seem like you'd see this very often, but if you

  1. have a Model with a custom primary_key
  2. have vanilla rails scaffolding
  3. POST the form to create a new record
  4. any validation fails
  5. the controller renders the 'new' template which renders the '_form' partial
  6. form_for(@my_model) is called from the partial
  7. form_for checks .persisted?
  8. SystemStackError exception
@chrisfinne chrisfinne AR .persisted? throws SystemStackError for an unsaved model with a
custom primary_key that didn't save due to validation error
d325d89
Member

Hello @chrisfinne,

Thanks you very much for your contribution! I've noted that you are creating a new model but we try to avoid it as much as possible since they are still numerous. It looks like you could just use the Movie model and add a validation on its name field. What do you think ?

Could you also please add a changelog entry to describe the change you've made ?

This is sometimes tricky to find a model that fits with your test case but you can use the grep command. For instance:

$ grep -Rn "self.primary_key" activerecord/test/models
@guilleiguaran guilleiguaran and 1 other commented on an outdated diff Mar 26, 2014
activerecord/CHANGELOG.md
@@ -97,4 +97,11 @@
*Yves Senn*
+* Fixed error where .persisted? throws SystemStackError for an unsaved model with a
guilleiguaran
guilleiguaran Mar 26, 2014 Owner

The CHANGELOG must be placed on top of the file.

rafaelfranca
rafaelfranca Mar 26, 2014 Owner
Fixed error where `.persisted?` throws `SystemStackError` for an unsaved model with a
Owner

Also please squash your commits.

@rafaelfranca rafaelfranca commented on an outdated diff Mar 26, 2014
activerecord/CHANGELOG.md
@@ -97,4 +97,11 @@
*Yves Senn*
+* Fixed error where .persisted? throws SystemStackError for an unsaved model with a
+ custom primary_key that didn't save due to validation error.
rafaelfranca
rafaelfranca Mar 26, 2014 Owner
custom primary key that didn't save due to validation error.
@rafaelfranca rafaelfranca commented on an outdated diff Mar 26, 2014
activerecord/CHANGELOG.md
@@ -97,4 +97,11 @@
*Yves Senn*
+* Fixed error where .persisted? throws SystemStackError for an unsaved model with a
+ custom primary_key that didn't save due to validation error.
+
+ Fixes #14393
rafaelfranca
rafaelfranca Mar 26, 2014 Owner
Fixes #14393.
@rafaelfranca rafaelfranca commented on an outdated diff Mar 26, 2014
activerecord/test/cases/transactions_test.rb
@@ -14,6 +15,11 @@ def setup
@first, @second = Topic.find(1, 2).sort_by { |t| t.id }
end
+ def test_persisted_after_failed_save
rafaelfranca
rafaelfranca Mar 26, 2014 Owner

Better to tell more about the problem in the test description. Something like: test_persisted_in_a_model_with_custom_primary_key_after_failed_save

Owner

Very good patch. I made some comments in the code.

Contributor

I fixed the CHANGELOG placement and syntax and renamed the test method as requested.

I tried squashing the commits but kept running into merge problems, so it looks like a mess. Should I just redo everything under a different pull request?

Owner

I can do it for you if you want.

Contributor

If that's easiest, thanks.

Owner

Merged manually.

Owner

Thank you so much

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