This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new eca1b1a447 [#13189] fix(lance): reject empty storage option keys
(#13190)
eca1b1a447 is described below
commit eca1b1a4474f53778fd6458be606f603d965a453
Author: StormSpirit <[email protected]>
AuthorDate: Thu Sep 17 21:03:30 2026 +0800
[#13189] fix(lance): reject empty storage option keys (#13190)
### What changes were proposed in this pull request?
- Reject a table property whose key is exactly `lance.storage.` before
prefix removal can produce an empty Lance storage-option key.
- Add regression coverage for the exact-prefix case, normal options,
arbitrary non-empty provider-specific keys, insertion ordering, and
catalog-to-table precedence.
- Keep the change limited to
`LancePropertiesUtils.getLanceStorageOptions()` and its unit tests;
reverse conversion and operation call sites are unchanged.
### Why are the changes needed?
`lance.storage.` currently passes Gravitino's prefix-property validation
and is converted into the empty Lance option key `""`. A direct
Docker-based experiment with Lance 6.0.0 and MinIO confirmed that Lance
can retain this malformed key, so the invalid input can cross the
Gravitino-to-Lance boundary without an actionable error at the source.
This PR intentionally fixes the extraction boundary only. On current
`main`, ordinary create can pass the malformed option to dataset
creation, declare can persist metadata while response-side resolution
still accepts it, and register can persist the property and return
success without resolving storage options. After this guard, ordinary
create fails before dataset creation or metadata persistence; declare
may persist metadata before response-side resolution returns an error,
while register still bypasses that resolution. Pre-persistence
validation for `toTableProperties()` and the relevant catalog/table
input paths is tracked as a separate follow-up.
Fix: #13189
### Does this PR introduce _any_ user-facing change?
Yes. An exact `lance.storage.` property key now raises an
input-validation error instead of producing an empty Lance
storage-option key. Existing non-empty options, including
provider-specific keys, retain their current behavior; no public API or
property precedence is changed. This guard is evaluated when storage
options are resolved, not when a property is initially set. Existing
malformed metadata may therefore cause describe and Lance alter
operations to fail, while a non-external drop may delete metadata before
storage cleanup fails, leaving an orphaned dataset.
### How was this patch tested?
- `./gradlew :lance:lance-common:test --tests
'org.apache.gravitino.lance.common.utils.TestLancePropertiesUtils'
-PskipITs` — 3 tests passed with no skips, failures, or errors.
- `./gradlew :lance:lance-common:spotlessCheck` — passed.
- `./gradlew rat` — passed.
- `git diff --check` — passed.
- A separate Docker-based Lance 6.0.0 probe with MinIO confirmed the
malformed empty key is retained by Lance; this is supporting evidence,
not an additional Gravitino CI gate.
Signed-off-by: jiangxt2 <[email protected]>
---
.../lance/common/utils/LancePropertiesUtils.java | 11 ++++++++-
.../common/utils/TestLancePropertiesUtils.java | 26 +++++++++++++++++++++-
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git
a/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LancePropertiesUtils.java
b/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LancePropertiesUtils.java
index 39fbe93bf4..4d235afcf8 100644
---
a/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LancePropertiesUtils.java
+++
b/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LancePropertiesUtils.java
@@ -21,6 +21,7 @@ package org.apache.gravitino.lance.common.utils;
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_STORAGE_OPTIONS_PREFIX;
+import com.google.common.base.Preconditions;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.stream.Collectors;
@@ -37,6 +38,7 @@ public final class LancePropertiesUtils {
*
* @param tableProperties the source properties
* @return the Lance storage options without the `lance.storage.` prefix
+ * @throws IllegalArgumentException if a property key is exactly
`lance.storage.`
*/
public static Map<String, String> getLanceStorageOptions(Map<String, String>
tableProperties) {
if (tableProperties == null || tableProperties.isEmpty()) {
@@ -47,7 +49,14 @@ public final class LancePropertiesUtils {
.filter(entry ->
entry.getKey().startsWith(LANCE_STORAGE_OPTIONS_PREFIX))
.collect(
Collectors.toMap(
- entry ->
entry.getKey().substring(LANCE_STORAGE_OPTIONS_PREFIX.length()),
+ entry -> {
+ String propertyKey = entry.getKey();
+ Preconditions.checkArgument(
+ !LANCE_STORAGE_OPTIONS_PREFIX.equals(propertyKey),
+ "Lance storage option key cannot be empty: %s",
+ propertyKey);
+ return
propertyKey.substring(LANCE_STORAGE_OPTIONS_PREFIX.length());
+ },
Map.Entry::getValue,
(left, right) -> right,
LinkedHashMap::new));
diff --git
a/lance/lance-common/src/test/java/org/apache/gravitino/lance/common/utils/TestLancePropertiesUtils.java
b/lance/lance-common/src/test/java/org/apache/gravitino/lance/common/utils/TestLancePropertiesUtils.java
index 55bfa6c347..51de5b37e1 100644
---
a/lance/lance-common/src/test/java/org/apache/gravitino/lance/common/utils/TestLancePropertiesUtils.java
+++
b/lance/lance-common/src/test/java/org/apache/gravitino/lance/common/utils/TestLancePropertiesUtils.java
@@ -19,6 +19,8 @@
package org.apache.gravitino.lance.common.utils;
import com.google.common.collect.ImmutableMap;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Map;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -31,14 +33,34 @@ public class TestLancePropertiesUtils {
ImmutableMap.of(
"lance.storage.endpoint", "http://minio:9000";,
"lance.storage.access_key_id", "ak",
+ "lance.storage.s3.custom_option", "custom-value",
"not.storage.key", "ignored");
Map<String, String> storageOptions =
LancePropertiesUtils.getLanceStorageOptions(properties);
- Assertions.assertEquals(2, storageOptions.size());
+ Assertions.assertEquals(3, storageOptions.size());
Assertions.assertEquals("http://minio:9000";,
storageOptions.get("endpoint"));
Assertions.assertEquals("ak", storageOptions.get("access_key_id"));
+ Assertions.assertEquals("custom-value",
storageOptions.get("s3.custom_option"));
Assertions.assertFalse(storageOptions.containsKey("not.storage.key"));
+ Assertions.assertEquals(
+ List.of("endpoint", "access_key_id", "s3.custom_option"),
+ new ArrayList<>(storageOptions.keySet()));
+ }
+
+ /** Verifies that the storage prefix itself cannot become an empty provider
option key. */
+ @Test
+ public void testGetLanceStorageOptionsRejectsEmptyOptionKey() {
+ String propertyValue = "secret-value-must-not-leak";
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ LancePropertiesUtils.getLanceStorageOptions(
+ Map.of("lance.storage.", propertyValue)));
+
+ Assertions.assertTrue(exception.getMessage().contains("lance.storage."));
+ Assertions.assertFalse(exception.getMessage().contains(propertyValue));
}
@Test
@@ -59,5 +81,7 @@ public class TestLancePropertiesUtils {
Assertions.assertEquals("http://table:9000";,
storageOptions.get("endpoint"));
Assertions.assertEquals("us-east-1", storageOptions.get("region"));
Assertions.assertEquals("table-ak", storageOptions.get("access_key_id"));
+ Assertions.assertEquals(
+ List.of("endpoint", "region", "access_key_id"), new
ArrayList<>(storageOptions.keySet()));
}
}
