Our php codes base has 6 different ways to do INSERT … ON DUPLICATE KEY UPDATE, how do I fix it?

Question

Our php codes base has 6 different ways to do INSERT … ON DUPLICATE KEY UPDATE. It happened over years because the php framework is evolving and my team members come and go, although I won't say we have a high turnover rate.

First, there are updateOrCreate and updateOrInsert in laravel. Their difference is already confusing. Check this article for some explanation, The Differences Between UpdateOrCreate And UpdateOrInsert in Laravel. They both generate 2 SQL clauses - first is SELECT, and second is UPDATE or INSERT depends on the result of first SELECT.

Second, because we use mysql, one senior guy introduced an open source project https://github.com/yadakhov/insert-on-duplicate-key to utilize INSERT … ON DUPLICATE KEY UPDATE on mysql. During that time it was the right move because we knew updateOrCreate and updateOrInsert generated two SQL clauses and we knew that could be problematic. And we also knew laravel did not support it back then.

Third, after we upgraded to laravel 9, we found laravel has upsert, introduced by https://github.com/laravel/framework/pull/34698. upsert will do INSERT … ON DUPLICATE KEY UPDATE on mysql too.

Fourth, a new guy joined the team and always use replace without knowing the exact difference between replace and insert on duplicate update (refer to What are practical differences between REPLACE and INSERT ... ON DUPLICATE KEY UPDATE in MySQL?)

Fifth, some member still writes his own 2 SQL clauses, select first then check the result to see if insert or update next. Maybe he is not familiar with INSERT … ON DUPLICATE KEY UPDATE or upsert, maybe he is just sloppy and our code review didn't catch that too.

So now in our code bases we have six different ways to deal with insert or update. I am not sure often this happens for a code base existing for more than 5 years (this particular case maybe be extreme, but we do have 2 or 3 different ways to do basically the same thing in other case). But do I fix this ? Code review and refactor are always easier said than done (The fact that we are having this problem now is a clear sign we didn't code review enough).

BTW, regarding the programming issue caused by 2 SQL clauses I asked a question on stackoverlfow. I am asking the question here from engineering point of view.

Answer 1

There are at least two questions in your question:

• how do I fix the codebase?

  (ok, this is obvious: you pick a prefered method, then use your a text editor/IDE for refactoring the code and test the system after the changes)

• how do I prevent this from happen again?

And there is also a 3rd question one should ask here:

• is it really worth to fix and/or prevent this?

Let's start with the last one. If I got this right, at least two of the methods you mentioned can cause real trouble and hard-to-spot failures (the "replace" method and the "separate SQL in wrong order/with wrong transactional boundaries" method). Those are definitely worth to be fixed, not just for hardening your system, but also for not being picked accidentally as templates for new code.

For the other methods: consistency in a code base has definitely some value, since it will increase readability and maintainability. Still, when you have a working system, and consistency can only be achieved by changing (and retesting) huge amounts of code, then it becomes a trade-off, since each change has always a certain risk of introducing bugs. If it is worth the hassle depends heavily on

• how many places in the code base will be affected

• how easy it is to rewrite the affected code blocks with a low-risk of changing the semantics unintentionally

• how much testing effort this will cause (which depends on how good your automated test suite is)

These are questions only you can answer by yourself, and the answers will help you to make a cost/benefit analysis. If you are unsure about the expected effort, you may invest a day or two, try to refactor, retest the affected part of the code base and see how much you can achieve in this time box.

So what is about preventing this from happen again?

You already answered this in your question, at least partly: "The fact that we are having this problem now is a clear sign we didn't code review enough". So yes, code reviews will definitely help. Still code reviews can be much more effective when you have a written coding standard which defines your preferred method for INSERT … ON DUPLICATE KEY UPDATE. The coding standard will also help newbies to implement things in the "best" way right from the start. You just have to decide with your team which of the different techniques your team will prefer.

A word on your specific question about using Laraval 9 upsert after having used updateOrCreate several times:

Please have a look into this older question:

• Should older code be updated to use newer language constructs, or should outdated constructs be stuck with?

But don't be disappointed that the accepted answer will not give you a clear way how to deal with the issue.

After reading again what you wrote above and the Stackoverflow links you provided, however, I think there is one difference in your situation: the different methods you described are not 100% identical in their robustness. Though I am not a Laravel expert, it seems the new upsert in Laravel 9 is the most reliable method available in the framework, more reliable than updateOrCreate, without the timestamp flaw updateOrInsert has, and without the performance penalty or robustness issues of most of the other methods. If I am right, and you don't need backwards compatibility to older Laravel versions, that would be a strong argument to use upsert as the new default in your coding standard, at least for any newly written code. Still, this will not tell you if changing all of the existing code proactively will have an affordable cost/benefit relationship.