Limit index length to 191 characters by default, additionally connect HPOS to verify DB tooling. #39250

vedanshujain · 2023-07-14T10:11:51Z

Additionally, connect verify db tooling to order tables when they are enabled.

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This PR introduces two changes:

It limited the index length to 191 characters as we do for other WooCommerce schema. This is needed because at least in the MyISAM engine, the maximum index size can only be 1000 bytes, which is occupied by 192 chars with utf8_mb4 encoding.
It links the already existing verify_db_tool with the HPOS schema if it's enabled (or if data sync is on). This tools alerts the site admin if a table is missing.

Closes #39051

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Start with a site with posts authoritative and sync disabled. Delete the HPOS tables if they already exist by going to WooCommerce > Status > Tools and running the Delete the custom orders tables tool.
Add this filter to simulate the error first:

add_filter( 'woocommerce_database_max_index_length', function () {
	return 320; // will cause error on MyISAM but works fine on InnoDB.
} );

We will now modify the code to create one of the tables with MyISAM which will simulate the error. Edit this file: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php#L2664 so that it now says ) $collate ENGINE = MyISAM; instead of just ) $collate ;.
Once the file is edited, let's try to create the table. Go to WooCommerce > Settings > Advance > Features and enable the option to keep posts and orders in sync. This will create all tables but the meta table, which will fail with error: Specified key was too long; max key length is 1000 bytes.
Go to WooCommerce > Status > Tools and run the Verify base database tables tool. It will identify the missing table and show this error: Verifying database... One or more tables are still missing: wp_wc_orders_meta.
Now, remove the filter that we added in Step 2. Go to WooCommerce > Settings > Advance > Features again and enable/disable the sync feature. This will trigger the table creation routine again.
Come back to WooCommerce > Status > Tools and run the Verify base database tables tool. It should show that all tables are present as expected.

Changelog entry

Automatically create a changelog entry from the details below.

Significance

Patch
Minor
Major

Type

Fix - Fixes an existing bug
Add - Adds functionality
Update - Update existing functionality
Dev - Development related task
Tweak - A minor adjustment to the codebase
Performance - Address performance issues
Enhancement

Message

Comment

github-actions · 2023-07-14T10:43:44Z

Test Results Summary

Commit SHA: 6d11563

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 53s
E2E Tests	189	0	0	19	0	208	14m 33s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

github-actions · 2023-07-14T12:59:11Z

Hi @rrennick,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions · 2023-07-14T12:59:12Z

Hi @rrennick,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Additionally, connect verify db tooling to order tables when they are enabled.

rrennick · 2023-07-14T16:44:50Z

This will create all tables but the meta table, which will fail with error: Specified key was too long; max key length is 1000 bytes.

@vedanshujain Should we update this message to reflect the limit imposed by the filter?

rrennick

Code looks good but I didn't test it yet. I left that for after resolving the discussion points.

rrennick · 2023-07-14T16:53:58Z

plugins/woocommerce/src/Internal/DataStores/Orders/CustomOrdersTableController.php

@@ -366,6 +366,7 @@ private function handle_data_sync_option_changed( string $feature_id ) {

 		if ( ! $this->data_synchronizer->check_orders_table_exists() ) {
 			$this->data_synchronizer->create_database_tables();
+


This looks accidental.

rrennick · 2023-07-14T16:56:47Z

plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php

@@ -2584,6 +2584,23 @@ public function get_database_schema() {
 		$operational_data_table_name = $this->get_operational_data_table_name();
 		$meta_table                  = $this->get_meta_table_name();

+		/**


Would it be worth adding this filter in its own function in a utility/helper class as we may find more uses for it as we work through some of the other upcoming data storage projects?

rrennick · 2023-07-31T15:46:59Z

@vedanshujain Not for this PR but there are couple small things I noticed:

When the meta table was missing the delete tables tool wasn't listed in the tools even though the other tables were
The sync went ahead whether or not the meta table was present. It would be better if it was stopped and the merchant had a notification of some kind

rrennick

Nice work on this @vedanshujain . It tested great for me.

nigeljamesstevenson · 2023-07-31T20:48:29Z

Thanks a lot for the input on this @rrennick - much appreciated! @vedanshujain, do you want us to create Issues for the suggestions that Ron has made?

coreymckrill · 2023-08-01T00:13:20Z

@vedanshujain I just noticed that WC is giving a warning that WC Smooth Generator is not compatible with HPOS, even though it has been declaring compatibility for quite a while now. I used git bisect to trace the change to this PR. From the changeset I don't immediately see what could be causing this, but wanted to give a heads up, as this could be affecting other extensions as well.

rrennick · 2023-08-01T16:58:36Z

I used git bisect to trace the change to this PR. From the changeset I don't immediately see what could be causing this

It's not registered as compatible with the two new features added in this PR.

In #38993, the `custom_order_tables` feature was split into two separate features, one for choosing which tables to use, and one for toggling data synchronization. My understanding is that this was done in order to have both of those settings appear on the Features settings page, instead of having to create a separate page for other HPOS-specific settings. However, this added some extra complexity, which then led to #39250 causing a regression where extensions that declared compatibility with HPOS were no longer recognized as compatible. With this PR, we're reverting back to only having one "feature" for HPOS, but introducing a way to add additional setting controls to the Features settings page.

This is a piece of a change that is already in trunk, but it needs to be backported to 8.0 (see #39250). This ensures that the feature ID and the option key match for each of the two HPOS-related features that were introduced in #38993.

* Ensure FeaturesController is aware of correct HPOS option keys This is a piece of a change that is already in trunk, but it needs to be backported to 8.0 (see #39250). This ensures that the feature ID and the option key match for each of the two HPOS-related features that were introduced in #38993. * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <github-actions@github.com>

In #38993, the `custom_order_tables` feature was split into two separate features, one for choosing which tables to use, and one for toggling data synchronization. My understanding is that this was done in order to have both of those settings appear on the Features settings page, instead of having to create a separate page for other HPOS-specific settings. However, this added some extra complexity, which then led to #39250 causing a regression where extensions that declared compatibility with HPOS were no longer recognized as compatible. With this PR, we're reverting back to only having one "feature" for HPOS, but introducing a way to add additional setting controls to the Features settings page.

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jul 14, 2023

vedanshujain marked this pull request as ready for review July 14, 2023 12:58

vedanshujain requested review from a team and rrennick and removed request for a team July 14, 2023 12:58

vedanshujain added 2 commits July 14, 2023 18:38

Limit index length to 191 characters by default.

c2590f2

Additionally, connect verify db tooling to order tables when they are enabled.

Coding standard fixes + changelog.

1833c74

vedanshujain force-pushed the fix/39051 branch from f2bafe0 to 1833c74 Compare July 14, 2023 13:08

rrennick reviewed Jul 14, 2023

View reviewed changes

Refactored filterd in the DB util for DRY.

6d11563

rrennick approved these changes Jul 31, 2023

View reviewed changes

rrennick added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Jul 31, 2023

nigeljamesstevenson added needs: internal testing Indicates if the PR requires further testing conducted by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Jul 31, 2023

nigeljamesstevenson merged commit fa2aba5 into trunk Jul 31, 2023
22 of 23 checks passed

nigeljamesstevenson deleted the fix/39051 branch July 31, 2023 20:48

github-actions bot added this to the 8.1.0 milestone Jul 31, 2023

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Jul 31, 2023

tammullen removed the needs: analysis Indicates if the PR requires a PR testing scrub session. label Aug 1, 2023

coreymckrill mentioned this pull request Aug 1, 2023

HPOS Features: Revert to one feature #39525

Merged

11 tasks

coreymckrill mentioned this pull request Aug 2, 2023

Ensure FeaturesController is aware of correct HPOS option keys #39551

Merged

11 tasks

vedanshujain mentioned this pull request Aug 21, 2023

WooCommerce Activation Database Error #39722

Closed

5 tasks

barryhughes mentioned this pull request Aug 31, 2023

activate hpos create an database error 1000byte exeeded #39990

Closed

5 tasks

nqkyf mentioned this pull request Sep 13, 2023

WordPress database error Specified key was too long; max key length is 1000 bytes for query on WAMP #34849

Closed

5 tasks

Poocey mentioned this pull request Sep 13, 2023

HPOS SQL Code Too Large #39051

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit index length to 191 characters by default, additionally connect HPOS to verify DB tooling. #39250

Limit index length to 191 characters by default, additionally connect HPOS to verify DB tooling. #39250

vedanshujain commented Jul 14, 2023 •

edited

github-actions bot commented Jul 14, 2023 •

edited

github-actions bot commented Jul 14, 2023

github-actions bot commented Jul 14, 2023

rrennick commented Jul 14, 2023

rrennick left a comment

rrennick Jul 14, 2023

rrennick Jul 14, 2023 •

edited

rrennick commented Jul 31, 2023

rrennick left a comment

nigeljamesstevenson commented Jul 31, 2023

coreymckrill commented Aug 1, 2023

rrennick commented Aug 1, 2023

		@@ -366,6 +366,7 @@ private function handle_data_sync_option_changed( string $feature_id ) {

		if ( ! $this->data_synchronizer->check_orders_table_exists() ) {
		$this->data_synchronizer->create_database_tables();

Limit index length to 191 characters by default, additionally connect HPOS to verify DB tooling. #39250

Limit index length to 191 characters by default, additionally connect HPOS to verify DB tooling. #39250

Conversation

vedanshujain commented Jul 14, 2023 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

github-actions bot commented Jul 14, 2023 • edited

Test Results Summary

github-actions bot commented Jul 14, 2023

github-actions bot commented Jul 14, 2023

rrennick commented Jul 14, 2023

rrennick left a comment

Choose a reason for hiding this comment

rrennick Jul 14, 2023

Choose a reason for hiding this comment

rrennick Jul 14, 2023 • edited

Choose a reason for hiding this comment

rrennick commented Jul 31, 2023

rrennick left a comment

Choose a reason for hiding this comment

nigeljamesstevenson commented Jul 31, 2023

coreymckrill commented Aug 1, 2023

rrennick commented Aug 1, 2023

vedanshujain commented Jul 14, 2023 •

edited

github-actions bot commented Jul 14, 2023 •

edited

rrennick Jul 14, 2023 •

edited