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 id conversion issue in delayed_sidekiq strategy #964

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

sundus-y
Copy link
Contributor

@sundus-y sundus-y commented Sep 4, 2024

Ensure ids extracted from Redis remain strings, preventing UUID issues. Previously, ids were being converted to integers, causing problems with UUIDs in the delayed_sidekiq strategy.

This update also enhances the test suite:

  • Existing tests are updated.
  • A new test ensures the issue is resolved.

Due to SQLite's lack of UUID support, a stub_uuid_model method is added. This method stubs models with UUIDs, using SecureRandom.uuid for the primary key.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

@sundus-y sundus-y force-pushed the delayed-sidekiq-uuid branch from 9f52020 to 79c3e8e Compare September 4, 2024 16:12
@sundus-y
Copy link
Contributor Author

@toptal-anvil hi, is there anything I'm missing to get this PR reviewed?

@toptal-anvil
Copy link

The provided command hi, is there anything I'm missing to get this PR reviewed? for #964 is not supported. Supported commands:

  • ping reviewers
  • ping author
    Please use the correct one.

@sundus-y
Copy link
Contributor Author

@toptal-anvil ping reviewers

@toptal-anvil-bot
Copy link

Anvil could not notify anyone, because there are no requested reviews for #964
Please request review from at least one team or member and use the command again.

@AlexVPopov
Copy link
Contributor

Hello @sundus-y 👋 🙂 ,

Thank you for your PR. We'll review it and get back to you.

create_locations_table
create_comments_table
create_users_table
end
Copy link

Choose a reason for hiding this comment

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

could elaborate why this is needed? Simply outdated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed a new table with uuid to test my changes so I created the users table. Then having every table creation inside a single block casued Rubocop Metrics/BlockLength error. To fix it I moved each table creation to an individual method.

Copy link

Choose a reason for hiding this comment

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

you could simply disable rubocop rule, but thanks for slight refactoring 👍

@konalegi
Copy link

konalegi commented Oct 8, 2024

hey @sundus-y thanks for you PR!
Could you please rebase and push your changes once again so I can trigger CI?
Overall PR looks good, let's make sure that specs are passing.

Sundus Yousuf added 2 commits October 8, 2024 12:38
Ensure ids extracted from Redis remain strings, preventing UUID issues.
Previously, ids were being converted to integers, causing problems
with UUIDs in the `delayed_sidekiq` strategy.

This update also enhances the test suite:
- Existing tests are updated.
- A new test ensures the issue is resolved.

Due to SQLite's lack of UUID support, a `stub_uuid_model` method is
added. This method stubs models with UUIDs, using `SecureRandom.uuid`
for the primary key.

Move table creations to individual methods.

Having every table creation inside a single block casued Rubocop
`Metrics/BlockLength` error. To fix it I moved each table creation
to an individual method.
@sundus-y sundus-y force-pushed the delayed-sidekiq-uuid branch from 79c3e8e to 1adf3bc Compare October 8, 2024 17:38
@sundus-y sundus-y requested a review from a team as a code owner October 8, 2024 17:38
@konalegi konalegi merged commit 6a2176f into toptal:master Oct 8, 2024
16 checks passed
@konalegi
Copy link

konalegi commented Oct 8, 2024

Thanks!

@sundus-y sundus-y deleted the delayed-sidekiq-uuid branch October 8, 2024 19:36
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.

5 participants