Skip to content

Fix deprecation warnings around RandomStringUtils #2507

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

Merged
merged 1 commit into from
Sep 8, 2025
Merged

Fix deprecation warnings around RandomStringUtils #2507

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions ...main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ public void tearDown() {
client.cleanUp(authToken);
}

private static String newRandomString(int length) {
return RandomStringUtils.insecure().next(length, true, true);
Copy link
Contributor

@dimas-b dimas-b Sep 4, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@XN137 XN137 Sep 8, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dimas-b dimas-b Sep 8, 2025

Choose a reason for hiding this comment

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

insecure() is technically acceptable for current PR, I think. These names do not have to be cryptographically secure.

Copy link
Contributor

@dimas-b dimas-b Sep 8, 2025

Choose a reason for hiding this comment

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

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).

}

@Test
public void testCatalogSerializing() throws IOException {
CatalogProperties props = new CatalogProperties("s3://my-old-bucket/path/to/data");
Expand Down Expand Up @@ -215,7 +219,7 @@ public void testCreateCatalogWithInvalidName() {
.setAllowedLocations(List.of("s3://my-old-bucket/path/to/data"))
.build();

String goodName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH, true, true);
String goodName = newRandomString(MAX_IDENTIFIER_LENGTH);

Catalog catalog =
PolarisCatalog.builder()
Expand All @@ -228,7 +232,7 @@ public void testCreateCatalogWithInvalidName() {
assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
}

String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
String longInvalidName = newRandomString(MAX_IDENTIFIER_LENGTH + 1);
List<String> invalidCatalogNames =
Arrays.asList(
longInvalidName,
Expand Down Expand Up @@ -751,7 +755,7 @@ public void testGetCatalogNotFound() {

@Test
public void testGetCatalogInvalidName() {
String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
String longInvalidName = newRandomString(MAX_IDENTIFIER_LENGTH + 1);
List<String> invalidCatalogNames =
Arrays.asList(
longInvalidName,
Expand Down Expand Up @@ -784,7 +788,7 @@ public void testCatalogRoleInvalidName() {
.build();
managementApi.createCatalog(catalog);

String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
String longInvalidName = newRandomString(MAX_IDENTIFIER_LENGTH + 1);
List<String> invalidCatalogRoleNames =
Arrays.asList(
longInvalidName,
Expand Down Expand Up @@ -1038,15 +1042,15 @@ public void testCreateListUpdateAndDeletePrincipal() {

@Test
public void testCreatePrincipalWithInvalidName() {
String goodName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH, true, true);
String goodName = newRandomString(MAX_IDENTIFIER_LENGTH);
Principal principal =
Principal.builder()
.setName(goodName)
.setProperties(Map.of("custom-tag", "good_principal"))
.build();
managementApi.createPrincipal(new CreatePrincipalRequest(principal, null));

String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
String longInvalidName = newRandomString(MAX_IDENTIFIER_LENGTH + 1);
List<String> invalidPrincipalNames =
Arrays.asList(
longInvalidName,
Expand Down Expand Up @@ -1078,7 +1082,7 @@ public void testCreatePrincipalWithInvalidName() {

@Test
public void testGetPrincipalWithInvalidName() {
String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
String longInvalidName = newRandomString(MAX_IDENTIFIER_LENGTH + 1);
List<String> invalidPrincipalNames =
Arrays.asList(
longInvalidName,
Expand Down Expand Up @@ -1193,12 +1197,12 @@ public void testCreateListUpdateAndDeletePrincipalRole() {

@Test
public void testCreatePrincipalRoleInvalidName() {
String goodName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH, true, true);
String goodName = newRandomString(MAX_IDENTIFIER_LENGTH);
PrincipalRole principalRole =
new PrincipalRole(goodName, false, Map.of("custom-tag", "good_principal_role"), 0L, 0L, 1);
managementApi.createPrincipalRole(principalRole);

String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
String longInvalidName = newRandomString(MAX_IDENTIFIER_LENGTH + 1);
List<String> invalidPrincipalRoleNames =
Arrays.asList(
longInvalidName,
Expand Down Expand Up @@ -1233,7 +1237,7 @@ public void testCreatePrincipalRoleInvalidName() {

@Test
public void testGetPrincipalRoleInvalidName() {
String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
String longInvalidName = newRandomString(MAX_IDENTIFIER_LENGTH + 1);
List<String> invalidPrincipalRoleNames =
Arrays.asList(
longInvalidName,
Expand Down