[10.x] HotFix: throw captured UniqueConstraintViolationException if there are no matching records on SELECT retry

[mpyw]

• Fixes Unexpected NULL in createOrFirst() with Multiple Unique Constraints #48235
• Related:
  • [10.x] Adds a createOrFirst method to Eloquent #47973
  • [10.x] Fix createOrFirst on transactions #48144
  • [10.x] Enhancing updateOrCreate() to Use firstOrCreate() #48160
  • [10.x] Ensuring Primary Reference on Retry in createOrFirst() #48161
  • [10.x] Make the firstOrCreate methods in relations use createOrFirst behind the scenes #48192
  • [10.x] Make the updateOrCreate methods in relations use firstOrCreate behind the scenes #48213
  • [10.x] Ensure createOrFirst does not return null at type level #48214

There is an issue with createOrFirst up to version v10.21.0. This causes the following problems when createOrFirst() returns NULL.

• firstOrCreate() also inadvertently returns NULL.
• updateOrCreate() causes an error because it tries to access the ->wasRecentlyCreated property on NULL.

The latter is particularly critical, so if possible, we would like you to release it as a hot fix in version v10.21.1. Please consider it.

[fuwasegu]

Looks like a good solution
thanks!

[mpyw]

@taylorotwell @driesvints @tonysm Please check it out 😃

[mpyw]

Failing tests look to be irrelevant. Ready to merge.

[tonysm]

@mpyw if it got it right, this isn't "fixing" the problem, right? It's more like letting the error throw so the developer knows about the edge case, which I guess it makes sense. I didn't think about this scenario as I was working on this feature. 🤔

[tonysm]

And the previous implementation of updateOrCreate would also fail in this scenario, right? A unique constraint violation would happen and a QueryException would be thrown when it tried to save the record.

[mpyw]

@tonysm Thank you for your comment!

Firstly, it's important to clarify that a QueryException was correctly thrown for unique constraint violations on the previous updateOrCreate implementation. However, with the recent implementation, silently returning NULL led to an unintended Attempt to read property "wasRecentlyCreated" on null error in updateOrCreate, which is a significant issue.

While you mentioned that this change seems to be just aiding the developer's debug process, it is crucial in the context of a framework providing functionality to web applications. The correct behavior here involves the code either succeeding or throwing a UniqueConstraintViolationException, rather than leading to an unintended null error.

Furthermore, it is worth noting that the UniqueConstraintViolationException is treated like a LogicException in some contexts, while there may be cases where we want to treat it like a RuntimeException. For instance, my application was initially designed to potentially encounter unique constraint errors with the non-unique $values in updateOrCreate, and this was part of the regular flow. Encountering a different, all-encompassing error instead of the designed UniqueConstraintViolationException in such a case poses a significant problem.

In conclusion, while it may appear that this change merely aids in debugging, it is a critical adjustment that prevents unintended behavior and enables developers to handle unique constraint violations more appropriately in various scenarios.

[mpyw]

@taylorotwell Thanks for merging!

[mpyw]

@tonysm I think I might understand a bit of what you're saying. Are you suggesting that, in the case of two or more independent unique keys (not composite unique keys), if one of them causes a unique constraint error, we should use the attribute that caused the error as a key to retrieve the existing record?

Indeed, there might be useful cases for this, but attempting to address this would make the operation extremely complex and would likely require changing the method signature to accept groups of $attributes. I am a colleague of @fuwasegu, the person who raised the issue, and we did not consider such cases. I believe it is acceptable to consider this issue out of scope for the current response.

[tonysm]

@mpyw I get it that the unique violation can be used programatically.

I'm trying to think if we better switch the createOrFirst to do a firstOrFail instead of first and let it throw a ModelNotFoundException instead of UniqueViolationException` (as you did in your recent closed PR).

I think this would signal better that the record couldn't be found using the given attributes.

But this wouldn't solve the issue in updateOrCreate, at least not in a BC way, since the ModelNotFoundException is not a subclass of the QueryException.

[mpyw]

@tonysm Using ModelNotFoundException as it indeed seems suitable for certain data inconsistency situations, such as the one addressed in PR #48161 with the ->useWritePdo() method. However, as I've rightly noted, that specific issue has already been resolved and should no longer occur.

Given this, I believe it's more appropriate in this context to rethrow the UniqueConstraintViolationException caused by the attributes listed in $values.

[fuwasegu]

@tonysm
Thank you for engaging in a discussion to improve the issue I reported!

I don't have any objections to the changes that @mpyw has made in this PR.

The flow here is:

1. Try to create a record using $attributes and $values -> if successful, return it.
2. If a Unique Key error prevents creation, we assume a record already exists and try to fetch and return it.

You argue that since the final operation is a 'SELECT', the appropriate exception to throw would be ModelNotFoundException. I understand that perspective.

However, this behavior contradicts what a developer might expect. They would assume that the retrieval is based on $attributes failing the unique constraint, while in reality, it's $values that's causing the conflict.
It is counterintuitive to search using $attributes and get no result when it appears that $attributes violates the unique key constraint. The real reason for this is that $values violates the unique constraint.

Thus, the root cause of the SELECT resulting in NULL is a Unique Key violation. So, shouldn't developers be dealing with UniqueViolationException instead?

[tonysm]

I'm not convinced that rethrowing the UniqueConstraintViolationException is the best way to go here. The dev would expect the method to handle unique violations, not throw it back at them. Passing unique values instead of attributes isn't the expected behavior. This is an edge case, and from the perspective of createOrFirst, I think it would make more sense to throw a ModelNotFoundException (since that's really what happened; it didn't find a record using the unique attributes even though a unique violation occurred). Maybe the unique constraint could be the "previous" exception to that.

But, as I said, the issue with the updateOrCreate is different; it expects a model to be returned, but it gets null instead. Again, this was caused by the edge case of passing a unique field as values instead of in the attributes list. In that case, I don't know what we could throw. ModelNotFoundException doesn't make sense because you're telling it to update or create, but UniqueConstraintViolationException also doesn't. In this case, either exception would work.

However, it was merged, so it's okay.

[trevorgehman]

They would assume that the retrieval is based on $attributes failing the unique constraint, while in reality, it's $values that's causing the conflict.
It is counterintuitive to search using $attributes and get no result when it appears that $attributes violates the unique key constraint. The real reason for this is that $values violates the unique constraint.

This is exactly the situation we encountered when our test suite failed updating to 10.21.0. We were getting type errors because firstOrCreate suddenly started returning null. The reason was we had a unique constraint on a column in the values list and were listening for a QueryException to check if it was caused by the constraint. But then that exception stopped being thrown.

I think throwing the UniqueConstraintViolationException exception is the right choice. Just my $0.02.