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

Fix: ProcessQueue throws truncated incorrect decimal value #1083

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

xyng
Copy link
Contributor

@xyng xyng commented Jul 8, 2024

Description

This PR provides a test and fix for #1082 - changes the param-type of processId in the prepared statement to string instead of integer, since the database field already uses varchar(50) and ProcessQueueCommand will pass a string.
That can lead to either non-matches (sqlite) or even runtime errors (in one of our environments using MariaDB).

I have

  • Checked that CGL are followed
  • Checked that the Tests are still working
  • Added description to CHANGELOG.md (github-handle is optional)
  • Added tests for the new code

Note

@tomasnorre I tried my best to get the test to reproduce. For some reason, only the SQLite one ran on my machine. For SQLite, there is no runtime error, but non-numeric values are not matched, leading the test case to fail the count-check which is somewhat sufficient to evaluate the problem.
I'd appreciate if you could check the MySQL and PostgreSQL derivates if they work correctly (either throw and exception or at least fail the count test).

The ProcessQueueCommand will pass a string for processId, which is stored as varchar.
So the test should use a value that is a string with non-number characters.
@tomasnorre
Copy link
Owner

tomasnorre commented Jul 8, 2024

@xyng Thank you so much for your contribution. Looks good at first glance. Will have the CI check the MySQL, MariaDB and PostgreSQL for us ;)

There are some code-style, use statement sorting not right atm, but I think that is due to a rector update that isn't caused by you. So don't worry about those if the pipeline turns red.

Update: Tests looks good in MySQL, MariaDB and PostgreSQL.

Could you please update the PR to include a line in the CHANGELOG.md too ?

processId is stored as varchar and stores strings.
The SQL-Parameter is marked as integer, which can cause missed matches or runtime errors.

Resolves: tomasnorre#1082
@xyng
Copy link
Contributor Author

xyng commented Jul 9, 2024

@tomasnorre thank you for checking and your feedback!
I have added an entry to the changelog - not entirely sure about the wording.

@tomasnorre
Copy link
Owner

@tomasnorre thank you for checking and your feedback! I have added an entry to the changelog - not entirely sure about the wording.

The wording is fine. :)

@tomasnorre tomasnorre merged commit 42b99da into tomasnorre:main Jul 9, 2024
31 of 34 checks passed
@tomasnorre
Copy link
Owner

@xyng Thanks for your contribution, cannot say when it will be released.

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