validations should be done before or after before_save() callbacks? #142

Closed
nviennot opened this Issue May 5, 2015 · 1 comment

Comments

Projects
None yet
2 participants
@nviennot
Owner

nviennot commented May 5, 2015

We left the discussion hanging in #126 -- but I think there's something fishy in the current behavior.

Right now the validation are performed right after the before_save callbacks. This decision was made because people have the habit of changing data in before_save callbacks.
But this behavior is a little odd, as before_save callbacks operate with non validated data.

If we were to do validations before before_save callbacks, I think we should freeze the model instance during the before_save callbacks. When the user tries to change an attribute within a before_save callback, nobrainer should raise. The model instance should be frozen because validations have been performed already. The exception message should tell the user to change its data in a before_validation callback.

Note: I'm not sure what would be the use cases of before_save callbacks, but if the user does anything expensive, the uniqueness locks will be kept acquired until the persistence operation to the DB is done, which can be for a long period of time depending on what the user does. This is not too bad, as there should be very little contention anyway on the uniqueness locks.

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig May 5, 2015

Contributor

before_validation -> validate -> before_save -> save seems like a
reasonable behavior (with the before_save not allowed to mutate the
object). I'm not entirely sure what the use case of before_save is either,
but I think this structure makes the most sense.

On Tue, May 5, 2015 at 8:37 AM, Nicolas Viennot notifications@github.com
wrote:

We left the discussion hanging in #126
#126 -- but I think there's
something fishy in the current behavior.

Right now the validation are performed right after the before_save
callbacks. This decision was made because people have the habit of changing
data in before_save callbacks.
But this behavior is a little odd, as before_save callbacks operate with
non validated data.

If we were to do validations before before_save callbacks, I think we
should freeze the model instance during the before_save callbacks. When
the user tries to change an attribute within a before_save callback,
nobrainer should raise. The model instance should be frozen because
validations have been performed already. The exception message should tell
the user to change its data in a before_validation callback.

Note: I'm not sure what would be the use cases of before_save callbacks,
but if the user does anything expensive, the uniqueness locks will be kept
acquired until the persistence operation to the DB is done, which can be
for a long period of time depending on what the user does. This is not too
bad, as there should be very little contention anyway on the uniqueness
locks.


Reply to this email directly or view it on GitHub
#142.

Contributor

ajselvig commented May 5, 2015

before_validation -> validate -> before_save -> save seems like a
reasonable behavior (with the before_save not allowed to mutate the
object). I'm not entirely sure what the use case of before_save is either,
but I think this structure makes the most sense.

On Tue, May 5, 2015 at 8:37 AM, Nicolas Viennot notifications@github.com
wrote:

We left the discussion hanging in #126
#126 -- but I think there's
something fishy in the current behavior.

Right now the validation are performed right after the before_save
callbacks. This decision was made because people have the habit of changing
data in before_save callbacks.
But this behavior is a little odd, as before_save callbacks operate with
non validated data.

If we were to do validations before before_save callbacks, I think we
should freeze the model instance during the before_save callbacks. When
the user tries to change an attribute within a before_save callback,
nobrainer should raise. The model instance should be frozen because
validations have been performed already. The exception message should tell
the user to change its data in a before_validation callback.

Note: I'm not sure what would be the use cases of before_save callbacks,
but if the user does anything expensive, the uniqueness locks will be kept
acquired until the persistence operation to the DB is done, which can be
for a long period of time depending on what the user does. This is not too
bad, as there should be very little contention anyway on the uniqueness
locks.


Reply to this email directly or view it on GitHub
#142.

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