You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "zhang-arvin (via GitHub)" <gi...@apache.org> on 2026/08/31 15:10:09 UTC
[PR] Make storage type names case-insensitive [polaris]
zhang-arvin opened a new pull request, #5419:
URL: https://github.com/apache/polaris/pull/5419
## What changes were proposed in this pull request?
Fixes #996: Storage type names are treated as case-sensitive (e.g., 's3' and 'S3' are treated differently), causing API calls to fail with "unsupported storage type" message.
### Changes
Added a custom Jackson deserializer for `StorageConfigInfo` that normalizes the `storageType` field to uppercase during deserialization, so case variations like 's3', 'S3', 'gcs', 'GCS', 'azure', 'AZURE', 'file', 'FILE' are all accepted.
## How was this patch tested?
- Verified the deserializer is registered in the ObjectMapper configuration
- The change is backward-compatible: existing uppercase values continue to work unchanged
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make storage type names case-insensitive [polaris]
Posted by "zhang-arvin (via GitHub)" <gi...@apache.org>.
zhang-arvin closed pull request #5419: Make storage type names case-insensitive
URL: https://github.com/apache/polaris/pull/5419
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make storage type names case-insensitive [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5419:
URL: https://github.com/apache/polaris/pull/5419#discussion_r3899195136
##########
runtime/service/src/main/java/org/apache/polaris/service/config/Serializers.java:
##########
@@ -242,4 +247,29 @@ public RevokeGrantRequest deserialize(JsonParser p, DeserializationContext ctxt)
}
}
}
+
+ /**
+ * Deserializer for {@link StorageConfigInfo} that normalizes the {@code storageType} field to
+ * uppercase to support case-insensitive storage type values (e.g., "s3" and "S3" are treated the
+ * same).
+ */
+ public static final class StorageConfigInfoDeserializer
+ extends JsonDeserializer<StorageConfigInfo> {
+ @Override
+ public StorageConfigInfo deserialize(JsonParser p, DeserializationContext ctxt)
+ throws IOException {
+ JsonNode treeNode = p.readValueAsTree();
+ if (treeNode.isObject()) {
+ ObjectNode node = (ObjectNode) treeNode;
+ JsonNode storageTypeNode = node.get("storageType");
+ if (storageTypeNode != null && storageTypeNode.isTextual()) {
+ node.put("storageType", storageTypeNode.asText().toUpperCase());
Review Comment:
Please use `toUpperCase(Locale.ROOT)` to avoid behaviour dependencies on the execution env.
##########
runtime/service/src/main/java/org/apache/polaris/service/config/Serializers.java:
##########
@@ -242,4 +247,29 @@ public RevokeGrantRequest deserialize(JsonParser p, DeserializationContext ctxt)
}
}
}
+
+ /**
+ * Deserializer for {@link StorageConfigInfo} that normalizes the {@code storageType} field to
+ * uppercase to support case-insensitive storage type values (e.g., "s3" and "S3" are treated the
+ * same).
+ */
+ public static final class StorageConfigInfoDeserializer
+ extends JsonDeserializer<StorageConfigInfo> {
+ @Override
+ public StorageConfigInfo deserialize(JsonParser p, DeserializationContext ctxt)
+ throws IOException {
+ JsonNode treeNode = p.readValueAsTree();
+ if (treeNode.isObject()) {
+ ObjectNode node = (ObjectNode) treeNode;
+ JsonNode storageTypeNode = node.get("storageType");
+ if (storageTypeNode != null && storageTypeNode.isTextual()) {
+ node.put("storageType", storageTypeNode.asText().toUpperCase());
+ }
+ }
+ // Use a new parser from the tree to avoid infinite recursion with the custom deserializer
+ JsonParser treeParser = treeNode.traverse(p.getCodec());
+ treeParser.nextToken();
+ return ctxt.readValue(treeParser, StorageConfigInfo.class);
+ }
Review Comment:
Could you add unit tests, please?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make storage type names case-insensitive [polaris]
Posted by "zhang-arvin (via GitHub)" <gi...@apache.org>.
zhang-arvin commented on PR #5419:
URL: https://github.com/apache/polaris/pull/5419#issuecomment-6076184161
Closing this as the current approach cannot work and the fix belongs at the spec/serde-discriminator layer. `StorageConfigInfo` is `@JsonTypeInfo(EXISTING_PROPERTY, "storageType")`, so Jackson resolves the polymorphic type id *before* any value deserializer runs — a lower-cased `"s3"` fails with `InvalidTypeIdException` before the PR's `StorageConfigInfoDeserializer` is ever entered (confirmed by a unit test: lowercase cases fail, uppercase passes). As @snazy noted, the storage-type values are effectively an API contract, so the correct fix is to either (a) keep requiring the canonical uppercase values, or (b) handle case-insensitivity at the discriminator/enum layer (the direction of #5036). I'll follow up there rather than here. Thanks for the review!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make storage type names case-insensitive [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5419:
URL: https://github.com/apache/polaris/pull/5419#issuecomment-6050562232
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make storage type names case-insensitive [polaris]
Posted by "zhang-arvin (via GitHub)" <gi...@apache.org>.
zhang-arvin commented on PR #5419:
URL: https://github.com/apache/polaris/pull/5419#issuecomment-6059491321
> See previous PR #1022. The values of the storage types is well defined in the spec and are effectively an API contract. I'd suggest to just use the uppercase values.
You're right, and I now think this PR should not go in as-is — for a stronger reason than the contract argument alone: **the deserializer in this PR does not actually take effect.**
`StorageConfigInfo` is generated with `@JsonTypeInfo(use = Id.NAME, include = As.EXISTING_PROPERTY, property = "storageType")` plus `@JsonSubTypes` keyed by `S3`/`GCS`/`AZURE`/`FILE`. With `EXISTING_PROPERTY`, `storageType` is consumed as the polymorphic **type id** and resolved *before* the value deserializer registered for `StorageConfigInfo` is invoked, so a `StorageConfigInfoDeserializer` registered via `SimpleModule.addDeserializer(StorageConfigInfo.class, ...)` is bypassed (it becomes the value deserializer inside `TypeWrappedDeserializer`, but the type id is read first).
I added a unit test that registers the mapper the way the server does (`Serializers.registerSerializers`) and deserializes `{"storageType":"s3", ...}`. It fails with:
```
com.fasterxml.jackson.databind.exc.InvalidTypeIdException: Could not resolve type id 's3' as a subtype of `org.apache.polaris.core.admin.model.StorageConfigInfo`: known type ids = [AZURE, FILE, GCS, S3, StorageConfigInfo]
at ...TypeWrappedDeserializer.deserialize(...)
```
(uppercase `"S3"` still works — the failure is only for the lowercase form this PR is meant to fix.)
So the normalization would have to happen at the polymorphic-type-id / enum layer (e.g. a discriminator `mapping` with lowercase aliases plus a case-insensitive `fromValue` on the generated enums, as discussed in #5036) — a spec/generator change — rather than a base-type deserializer. Given that the `dev` ML discussion did not converge on case-insensitive storage types for the user-facing API (dimas-b proposed closing #996), I don't want to push a serde hack that also does not work.
Per your suggestion I'll go with "just use the uppercase values": I'd propose closing this PR (leaving #996 to the spec-level discussion), unless you'd prefer I rework it at the discriminator/enum layer. Also dropping dimas-b's earlier `toUpperCase(Locale.ROOT)` / "add tests" asks, since the approach is being abandoned.
Happy to take whichever direction you prefer.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make storage type names case-insensitive [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #5419:
URL: https://github.com/apache/polaris/pull/5419#issuecomment-5570103933
See previous PR #1022.
The values of the storage types is well defined in the spec and are effectively an API contract.
I'd suggest to just use the uppercase values.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org