Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make location options fields unique #1011

Merged
merged 5 commits into from
Sep 20, 2022
Merged

Conversation

philipxyc
Copy link
Contributor

The problem

Every time an admin clicks on the save button on the options page for a location, around 25 rows of new data will be inserted to location_options table.

Expected behavior

For dealing with update options for a location, old option records should be modified instead of inserting new records.

Why matters

I finally ended up with a location_options table with 500+ rows for only a single location, which significantly slower my page response time to longer than 10 seconds.

Root cause

In the updateOptions function of LocationOption.php, there implemented the logic for updating the existing option record.

protected function updateOptions()
{
if (!$location = $this->locationContext)
return false;
self::upsert(collect($this->itemsToSaveCache)
->map(function ($value, $key) use ($location) {
return [
'location_id' => $location->location_id,
'item' => $key,
'value' => json_encode($value),
];
})->values()->all(), ['location_id', 'item'], ['value']);
unset(static::$cache[$location->location_id]);
return true;
}

However, the self::upsert function is used there, on which documentation writes:

All databases except SQL Server require the columns in the second argument of the upsert method to have a "primary" or "unique" index. In addition, the MySQL database driver ignores the second argument of the upsert method and always uses the "primary" and "unique" indexes of the table to detect existing records.

This means the upsert logic in updateOptions function is fallbacked to the behavior as insert, because the fields don't have "unique" index.

Work in progress (help needed)

This migration works fine if there is no duplicate ('location_id', 'item') data in the current location_options table.

However, as mentioned above, it is very likely already exists many duplicated data there. In this case, the migration will fail because of the error "Integrity constraint violation: 1062 Duplicate entry".

To properly solve the issue, a logic for removing existing duplicates for this table should be implemented before the schema change. I did some research about it but was still not able to find a good way. I would be appreciated it if you can give me some help in implementing this.

philipxyc and others added 2 commits September 3, 2022 16:16
Signed-off-by: Sam Poyigi <6567634+sampoyigi@users.noreply.github.com>
@sampoyigi
Copy link
Member

Added the logic to remove existing duplicates before adding unique columns. Let me know if it works for you.

@philipxyc
Copy link
Contributor Author

Looks good to me! My only concern is if the logic going to keep the latest options or just randomly keep one, I think the eloquent going to get the latest rows by default? My database has already been manually migrated so I can't test it out for now. Otherwise, I think this PR is good to go!

Signed-off-by: Sam Poyigi <6567634+sampoyigi@users.noreply.github.com>
@sampoyigi sampoyigi merged commit 5ec9a23 into tastyigniter:3.x Sep 20, 2022
sampoyigi added a commit to tastyigniter/flame that referenced this pull request Nov 24, 2022
- Remove locked property from composer.json manifest (tastyigniter/TastyIgniter@be92fda8)
- Drop asset minification feature to fix JS issue with admin not able to login and unable to edit mail templates. #970 #948 (tastyigniter/TastyIgniter@b9df4df1)
- Add allergens relation to 3.x branch (tastyigniter/TastyIgniter#978) (tastyigniter/TastyIgniter@ed16a6d3)
- minor fix (tastyigniter/TastyIgniter@e8d87178)
- Fix location_options issue when creating a location #981 (tastyigniter/TastyIgniter@ccbba690)
- minor fixes (tastyigniter/TastyIgniter@721943ae)
- Remove unique validation rule on allergen, category, location, mealtime, menu and table requests (tastyigniter/TastyIgniter@20a9e629)
- Add permissions to order, reservation delete bulk action #986 (tastyigniter/TastyIgniter@16f24e05)
- Add padding to modal form fields container (tastyigniter/TastyIgniter@bc9ceb12)
- Add branch name to split GA (tastyigniter/TastyIgniter@f70ccc7b)
- New admin.list.extendRecords event (tastyigniter/TastyIgniter#989) (tastyigniter/TastyIgniter@aeb055b7)
- Minor fix (tastyigniter/TastyIgniter@f4996352)
- Use locale dates formats in order and reservation emails (tastyigniter/TastyIgniter@3f5303e6)
- Minor refactor to assignables (tastyigniter/TastyIgniter@d5fd36cc)
- Add location logo mail variables (tastyigniter/TastyIgniter@d91683e9)
- Move allocate table logic from booking manager to reservation mode (tastyigniter/TastyIgniter@a4530bdd)
- Minor fix (tastyigniter/TastyIgniter@4d9bdeb8)
- Add migration to drop all foreign keys due to irritating foreign key errors (tastyigniter/TastyIgniter@ec0542bd)
- Ensure list column names do not much model relation name (tastyigniter/TastyIgniter@5e84a712)
- Check foreign key/index exists before dropping (tastyigniter/TastyIgniter@bbaa0796)
- Use customer email as replyTo when appropriate (tastyigniter/tastyigniter@#1002) (tastyigniter/TastyIgniter@4635985d)
- Ensure guest_num and duration have minimum values (tastyigniter/TastyIgniter#1001) (tastyigniter/TastyIgniter@a3f024a0)
- use lowest menu option price when menu item price is 0 (tastyigniter/TastyIgniter@137734cb)
- Add location options as HasMany relation (tastyigniter/TastyIgniter@652cb6e2)
- Fix location list order type filter (tastyigniter/TastyIgniter@68e405b0)
- Add comment column to reservations list tastyigniter/ti-ext-reservation#32 (tastyigniter/TastyIgniter@3016c2fd)
- Sort menu item option dropdown alphabetically (tastyigniter/TastyIgniter@f30bbff6)
- Fix issue where cart item option quantity type calculated value is not properly displayed (tastyigniter/TastyIgniter@f2c500cc)
- Update [README.md](http://readme.md/) (tastyigniter/TastyIgniter@b3732a3f)
- Install core update first instead of ignoring non-core updates (tastyigniter/TastyIgniter@2a754e46)
- Fix broken link (tastyigniter/TastyIgniter@a937a00d)
- Minor fix (tastyigniter/TastyIgniter@6486a89d)
- Fix issue where media finder attachment config modal not opening (tastyigniter/TastyIgniter#1010) (tastyigniter/TastyIgniter@2c1f30d6)
- Minor fix (tastyigniter/TastyIgniter@5bf7b4e7)
- Cache location options (tastyigniter/TastyIgniter@cfe5f965)
- Make location options fields unique (tastyigniter/TastyIgniter#1011) (tastyigniter/TastyIgniter@5ec9a233)
- Minor fixes (tastyigniter/TastyIgniter@5cbaced6)
- Add support for locationable trait on HasManyThrough, HasOneThrough relationships (tastyigniter/TastyIgniter@da6bc020)
- Allow rendering of multiple list widgets (tastyigniter/TastyIgniter@ed2a9101)
- Fix dashboard news widget feed url (tastyigniter/TastyIgniter@98bc919d)
- Reorder reservation form fields (tastyigniter/TastyIgniter@895d9949)
- Minor UI fix (tastyigniter/TastyIgniter@5e7fbc53)
- Fix issue where reservation list can not be filtered by multiple statuses (tastyigniter/TastyIgniter@de8ec050)
- Ensure admin password max length is 32 when installing (tastyigniter/TastyIgniter@9ceb0251)
- Minor fix (tastyigniter/TastyIgniter@359687ee)
- Fix dashboard charts not displaying data (tastyigniter/TastyIgniter@80a8ba3a)
- Minor refactor (tastyigniter/TastyIgniter@b780b42c)
- Strip html tags from formatted address (tastyigniter/TastyIgniter@88f9243d)
- Support multiple extension paths (tastyigniter/TastyIgniter#1017) (tastyigniter/TastyIgniter@cd89efb7)
- Allow payments, themes form query to be extended (tastyigniter/TastyIgniter@c61f8de1)
- Fix issue where staff list is empty (tastyigniter/TastyIgniter@3128a19a)
- Code field on mail layouts and partials should always be unique (tastyigniter/TastyIgniter@b8896856)
- Avoid marking order as processed more than once (tastyigniter/TastyIgniter@723ed8c5)
- Minor UI fix (tastyigniter/TastyIgniter@d3da0a09)
- Minor fix (tastyigniter/TastyIgniter@cf831c80)
- Fixed issue where its impossible to unassign a group or staff (tastyigniter/TastyIgniter@aa539055)
- Fix issue with clockpicker positioning and some minor UI adjustments (tastyigniter/TastyIgniter@303444db)
- Avoid negative order total when calculating totals (tastyigniter/TastyIgniter@f9501d9a)
- Fix issue with managing records when recordeditor is in radio mode (tastyigniter/TastyIgniter@c8ff4870)
sampoyigi added a commit that referenced this pull request Nov 26, 2022
- Remove locked property from composer.json manifest (be92fda8)
- Drop asset minification feature to fix JS issue with admin not able to login and unable to edit mail templates. #970 #948 (b9df4df1)
- Add allergens relation to 3.x branch (#978) (ed16a6d3)
- minor fix (e8d87178)
- Fix location_options issue when creating a location #981 (ccbba690)
- minor fixes (721943ae)
- Remove unique validation rule on allergen, category, location, mealtime, menu and table requests (20a9e629)
- Add permissions to order, reservation delete bulk action #986 (16f24e05)
- Add padding to modal form fields container (bc9ceb12)
- Add branch name to split GA (f70ccc7b)
- New admin.list.extendRecords event (#989) (aeb055b7)
- Minor fix (f4996352)
- Use locale dates formats in order and reservation emails (3f5303e6)
- Minor refactor to assignables (d5fd36cc)
- Add location logo mail variables (d91683e9)
- Move allocate table logic from booking manager to reservation mode (a4530bdd)
- Minor fix (4d9bdeb8)
- Add migration to drop all foreign keys due to irritating foreign key errors (ec0542bd)
- Ensure list column names do not much model relation name (5e84a712)
- Check foreign key/index exists before dropping (bbaa0796)
- Use customer email as replyTo when appropriate (tastyigniter/tastyigniter@#1002) (4635985d)
- Ensure guest_num and duration have minimum values (#1001) (a3f024a0)
- use lowest menu option price when menu item price is 0 (137734cb)
- Add location options as HasMany relation (652cb6e2)
- Fix location list order type filter (68e405b0)
- Add comment column to reservations list tastyigniter/ti-ext-reservation#32 (3016c2fd)
- Sort menu item option dropdown alphabetically (f30bbff6)
- Fix issue where cart item option quantity type calculated value is not properly displayed (f2c500cc)
- Update [README.md](http://readme.md/) (b3732a3f)
- Install core update first instead of ignoring non-core updates (2a754e46)
- Fix broken link (a937a00d)
- Minor fix (6486a89d)
- Fix issue where media finder attachment config modal not opening (#1010) (2c1f30d6)
- Minor fix (5bf7b4e7)
- Cache location options (cfe5f965)
- Make location options fields unique (#1011) (5ec9a233)
- Minor fixes (5cbaced6)
- Add support for locationable trait on HasManyThrough, HasOneThrough relationships (da6bc020)
- Allow rendering of multiple list widgets (ed2a9101)
- Fix dashboard news widget feed url (98bc919d)
- Reorder reservation form fields (895d9949)
- Minor UI fix (5e7fbc53)
- Fix issue where reservation list can not be filtered by multiple statuses (de8ec050)
- Ensure admin password max length is 32 when installing (9ceb0251)
- Minor fix (359687ee)
- Fix dashboard charts not displaying data (80a8ba3a)
- Minor refactor (b780b42c)
- Strip html tags from formatted address (88f9243d)
- Support multiple extension paths (#1017) (cd89efb7)
- Allow payments, themes form query to be extended (c61f8de1)
- Fix issue where staff list is empty (3128a19a)
- Code field on mail layouts and partials should always be unique (b8896856)
- Avoid marking order as processed more than once (723ed8c5)
- Minor UI fix (d3da0a09)
- Minor fix (cf831c80)
- Fixed issue where its impossible to unassign a group or staff (aa539055)
- Fix issue with clockpicker positioning and some minor UI adjustments (303444db)
- Avoid negative order total when calculating totals (f9501d9a)
- Fix issue with managing records when recordeditor is in radio mode (c8ff4870)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants