Query actions with partial match

[alexmigf]

Hello,

I belong to the WP Overnight team and we have a plugin that uses the Action Scheduler for scheduling emails.

The args column inside the wp_actionscheduler_actions table has data saved has JSON array, and with the current query is not possible to query for matches inside a multidimensional array.

Because our plugin could greatly benefit from this addition, we decided to extend your ActionScheduler_DBStore class and perform additional changes to the get_query_actions_sql method. Our plugin is a WooCommerce plugin, and since WC bundles its own version of Action Scheduler, loading a modified class using action_scheduler_store_class is a potential source of conflicts. This solution also could benefit other plugins too, that's why we think it makes sense to implement this into the Action Scheduler core.

We added a new query argument args_partial_match to indicate if we want to use the partial match for the args or keep using the default. For the partial match we are using the MySQL JSON_EXTRACT function if the database version is superior to 5.7, and using the LIKE operator if the version is inferior.

// Default query (we have to match all key value pairs)
$args = array(
	'per_page' => -1,
	'hook' => 'wpo_wcsre_send_emails',
	'args' => array(
		'order_id' => 80,
		'email_id' => 81,
	)
);
$query = as_get_scheduled_actions($args);

// With the new partial match query (we can match only one key value pair)
$args = array(
	'per_page' => -1,
	'hook'=> 'wpo_wcsre_send_emails',
	'args'=> array(
		'email_id'=> 81,
	),
	'args_partial_match'=> true
);
$query = as_get_scheduled_actions($args);

Feel free to comment or suggest modifications.

[thenbrent]

since WC bundles its own version of Action Scheduler, loading a modified class using action_scheduler_store_class is a potential source of conflicts. This solution also could benefit other plugins too, that's why we think it makes sense to implement this into the Action Scheduler core.

@alexmigf thank you for both recognizing the risk of conflicts shipping this outside of AS itself, and contributing upstream to the library. Both are greatly appreciated to help improve everyone's experience with AS.

After a quick review, this looks fine to me. The concerns I have around adding this type of functionality have already been accounted for in the patch by using the new args_partial_match param.

I'll let @rrennick do proper review though before it's merged.

[rrennick]

@alexmigf Could you provide an example $args with the code snippet you are using that doesn't work with the current datastore but does work with this PR?

Edit: I apologize. I see that you have provided that.

[rrennick]

@alexmigf Nice work on this one. I do see two issues

• This only works in the custom table data store. If the partial match logic was moved to the base data store then any datastore using json_encode() on $args could take advantage of it.
• It only works when the length of json_encoded $args is 191 characters or less in the custom table data store.

[alexmigf]

@alexmigf Nice work on this one. I do see two issues

* This only works in the custom table data store. If the partial match logic was moved to the base data store then any datastore using `json_encode()` on `$args` could take advantage of it.

* It only works when the length of json_encoded `$args` is 191 characters or less in the custom table data store.

I just keep the same structure already in place in this array $query = wp_parse_args( $query, array(...) );, and also the database table column data_type remains the same. I'm assuming this is a limitation for this feature.

[rrennick]

If the partial match logic was moved to the base data store then any datastore using json_encode() on $args could take advantage of it.

@alexmigf The base data store is https://github.com/woocommerce/action-scheduler/blob/master/classes/abstracts/ActionScheduler_Store.php . The current implementation in the PR only applies to the included tables data store. If you aren't comfortable with splitting the logic across the two data stores, let us know.

[alexmigf]

@rrennick I'm not quite sure what you mean by:

splitting the logic across the two data stores

Are you referring to this? https://github.com/woocommerce/action-scheduler/blob/master/classes/data-stores/ActionScheduler_wpPostStore.php

Because i believe wpPostStore was deprecated for core usage.

Let me know your thoughts.

[rrennick]

Because i believe wpPostStore was deprecated for core usage.

The tables data store requires PHP 5.5. Also, there are sites using custom data stores. This is a great enhancement but the base store needs to support the parameter with fallback behaviour. Then both the included stores and custom stores can take advantage of that logic.

[alexmigf]

@rrennick The tables data store correspond to the DBStore and the base store to wpPostStore, is that correct?

[rrennick]

and the base store to wpPostStore

Base store class is https://github.com/woocommerce/action-scheduler/blob/master/classes/abstracts/ActionScheduler_Store.php.

[Spreeuw]

@rrennick I'm not sure I'm missing something here, but the class that you're linking is the abstract, and as such doesn't implement anything related to the processing of the parameters itself (nor mentions any of the existing parameters). This pull request only adds some additional logic to the get_query_actions_sql method (something that might not even be relevant to custom data stores, depending on how they handle/store the action parameters), it doesn't change anything to how the existing parameters are processed. I'd love to hear your thoughts on this!

[dan-lutd]

@rrennick and @alexmigf I will test this for my simple use case.

[Konamiman]

Hi @rrennick, this one is quite old, do you think it would be ready to merge once the conflicts are resolved or is there anything else that should be changed?

Also @alexmigf, could you please fix the conflicts and add some tests for the added functionality?

Edit: I see now that @rrennick requested a change that hasn't been implemented, and which apparently requires further clarification. To start with, @alexmigf are you still interested in this being eventually merged?

[rrennick]

@Konamiman Thanks for the ping. The enhancement in this PR looks like it could benefit quite a few use cases. Feel free to follow through with this one as you see fit.

[jorgeatorres]

Hi @alexmigf!

Thanks for your contribution. We understand it's been a while since you last worked on this, so please let us know if you're still interested in seeing this through (reiterating the question asked here). We would love to merge this PR, but it would require a a little work before we can do that.

• First of fall, a rebase is needed to fix conflicts.

• There's also some feedback I've added in the code itself.

• Finally, we're concerned about the LIKE implementation working differently from the JSON-based one (see example below) and wonder whether we should drop the LIKE implementation altogether and throw an exception or directly require MySQL 5.7 in AS. Alternatively, we could support different types of matching, for example, by allowing different values for args_partial_match such as 'json' and 'like' instead of it being a boolean. This would at least make the behavior you get explicit (and not depend on which MySQL version you have installed). What are your thoughts on this?

  Example: LIKE vs JSON_EXTRACT differences in behavior

  An entry with, say, JSON args such as {"test":{"email":"myemail"}}, would not be picked up by the JSON_EXTRACT implementation if passed 'email' => 'myemail' as parameter (because 'email' is not a top-level property) but it'd be returned by the LIKE implementation, which is not really expected.

Thanks again!

[alexmigf]

@jorgeatorres @Konamiman sure, I just need some time to get back to this.

[alexmigf]

@jorgeatorres

Finally, we're concerned about the LIKE implementation working differently from the JSON-based one (see example below) and wonder whether we should drop the LIKE implementation altogether and throw an exception or directly require MySQL 5.7 in AS. Alternatively, we could support different types of matching, for example, by allowing different values for args_partial_match such as 'json' and 'like' instead of it being a boolean. This would at least make the behavior you get explicit (and not depend on which MySQL version you have installed). What are your thoughts on this?

To make the implementation more clear to everyone dropping LIKE would be ideal, but let me know if you are able to change the MySQL version requirement or if you prefer both ways.

[joppuyo]

I would be interested in seeing the LIKE-based implementation since in my case I don't have nested keys and I would like the ensure the broadest possible compatibility. But maybe it should be an opt-in feature along with a note in the readme about the limitations of this method to prevent any surprises for the users of the library.

According to the wordpress.org statistics, about half the sites are still running a version under 5.7.

[jorgeatorres]

Hi @alexmigf! 👋

Thanks for getting back to me. We discussed this a bit more on the team, and considering the LIKE implementation works everywhere we feel it shouldn't be dropped, but we should instead support both approaches.

We propose this moves forward in this way: we rename the query arg as partial_args_matching to better convey what it does, but instead of it being either true or false, it'd support 3 possible values:

• 'off' which would mean no partial matching occurs and would result in the exact same behavior we currently have. This value will be the default (i.e. the same as not setting partial_args_matching at all).
• 'like' will engage the LIKE implementation.
• 'json' will engage the JSON_*() methods implementation and will check the db server version, throwing an exception if < 5.7.

For any other value outside of those 3 we will either throw an exception or default to 'off', which is something we can decide later during PR review. With those 3 options (and a default value) we should be able to handle all the 3 cases in a switch or similar, maybe simplifying the code a bit.

We feel this would allow us to reach a wider audience and for use cases that need the specialized JSON version (such as yours), that will be available too.

Some examples:

as_get_scheduled_actions(
	[
		'per_page'              => -1,
		'args'                  => [
			'email_address' => '[email protected]',
		],
		'partial_args_matching' => 'like',
	]
);

as_get_scheduled_actions(
	[
		'per_page'              => -1,
		'args'                  => [
			'email_address' => '[email protected]',
		],
		'partial_args_matching' => 'json',
	]
);

// This would be the same as passing `'partial_args_matching' => 'off'` as arg.
as_get_scheduled_actions(
	[
		'per_page'              => -1,
		'args'                  => [
			'email_address' => '[email protected]',
		],
	]
);

Please let me know what your thoughts are on this suggested approach. Also, let me reiterate our appreciation for the work you're doing and for picking this up again after quite a while. 💯

[alexmigf]

@jorgeatorres review requested and thank you for the valuable inputs.

[alewolf]

This has not been added to the documentation yet. Is there a particular reason? Would you accept a PR that adds it to the docs?

[jorgeatorres]

This has not been added to the documentation yet. Is there a particular reason? Would you accept a PR that adds it to the docs?

Hey @alewolf! There isn't any particular reason. We will be happy to review a PR that adds some documentation on this feature.