Fix CleanupExpiredMessages Error (#4810)

[faith0831]

This change is 

[faith0831]

@faith0831 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@dotnet-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

[sfmskywalker]

Hello @faith0831 , thank you for submitting a PR. Can you explain what the issue is and how this PR fixes that? I noticed that it removes the use of the pagination args, which might have other consequences.

[mohdali]

Hello @faith0831 , thank you for submitting a PR. Can you explain what the issue is and how this PR fixes that? I noticed that it removes the use of the pagination args, which might have other consequences.

@sfmskywalker It is related to #4810 for MySQL as LIMIT is not supported in sub queries. However, I'm not sure if removing the pagination is the way to go as it seems to be possible to replace the subquery with a LEFT JOIN. I didn't get the chance to test that though.

@faith0831 is it possible to use a LEFT JOIN instead of getting rid of pagination completely?

[sfmskywalker]

@mohdali I see, thanks for the clarification. We shouldn't drop paging support because it might negatively impact other users that use other database servers and where doing a batched deletion of workflow inbox messages is preferred (in case of high numbers of records).

Additionally, if the paging SQL clauses cause an issue for MySQL for the WorkflowInbox table, it will also be an issue for all other DB tables, which means that this PR might "fix" one affected area, it will not fix the root issue.

[faith0831]

Hello @faith0831 , thank you for submitting a PR. Can you explain what the issue is and how this PR fixes that? I noticed that it removes the use of the pagination args, which might have other consequences.

@sfmskywalker It is related to #4810 for MySQL as LIMIT is not supported in sub queries. However, I'm not sure if removing the pagination is the way to go as it seems to be possible to replace the subquery with a LEFT JOIN. I didn't get the chance to test that though.

@faith0831 is it possible to use a LEFT JOIN instead of getting rid of pagination completely?

@mohdali Thank you for your assistance, I will continue to try to solve this problem.

[faith0831]

@mohdali I see, thanks for the clarification. We shouldn't drop paging support because it might negatively impact other users that use other database servers and where doing a batched deletion of workflow inbox messages is preferred (in case of high numbers of records).

Additionally, if the paging SQL clauses cause an issue for MySQL for the WorkflowInbox table, it will also be an issue for all other DB tables, which means that this PR might "fix" one affected area, it will not fix the root issue.

Thank you for your reply @sfmskywalker, this PR indeed has the issues you pointed out.

[sfmskywalker]

@faith0831 @mohdali Should we keep this PR open, change it to Draft, or abandon?

[mohdali]

@sfmskywalker we need to change how the MySql query for clearing the inbox is executed. I didn't get the time to check the possible solution. I think this PR can be changed to draft.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[mohdali]

@sfmskywalker In order to resolve this issue we need to change the DeleteManyAsync query specifically for MySQL to avoid using pagination. I think it is still possible to batch the deletion in MySQL but needs to be done using a subquery instead.

In that case, what is the recommended approach? Show we create a new IWorkflowInboMessageStore in Elsa.EntitiyFrameworkCore.MySql and let it hide the default store? We only need to add that store if the Workflow Inbox feature is enabled though (not sure what is the correct pattern here).

I think in general there will be some exceptions like this depending on each DB capabilities, we need to have a clear pattern to handle those cases.