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

Migrations fails because args too long #418

Closed
tillkruss opened this issue Jan 9, 2020 · 24 comments · Fixed by #455
Closed

Migrations fails because args too long #418

tillkruss opened this issue Jan 9, 2020 · 24 comments · Fixed by #455
Milestone

Comments

@tillkruss
Copy link
Contributor

tillkruss commented Jan 9, 2020

How are we suppose to migrate actions to 3.0 with "long" arguments?

None of our actions can be migrated because:

Error saving action: Error saving action: ActionScheduler_Action::$args too long. To ensure the args column can be indexed, action args should not be more than 191 characters when encoded as JSON.

An example of our args:

'data' => array ('opf' => '300', 'firstname' => 'John Doe', 'email' => 'user@example.com', 'page' => 'https://example.com/programs/foobar/', 'user-agent' => 'Mozilla/5.0 (Linux; Android 5.1; A1601 Build/LMY47I) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.98 Mobile Safari/537.36', 'ip' => '110.54.151.154', )
@tillkruss tillkruss changed the title Args too long Migrations fails because args too long Jan 9, 2020
@tillkruss
Copy link
Contributor Author

Also, if the argument length is limited how is meta data to be stored properly using v3?

@thenbrent
Copy link
Contributor

Hi @tillkruss

if the argument length is limited how is meta data to be stored properly using v3?

There's an answer to that here: #199

How are we suppose to migrate actions to 3.0 with "long" arguments?

You can learn more about this issue here: #401 and #402.

@tillkruss
Copy link
Contributor Author

tillkruss commented Jan 16, 2020

@thenbrent: This seems very poorly designed.

There is no meta column on the wp_actionscheduler_actions table. So where are we suppose to store action arguments or meta data?

Transients should be temporary, options would spam the wrong table with meta data.

I guess we could store them in Redis, but that's completely disconnected from WordPress.

Could we add another column to the table for meta data?

How can I use the existing migrate command, but move the args to post meta values? Is there a filter for that?

@rrennick
Copy link
Contributor

So where are we suppose to store action arguments or meta data?

You could create a table for the meta data associated with the action. Then pass the key field as the args for your action.

How can I use the existing migrate command, but move the args to post meta values? Is there a filter for that?

No, you would need to move the data first then allow the migration to proceed.

@tillkruss
Copy link
Contributor Author

Is there any interest in improving the design of action scheduler, for example by adding a metadata as longtext column to the wp_actionscheduler_actions table?

@thenbrent
Copy link
Contributor

adding a metadata as longtext column to the wp_actionscheduler_actions table

If that had sufficient requests from developers, we'd consider it.

In the meantime, you can add it for your needs in a plugin that creates a custom data store.

If you release that plugin publicly and make it available for others to use, and it then proves popular, we'd also consider merging that into AS core.

At the moment, despite the deprecated arg length notice being around for almost 18 months, we're only aware of two requests for longer args, which doesn't warrant inclusion in AS core.

@front-stream
Copy link

Would be good to add this feature. For example I cannot pass wp user object.

@rrennick
Copy link
Contributor

For example I cannot pass wp user object.

One of the main functions of Action Scheduler is to offload processing to the background. Storing entire objects instead of the IDs to those objects somewhat defeats that purpose. Storing entire objects also makes searching for an existing action to determine whether an action should be scheduled much more resource intensive.

@thenbrent
Copy link
Contributor

@front-stream thanks for sharing a vote though. We've got 3 requests for such a feature now.

@anderly
Copy link

anderly commented Jan 29, 2020

@thenbrent I'd second @tillkruss' request for a standard way of doing this. Example, I'm using AS for storing WooCommerce product and order-specific requests that are based on individual product types (simple product, WooCommerce subscriptions, as well as product variations) that have state that warrants more than a single ID or couple of simple args. A meta column would do the trick and I'd rather not build a separate plugin for this or custom tables. I'd also rather not store it with the order itself in postmeta since my data can vary depending on the subscription status. So, I really need to capture a point-in-time snapshot of some order meta, product and variation meta combined with subscription meta into a unique request.

Basically, I'd like to be able to call AS with arguments (small or in some cases larger) and be able to fire and forget without having to come up with a custom state-management solution for the parameters I want my function to receive when it runs.

One possibility would be hashing the args to make indexing possible and detecting dupes possible, while still retaining the unhashed args in a longtext column for ease-of-use. Doesn't have to be super secure so a simple md5 or sha1 hash would work.

Example:

$args = [ 
  'param1' => 'Param 1', 
  'param2' => 'Param 2', 
  'param3' => [ 
    'param3.1' => 'Nested 1', 
    'param3.2' => 'Nested 2',
    'param3.3' => 'Nested 3',
    'param3.4' => 'Nested 4',
    'param3.5' => 'Nested 5',
    'param3.6' => 'Nested 6',
  ], 
  'param4' => 'Param 4', 
  'param5' => 'Param 5', 
  'param6' => 'Param 6', 
  'param7' => 'Param 7',
  'param8' => 'Param 8',
  'param9' => 'Param 9',
  'param10' => 'Param10',
];

// Returns "f8b059c967eede8984c96dc3da451c4f"
$args_key = md5( json_encode( $args ) );

// Use $args_key as indexable key in `wp_actionscheduler_actions` table.
// Store json_encoded or serialized version of args in meta long text column.

Just wanted to lend an upvote and explain that it would be useful.

Thanks for considering!

@thenbrent
Copy link
Contributor

@anderly thanks for the vote. We're still unlikely to add this into AS core, but @rrennick and I did talk about making a separate piece of code available that implements a custom data store and adds the meta column. That code could then be used as an addon library for AS in cases where it's needed.

Would that be suitable for those here who have requested this feature?

@anderly
Copy link

anderly commented Jan 30, 2020

@thenbrent that sure would be helpful. Would love to see it in AS core, but that's a close second!

Thanks again!

@miklb
Copy link

miklb commented Feb 3, 2020

howdy! I just walked into a WooCommerce project and have encountered this same issue. If anyone has some hack code I could use to get things back up to bypass this I'll forever owe you coffee.

Obviously I'll be monitoring on how to best long term implement a solution.

@miklb
Copy link

miklb commented Feb 3, 2020

quick follow up, on my end, it turned out to be a marketing (Unific) plugin causing Action Scheduler to choke. Disabling it returned me to normal processing. YMMV
We're going to pass the info upstream of course.

@anderly
Copy link

anderly commented Feb 3, 2020

@miklb,

I went ahead and implemented the following temporary solution to store my AS args with the WC order in wp_postmeta and later remove it:

    $as_args = array(
        'arg1' => 'arg1_value',
        'arg2' => 'arg2_value'
    );

    $hash = md5( json_encode( $as_args ) );
    update_post_meta( $order_id, $hash, $as_args );

    // Fire Action Scheduler
    as_schedule_single_action( time(), 'my_as_hook', array( $order_id, $hash ), 'namespace' );

    // Later in my hook handler
    public function my_as_hook_handler( $order_id, $hash ) {
        $as_args = get_post_meta( $order_id, $hash, true );
        $arg1 = $as_args['arg1];
        $arg2 = $as_args['arg2];
        delete_post_meta( $order_id, $hash );

        // Do some work with $arg1 and $arg2....
    }

@peterfabian
Copy link
Contributor

peterfabian commented Feb 5, 2020

I just wanted to note this is also an issue when WebHooks in WC are tied to an Action, as some actions pass around objects, it can fail to run on those. It's perhaps an esoteric use case, but this will become broken with WC + AS 3.
Screenshot 2020-02-05 at 22 35 59
Screenshot 2020-02-05 at 22 31 18

@thenbrent
Copy link
Contributor

I'm going to reopen this issue and we can look at finding a solution, ideally before AS 3.0 ships with WC 4.0 @rrennick.

@thenbrent thenbrent reopened this Feb 6, 2020
@tillkruss
Copy link
Contributor Author

The easiest fix would be removing the index on the column and any kinda of sorting on table column, no?

@thenbrent
Copy link
Contributor

@tillkruss it's not just an index anymore, it's the new table's column limit now. We could just change the table's schema and update the table. We'd then end up with the same issues we had previously though - worse performance for all actions, and/or add an index and potentially find the wrong action.

I'd prefer to have some kind of meta storage column used only by those actions that actually need it.

@anderly
Copy link

anderly commented Feb 6, 2020

@thenbrent,

Any thoughts on the idea of hashing the args and using that as the table key to index on?

Hashing of args is commonly used to prevent API replay attacks and seems like would be a good fit for preventing duplicate actions from being queued with same args.

Using sha256 would be best, but longest and slower than sha1 or md5. Still only need 64 chars to store sha256.

Then, you could keep args as json encoded/serialized in a new meta field.

Let me know what you think.

@rrennick
Copy link
Contributor

rrennick commented Feb 6, 2020

Let me know what you think.

Thanks for the suggestion. I'm working on something along those lines.

@thenbrent
Copy link
Contributor

thenbrent commented Feb 7, 2020

@anderly yeah hashing the args is a good idea, thanks for sharing it!

seems like would be a good fit for preventing duplicate actions from being queued with same args.

@rrennick I suspect you're already aware, but just in case, we don't prevent scheduling actions with same args, so there shouldn't be any UNIQUE constraints on the arg hashes (if the hases are stored in the existing args column, which I suspect they will be, we don't need to worry about that).

@rrennick
Copy link
Contributor

rrennick commented Feb 7, 2020

we don't prevent scheduling actions with same args

Right. We do support finding actions with specific arguments. That won't interfere with hashing because we can hash the find $args and use the hash to match the $args column. The only place AS uses a wildcard search of the args is the dashboard list table. Using a has will make that a bit slow for actions that have large parameter arrays. That should be okay for some degradation of performance.

@UhriG
Copy link

UhriG commented Feb 8, 2023

Hi there, I solved this issue encoding and compressing the data.

 $this->batch[$i] = array(
                'user_id' => $user_id,
                'user_avatar_url' => $user_avatar_url
            );
            
            
            if ($i > 0 && $i % $this->batch_size == 0 ) {
                as_schedule_single_action(time(), $this->as_hook, array('batch' => base64_encode(gzcompress(serialize($this->batch), 9))), $this->as_group);

                $this->batch = [];
            }
            
  // Hook          
public function download_avatar_batch($batch)
    {
        $batch = unserialize(gzuncompress(base64_decode($batch)));
        foreach ($batch as $user) {
            $this->download_avatar($user['user_id'], $user['user_avatar_url']);
        }
    }

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 a pull request may close this issue.

8 participants