OpenAPI: Support CATALOG_NAME env var in REST fixture#16007
OpenAPI: Support CATALOG_NAME env var in REST fixture#16007rexminnis wants to merge 2 commits intoapache:mainfrom
Conversation
The REST catalog test fixture hardcoded catalog_name to "rest_backend", breaking interop when Trino, PyIceberg, or other engines expected a different name. The existing CATALOG_* prefix scheme required the unintuitive CATALOG_CATALOG_NAME=foo to override it. Adds explicit CATALOG_NAME env var resolution and documents both paths in docker/iceberg-rest-fixture/README.md. No behavior change when CATALOG_NAME is unset — default remains "rest_backend". Adds RESTCatalogServerTest covering set, property-wins-over-env, missing, and empty env cases. Closes apache#14972
| LOG.info("No warehouse location set. Defaulting to temp location: {}", warehouseLocation); | ||
| } | ||
|
|
||
| resolveCatalogNameFromEnv(catalogProperties, System.getenv()); |
There was a problem hiding this comment.
Why do we need this change? The catalogProperties comes from RCKUtils.environmentCatalogConfig(). I expect it contains CATALOG_NAME environment variable.
There was a problem hiding this comment.
Thanks for the review, @ebyhr.
environmentCatalogConfig() turns CATALOG_NAME into the property key name, not catalog.name:
CATALOG_NAME
→ strip "CATALOG_" prefix → "NAME"
→ "__" → "-" → "NAME"
→ "_" → "." → "NAME" (no underscores left)
→ lowerCase → "name"
But RESTCatalogServer reads property catalog.name (CATALOG_NAME constant on line 48 = "catalog.name"). So env CATALOG_NAME=foo ends up as property name, which is ignored, and the backend catalog stays rest_backend.
To set catalog.name via the existing scheme, a user has to set CATALOG_CATALOG_NAME=foo — which #14972 calls out as unintuitive. This PR adds the shorter CATALOG_NAME env var that users naturally try first.
Happy to rework if you'd prefer a different shape — e.g., just documenting CATALOG_CATALOG_NAME without a new code path — but the reporter explicitly asked for a simpler env var.
- Rename RESTCatalogServerTest to TestRESTCatalogServer (repo convention per TestClassNamingConvention — Iceberg uses Test prefix, not suffix). - Use Maps.newHashMap() from the relocated Guava module instead of new HashMap<>() (RegexpSingleline rule). No behavior change.
2 participants
What changes are included in this PR?
Adds
CATALOG_NAMEenvironment variable support to the REST catalog testfixture, allowing users to override the backend catalog name without the
unintuitive
CATALOG_CATALOG_NAME=...form required by the existingCATALOG_*prefix translation.RESTCatalogServerresolvesCATALOG_NAMEexplicitly before the standardproperty lookup, using
putIfAbsentso any caller-providedcatalog.nameproperty still wins.
docker/iceberg-rest-fixture/README.mdgains a Configuration sectiondocumenting
CATALOG_NAMEand the generalCATALOG_*env-var scheme.RESTCatalogServerTestexercises the helper with 4 cases: env set,property-wins-over-env, env missing, env empty.
Closes #14972.
Are these changes tested?
Yes — new
RESTCatalogServerTestruns as part of:iceberg-open-api:test.Are there any user-facing changes?
No behavior change when
CATALOG_NAMEis unset — default remainsrest_backend. Users can now set-e CATALOG_NAME=mycatalogon theapache/iceberg-rest-fixturecontainer to change the catalog name servedby the fixture.