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

[BUGFIX] Replacing the occurrences of gen_range with a safe alternative #1754

Closed
NishantJoshi00 opened this issue Jul 19, 2023 · 12 comments · Fixed by #2126
Closed

[BUGFIX] Replacing the occurrences of gen_range with a safe alternative #1754

NishantJoshi00 opened this issue Jul 19, 2023 · 12 comments · Fixed by #2126
Assignees
Labels
E-easy Effort: Should be easy to implement and would make a good first PR good first issue Good for newcomers help wanted Extra attention is needed

Comments

@NishantJoshi00
Copy link
Member

NishantJoshi00 commented Jul 19, 2023

Description

Following up with the issue rust-random/rand#1326 created by @lsampras. The function gen_range in itself can cause panics and error handling isn't done for some cases. This is a unpredictable behaviour and needs to be replaced with a safe alternative which allows for error handling and recoverability.

Objective

  • Deciding open a safe alternative for gen_range while keeping the behaviour relatively same
  • Replacing the occurrences of gen_range with the decided safe alternative

Guidelines

  • Please mention the approach for solving the above mentioned issue and reasoning of it being recoverable.
@NishantJoshi00 NishantJoshi00 added E-easy Effort: Should be easy to implement and would make a good first PR good first issue Good for newcomers help wanted Extra attention is needed labels Jul 19, 2023
@anupj
Copy link
Contributor

anupj commented Jul 21, 2023

@NishantJoshi00 I'm happy to take this one

@NishantJoshi00
Copy link
Member Author

Hey @anupj, sure I'll assign it you!
If you have any question feel free to drop them here!

@anupj
Copy link
Contributor

anupj commented Jul 23, 2023

@NishantJoshi00 so if I understand the issue correctly. gen_range doesn't return a Result and therefore not ideal to use.
We want to replace it with Uniform or something similar that returns a Result. Is that a fair assessment?

@anupj
Copy link
Contributor

anupj commented Aug 3, 2023 via email

@anupj
Copy link
Contributor

anupj commented Aug 7, 2023

@pixincreate and @NishantJoshi00 i'm actively working on this issue this week so will reach out if I need help. Thanks for your patience.

@anupj
Copy link
Contributor

anupj commented Aug 7, 2023

@NishantJoshi00 after making the changes when I run cargo test --all-features I get the following error messages:

failures:

---- core::api_keys::tests::test_hashing_and_verification stdout ----
thread 'core::api_keys::tests::test_hashing_and_verification' panicked at 'called `Result::unwrap()` on an `Err` value: {"error":{"type":"server_not_available","code":"HE_00","message":"Something went wrong"}}
├╴at crates/router/src/core/api_keys.rs:47:18
├╴Failed to KMS decrypt API key hashing key

├─▶ Failed to KMS decrypt input data
│   ├╴at crates/external_services/src/kms.rs:82:14
│   ╰╴Failed to decrypt KMS value

╰─▶ failed to construct request
    ╰╴at crates/external_services/src/kms.rs:81:14', crates/router/src/core/api_keys.rs:564:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- types::storage::payment_attempt::tests::test_payment_attempt_mandate_field stdout ----
thread 'types::storage::payment_attempt::tests::test_payment_attempt_mandate_field' panicked at 'Failed to decrypt master enc key: Failed to KMS decrypt input data
├╴at crates/external_services/src/kms.rs:82:14
├╴Failed to decrypt KMS value

╰─▶ failed to construct request
    ╰╴at crates/external_services/src/kms.rs:81:14', crates/router/src/services.rs:264:14

---- types::storage::payment_attempt::tests::test_find_payment_attempt stdout ----
thread 'types::storage::payment_attempt::tests::test_find_payment_attempt' panicked at 'Failed to decrypt master enc key: Failed to KMS decrypt input data
├╴at crates/external_services/src/kms.rs:82:14
├╴Failed to decrypt KMS value

╰─▶ failed to construct request
    ╰╴at crates/external_services/src/kms.rs:81:14', crates/router/src/services.rs:264:14


failures:
    core::api_keys::tests::test_hashing_and_verification
    types::storage::payment_attempt::tests::test_find_payment_attempt
    types::storage::payment_attempt::tests::test_payment_attempt_mandate_field

test result: FAILED. 48 passed; 3 failed; 1 ignored; 0 measured; 0 filtered out; finished in 3.24s

Have you come across this before? I'm running all tests for the first time.

@anupj
Copy link
Contributor

anupj commented Aug 7, 2023

Got it, i'll ignore these for now. thanks @pixincreate

@SanchithHegde
Copy link
Member

after making the changes when I run cargo test --all-features I get the following error messages: [...]

Hey @anupj, if the code that you'd like to test is not behind a feature gate, you can just run cargo test.

If it is indeed behind a feature gate and the feature is not one of kms, s3, basilisk, or release, you should still be able to run/test it locally. The mentioned features can only be run/tested in production environments with all the services set up, they involve a significant amount of effort to be set up locally.

@anupj
Copy link
Contributor

anupj commented Aug 8, 2023

Thanks @SanchithHegde When I run cargo test I can move forward but I now get this error message:

failures:

---- types::storage::payment_attempt::tests::test_find_payment_attempt stdout ----
thread 'types::storage::payment_attempt::tests::test_find_payment_attempt' panicked at 'called `Result::unwrap()` on an `Err` value: Timed out while trying to connect to the database
├╴at crates/router/src/connection.rs:118:10

╰─▶ Timed out in bb8
    ╰╴at crates/router/src/connection.rs:117:10', crates/router/src/types/storage/payment_attempt.rs:85:14

---- types::storage::payment_attempt::tests::test_payment_attempt_mandate_field stdout ----
thread 'types::storage::payment_attempt::tests::test_payment_attempt_mandate_field' panicked at 'called `Result::unwrap()` on an `Err` value: Timed out while trying to connect to the database
├╴at crates/router/src/connection.rs:118:10

╰─▶ Timed out in bb8
    ╰╴at crates/router/src/connection.rs:117:10', crates/router/src/types/storage/payment_attempt.rs:130:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    types::storage::payment_attempt::tests::test_find_payment_attempt
    types::storage::payment_attempt::tests::test_payment_attempt_mandate_field

test result: FAILED. 49 passed; 2 failed; 1 ignored; 0 measured; 0 filtered out; finished in 10.17s

Did I miss a step to setup a local database for testing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Effort: Should be easy to implement and would make a good first PR good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
4 participants
@anupj @SanchithHegde @NishantJoshi00 and others