Open wata727 opened 7 months ago
Sorry for the late response :bow:
You're right. #extension_allowlist
raises CarrierWave::IntegrityError
then skips #cache
, resulting in the attribute value unchanged.
How important is this behavior of being able to detect a change in validation failure in your usecase? I mean, there can be an explanation that the attribute value was not changed because assigning an invalid value was rejected. But if this behavior is crucial we may need to find a way to keep it.
By the way, this single-file snippet you used is very useful for reproducing issues. Let me use this way in other places 👍
Thank you for your reply. For example, if you have models like the one above, you may run into issues when using nested attributes to submit images from a form. Imagine params like below are submitted from the form:
{
id: 1,
profile_attributes: {
id: 2,
avatar: <...> # image
}
}
The controller handles it as follows:
def update
@user = User.find(params[:id])
if @user.update(params.permit(:profile_attributes))
redirect_to(@user)
else
render :edit, status: :unprocessable_entity
end
end
In this case, if the passed image is invalid, the update should fail and the user should be given feedback on the cause of the error. However, since user.invalid?
does not return true, the error is ignored and the update succeeds.
Intuitively, similar to Active Model, I believe it's better to keep the invalid value assigned rather than rejecting it. That's why I recommend changing the identifier even on assignment of invalid values. What do you think?
By the way, this single-file snippet you used is very useful for reproducing issues. Let me use this way in other places 👍
👍 This snippet was inspired by Rails. It's good practice to be able to reproduce the issue in a single file. You might want to start with an issue template.
While testing CarrierWave v3, we encountered an issue where saving files with invalid extensions did not fail correctly. After digging into the details, it seems that due to https://github.com/carrierwaveuploader/carrierwave/pull/2658, it is not marked as changed if validation fails.
v2.2.5 works correctly:
A temporary identifier is generated based on the cached file, but if the save fails like this, the identifier will not be generated and I think this issue will occur. https://github.com/carrierwaveuploader/carrierwave/blob/v3.0.5/lib/carrierwave/mounter.rb#L100 https://github.com/carrierwaveuploader/carrierwave/blob/v3.0.5/lib/carrierwave/mounter.rb#L98
P.S. Thank you for maintaining CarrierWave! We are always helped by your wonderful work :)