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.

Sign up for GitHub

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

Jump to bottom

Set the correct length of parameters when constructing the Redis arguments #41838

Merged
merged 1 commit into from
Jul 20, 2024
Merged

Set the correct length of parameters when constructing the Redis arguments #41838

merged 1 commit into from
Jul 20, 2024

Conversation

rubik-cube-man
Copy link
Contributor

Currently the arguments when constructing the PARAMS part of a redis query are incorrect.

queryArgs.param("key", "value") currently results in:

PARAMS 1 key value

However, this change would correct it to construct:

PARAMS 2 key value

I believe the unit tests happen to be passing by pure chance as they are constructing something similar to this at the moment:

PARAMS 2 blob blob_data DIALECT 2

@geoand geoand requested a review from jmartisk July 15, 2024 07:30
@gsmet gsmet requested review from Ladicek and cescoffier July 16, 2024 16:07
cescoffier
cescoffier approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

Should we add a test? I'm wondering why this was not covered in the test suite.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
Ladicek
Ladicek approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I agree a test would be nice.

@cescoffier
Copy link
Member

@rubik-cube-man could you add a test?

@rubik-cube-man
Copy link
Contributor Author

Yeah, not a problem! Is this the kind of test that you had in mind?

gastaldi
gastaldi approved these changes Jul 20, 2024
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 20, 2024
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jul 20, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 3b7f59b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@gastaldi gastaldi merged commit c9d196c into quarkusio:main Jul 20, 2024
31 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 20, 2024
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Jul 20, 2024
@gsmet gsmet modified the milestones: 3.14 - main, 3.13.0 Jul 22, 2024
@gsmet gsmet modified the milestones: 3.13.0, 3.8.6 Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladicek Ladicek Ladicek approved these changes

@cescoffier cescoffier cescoffier approved these changes

@gastaldi gastaldi gastaldi approved these changes

@jmartisk jmartisk Awaiting requested review from jmartisk

Assignees
No one assigned
Labels
area/redis triage/flaky-test
Projects
None yet
Milestone
3.8.6
Development

Successfully merging this pull request may close these issues.
6 participants
@rubik-cube-man @cescoffier @gastaldi @Ladicek @gsmet @quarkusbot