Copilot commented on code in PR #14654:
URL: https://github.com/apache/gravitino/pull/14654#discussion_r4477995078
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestFilesetMetaService.java:
##########
@@ -1013,16 +1012,22 @@ public void
testUpdateRollsBackMetadataWhenVersionInsertFails() throws IOExcepti
original.nameIdentifier(),
entity -> {
FilesetEntity current = (FilesetEntity) entity;
- return FilesetEntity.builder()
- .withId(current.id())
- .withName(current.name())
- .withNamespace(current.namespace())
- .withFilesetType(current.filesetType())
-
.withStorageLocations(ImmutableMap.of(tooLongLocationName, "/tmp-v2"))
- .withComment("new comment")
- .withProperties(current.properties())
- .withAuditInfo(current.auditInfo())
- .build();
+ FilesetEntity updated =
+ Mockito.spy(
+ FilesetEntity.builder()
+ .withId(current.id())
+ .withName(current.name())
+ .withNamespace(current.namespace())
+ .withFilesetType(current.filesetType())
+
.withStorageLocations(current.storageLocations())
+ .withComment("new comment")
+ .withProperties(current.properties())
+ .withAuditInfo(current.auditInfo())
+ .build());
+
Mockito.doReturn(ImmutableMap.of(tooLongLocationName, "/tmp-v2"))
+ .when(updated)
+ .storageLocations();
Review Comment:
This test creates an entity whose internal `storageLocations` field (set via
the builder) differs from what `storageLocations()` returns (overridden via
spy). That inconsistency makes the test fragile and tightly coupled to
implementation details (e.g., whether validation/persistence reads the field
vs. the accessor). Consider bypassing validation more directly (e.g., stubbing
`validate()` on a spy/mock used by the service) while keeping a single source
of truth for storage locations, so the test continues to exercise the intended
DB-length failure even if internals change.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -2466,6 +2466,74 @@ public void testCreateFilesetWithTooLongComment() throws
IOException {
Assertions.assertTrue(fs.exists(filesetPath));
}
+ @ParameterizedTest
+ @EnumSource(Fileset.Type.class)
+ void testCreateFilesetWithTooLongLocationName(Fileset.Type type) throws
IOException {
+ String schemaName = "schema" + generateTestId();
+ String name = "fileset" + generateTestId();
+ createSchema(schemaName, "comment", null, null);
+ Path filesetPath = new Path(TEST_ROOT_PATH + "/" + schemaName, name);
+ String locationName = StringUtils.repeat("a", 257);
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ createMultiLocationFileset(
+ name,
+ schemaName,
+ "comment",
+ type,
+ ImmutableMap.of(),
+ ImmutableMap.of(locationName, filesetPath.toString()),
+ ImmutableMap.of(PROPERTY_DEFAULT_LOCATION_NAME,
locationName)));
+ Assertions.assertEquals(
+ "The storage location name of the fileset must not exceed 256
characters",
+ exception.getMessage());
+ FileSystem fs = filesetPath.getFileSystem(new Configuration());
+ Assertions.assertFalse(fs.exists(filesetPath));
+ Assertions.assertFalse(
+ store.exists(NameIdentifier.of("m1", "c1", schemaName, name),
Entity.EntityType.FILESET));
+ }
+
+ @Test
+ void testCreateFilesetWithTooLongInheritedLocationName() throws IOException {
+ String schemaName = "schema" + generateTestId();
+ String name = "fileset" + generateTestId();
+ String locationName = StringUtils.repeat("a", 257);
Review Comment:
These tests also hardcode both the failing length (257) and the limit (256)
in the expected error message. Prefer deriving these values from
`EntityFieldLimits.MAX_STORAGE_LOCATION_NAME_LENGTH` (and computing `limit + 1`
for the failing case) to keep the tests aligned with the actual limit constant.
##########
core/src/test/java/org/apache/gravitino/meta/TestEntityFieldLimits.java:
##########
@@ -85,6 +85,26 @@ public void testColumnCommentLength() {
EntityFieldLimits.MAX_COLUMN_COMMENT_LENGTH);
}
+ @Test
+ void testFilesetStorageLocationNameLength() {
+ assertLengthLimit(
+ TestEntityFieldLimits::filesetBuilder, "storage location name",
"fileset", 256);
+ }
+
+ @Test
+ void testFilesetStorageLocationNameCountsCodePoints() {
+ String emoji = new String(Character.toChars(0x1F600));
+ String maxLengthName = StringUtils.repeat(emoji, 256);
+ Assertions.assertDoesNotThrow(() -> filesetBuilder(maxLengthName));
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () -> filesetBuilder(maxLengthName
+ emoji));
+ Assertions.assertEquals(
+ "The storage location name of the fileset must not exceed 256
characters",
Review Comment:
The tests hardcode the 256-character limit in multiple places. Since this PR
introduces `EntityFieldLimits.MAX_STORAGE_LOCATION_NAME_LENGTH`, using that
constant in the test inputs and expected message construction will reduce drift
risk if the limit ever changes.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -2466,6 +2466,74 @@ public void testCreateFilesetWithTooLongComment() throws
IOException {
Assertions.assertTrue(fs.exists(filesetPath));
}
+ @ParameterizedTest
+ @EnumSource(Fileset.Type.class)
+ void testCreateFilesetWithTooLongLocationName(Fileset.Type type) throws
IOException {
+ String schemaName = "schema" + generateTestId();
+ String name = "fileset" + generateTestId();
+ createSchema(schemaName, "comment", null, null);
+ Path filesetPath = new Path(TEST_ROOT_PATH + "/" + schemaName, name);
+ String locationName = StringUtils.repeat("a", 257);
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ createMultiLocationFileset(
+ name,
+ schemaName,
+ "comment",
+ type,
+ ImmutableMap.of(),
+ ImmutableMap.of(locationName, filesetPath.toString()),
+ ImmutableMap.of(PROPERTY_DEFAULT_LOCATION_NAME,
locationName)));
+ Assertions.assertEquals(
+ "The storage location name of the fileset must not exceed 256
characters",
+ exception.getMessage());
Review Comment:
These tests also hardcode both the failing length (257) and the limit (256)
in the expected error message. Prefer deriving these values from
`EntityFieldLimits.MAX_STORAGE_LOCATION_NAME_LENGTH` (and computing `limit + 1`
for the failing case) to keep the tests aligned with the actual limit constant.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -2466,6 +2466,74 @@ public void testCreateFilesetWithTooLongComment() throws
IOException {
Assertions.assertTrue(fs.exists(filesetPath));
}
+ @ParameterizedTest
+ @EnumSource(Fileset.Type.class)
+ void testCreateFilesetWithTooLongLocationName(Fileset.Type type) throws
IOException {
+ String schemaName = "schema" + generateTestId();
+ String name = "fileset" + generateTestId();
+ createSchema(schemaName, "comment", null, null);
+ Path filesetPath = new Path(TEST_ROOT_PATH + "/" + schemaName, name);
+ String locationName = StringUtils.repeat("a", 257);
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ createMultiLocationFileset(
+ name,
+ schemaName,
+ "comment",
+ type,
+ ImmutableMap.of(),
+ ImmutableMap.of(locationName, filesetPath.toString()),
+ ImmutableMap.of(PROPERTY_DEFAULT_LOCATION_NAME,
locationName)));
+ Assertions.assertEquals(
+ "The storage location name of the fileset must not exceed 256
characters",
+ exception.getMessage());
+ FileSystem fs = filesetPath.getFileSystem(new Configuration());
+ Assertions.assertFalse(fs.exists(filesetPath));
+ Assertions.assertFalse(
+ store.exists(NameIdentifier.of("m1", "c1", schemaName, name),
Entity.EntityType.FILESET));
+ }
+
+ @Test
+ void testCreateFilesetWithTooLongInheritedLocationName() throws IOException {
+ String schemaName = "schema" + generateTestId();
+ String name = "fileset" + generateTestId();
+ String locationName = StringUtils.repeat("a", 257);
+ String schemaPath = TEST_ROOT_PATH + "/" + schemaName;
+ createMultiLocationSchema(
+ schemaName,
+ "comment",
+ ImmutableMap.of(),
+ ImmutableMap.of(
+ PROPERTY_MULTIPLE_LOCATIONS_PREFIX + locationName,
+ schemaPath,
+ PROPERTY_DEFAULT_LOCATION_NAME,
+ locationName));
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ createMultiLocationFileset(
+ name,
+ schemaName,
+ "comment",
+ Fileset.Type.MANAGED,
+ ImmutableMap.of(),
+ ImmutableMap.of(),
+ ImmutableMap.of()));
+ Assertions.assertEquals(
+ "The storage location name of the fileset must not exceed 256
characters",
+ exception.getMessage());
Review Comment:
These tests also hardcode both the failing length (257) and the limit (256)
in the expected error message. Prefer deriving these values from
`EntityFieldLimits.MAX_STORAGE_LOCATION_NAME_LENGTH` (and computing `limit + 1`
for the failing case) to keep the tests aligned with the actual limit constant.
##########
core/src/test/java/org/apache/gravitino/meta/TestEntityFieldLimits.java:
##########
@@ -85,6 +85,26 @@ public void testColumnCommentLength() {
EntityFieldLimits.MAX_COLUMN_COMMENT_LENGTH);
}
+ @Test
+ void testFilesetStorageLocationNameLength() {
+ assertLengthLimit(
+ TestEntityFieldLimits::filesetBuilder, "storage location name",
"fileset", 256);
+ }
+
+ @Test
+ void testFilesetStorageLocationNameCountsCodePoints() {
+ String emoji = new String(Character.toChars(0x1F600));
+ String maxLengthName = StringUtils.repeat(emoji, 256);
+ Assertions.assertDoesNotThrow(() -> filesetBuilder(maxLengthName));
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () -> filesetBuilder(maxLengthName
+ emoji));
+ Assertions.assertEquals(
+ "The storage location name of the fileset must not exceed 256
characters",
Review Comment:
The tests hardcode the 256-character limit in multiple places. Since this PR
introduces `EntityFieldLimits.MAX_STORAGE_LOCATION_NAME_LENGTH`, using that
constant in the test inputs and expected message construction will reduce drift
risk if the limit ever changes.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -2466,6 +2466,74 @@ public void testCreateFilesetWithTooLongComment() throws
IOException {
Assertions.assertTrue(fs.exists(filesetPath));
}
+ @ParameterizedTest
+ @EnumSource(Fileset.Type.class)
+ void testCreateFilesetWithTooLongLocationName(Fileset.Type type) throws
IOException {
+ String schemaName = "schema" + generateTestId();
+ String name = "fileset" + generateTestId();
+ createSchema(schemaName, "comment", null, null);
+ Path filesetPath = new Path(TEST_ROOT_PATH + "/" + schemaName, name);
+ String locationName = StringUtils.repeat("a", 257);
Review Comment:
These tests also hardcode both the failing length (257) and the limit (256)
in the expected error message. Prefer deriving these values from
`EntityFieldLimits.MAX_STORAGE_LOCATION_NAME_LENGTH` (and computing `limit + 1`
for the failing case) to keep the tests aligned with the actual limit constant.
##########
core/src/test/java/org/apache/gravitino/meta/TestEntityFieldLimits.java:
##########
@@ -85,6 +85,26 @@ public void testColumnCommentLength() {
EntityFieldLimits.MAX_COLUMN_COMMENT_LENGTH);
}
+ @Test
+ void testFilesetStorageLocationNameLength() {
+ assertLengthLimit(
+ TestEntityFieldLimits::filesetBuilder, "storage location name",
"fileset", 256);
+ }
+
+ @Test
+ void testFilesetStorageLocationNameCountsCodePoints() {
+ String emoji = new String(Character.toChars(0x1F600));
+ String maxLengthName = StringUtils.repeat(emoji, 256);
+ Assertions.assertDoesNotThrow(() -> filesetBuilder(maxLengthName));
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () -> filesetBuilder(maxLengthName
+ emoji));
+ Assertions.assertEquals(
+ "The storage location name of the fileset must not exceed 256
characters",
Review Comment:
The tests hardcode the 256-character limit in multiple places. Since this PR
introduces `EntityFieldLimits.MAX_STORAGE_LOCATION_NAME_LENGTH`, using that
constant in the test inputs and expected message construction will reduce drift
risk if the limit ever changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
