Fix deprecation warnings around RandomStringUtils #2507
Conversation
the `RandomStringUtils.random` method is deprecated since commons-lang 3.17.0 apache/commons-lang@69cb996
| } | ||
|
|
||
| private static String newRandomString(int length) { | ||
| return RandomStringUtils.insecure().next(length, true, true); |
There was a problem hiding this comment.
Replacing something "deprecated" with something "insecure" does not look very nice to me 😅
Also (semi-related), using unseeded random input in tests could make them non-deterministic.
Do we really need random inputs here?
My impression is that we need to ensure that certain chars are permitted or not and the length is restricted. Using deterministic test parameters is probably a more natural approach here.
I understand that this PR merely attempts to avoid deprecation, but since the related test code came to focus here, WDYT about making a more substantial change to make the test deterministic and improve coverage of input cases?
There was a problem hiding this comment.
afaict it is named insecure() to make clear that the source of randomness is not fit for cryptographic purposes.
we can also switch to secure() instead if you prefer?
WDYT about making a more substantial change to make the test deterministic and improve coverage of input cases
we can do that in a followup PR imo, but if we want to improve the coverage compared to randomized input you need to give concrete examples on what inputs you think would "improve" that coverage as for me its not clear.
we can of course make the test inputs deterministic (by picking one randomized sample?) but i dont think this would improve coverage.
There was a problem hiding this comment.
insecure() is technically acceptable for current PR, I think. These names do not have to be cryptographically secure.
There was a problem hiding this comment.
I can only guess what the intention behind the randomized version might have been.
I'd start with names containing ASCII upper/lower case letters and numbers having the the max allowed name length. I think that one case is sufficient for now.
Replacing random name with a fixes name as noted above is not going to reduce the tests' validity, but will improve determinism.
If we want to test other edge cases, it might be preferable to do so in a unit test targeting the validation code (if such validation code exists).
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
* Update dependency io.netty:netty-codec-http2 to v4.2.6.Final (apache#2520) * Update dependency org.testcontainers:localstack to v1.21.3 (apache#2497) * Allow overriding createCatalog calls in integrations tests (apache#2516) This is mostly to add testing flexibility in downstream projects. * Allow `PolarisServerManager` implementations to define custom client headers (apache#2510) `PolarisServerManager` is a plugin point for downstream builds to define runtime env. for tests under `integration-tests`. This change allows more flexibility for test runtime environments by allowing injecting extra headers into test clients. * Fix deprecation warnings around RandomStringUtils (apache#2507) the `RandomStringUtils.random` method is deprecated since commons-lang 3.17.0 apache/commons-lang@69cb996 * Add support for poetry build with wheel (apache#2425) * Core: Clarify the purpose of REPLACE_NEW_LOCATION_PREFIX_WITH_CATALOG_DEFAULT_KEY (apache#2509) * Update gradle/actions digest to ed40850 (apache#2524) * Update dependency com.fasterxml.jackson:jackson-bom to v2.20.0 (apache#2470) * Update hadoop to v3.4.2 (apache#2466) * Last merged commit a1d1176 --------- Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Yufei Gu <[email protected]>
2 participants
the
RandomStringUtils.randommethod is deprecated since commons-lang 3.17.0apache/commons-lang@69cb996