[kevinjqliu]

This PR adds server-side warehouse validation to the REST catalog test fixture server, complementing the client-side changes in #16059.

When a warehouse query parameter is provided to /v1/config that doesn't match the configured warehouse, the server returns a 404 NoSuchWarehouseException.

Testing

Build and run the REST catalog server locally:

Default (without setting catalog.name)

Preserves old behavior, everything returns 200. Internally the default catalog name is rest_backend. We are explicitly not gating on that to be backwards compatible

./gradlew :iceberg-open-api:shadowJar -x test
java -jar open-api/build/libs/iceberg-open-api-test-fixtures-runtime-1.12.0-SNAPSHOT.jar

• http://localhost:8181/v1/config -> 200
• http://localhost:8181/v1/config?warehouse=mywarehouse -> 200
• http://localhost:8181/v1/config?warehouse=thisdoesntexist -> 200

With setting catalog.name

./gradlew :iceberg-open-api:shadowJar -x test
CATALOG_CATALOG_NAME=mywarehouse java -jar open-api/build/libs/iceberg-open-api-test-fixtures-runtime-1.12.0-SNAPSHOT.jar

• http://localhost:8181/v1/config -> 200
• http://localhost:8181/v1/config?warehouse=mywarehouse -> 200, matches CATALOG_CATALOG_NAME (catalog.name)
• http://localhost:8181/v1/config?warehouse=thisdoesntexist -> 404

[kevinjqliu]

@gaborkaszab you've been in this part of the codebase, could you take a look?

[gaborkaszab]

Thanks for the PR, @kevinjqliu !
I took a quick look and suggested some alternative ways to do this. Let me know what you think!
Now that we have both the client and server side of this, would be nice to see some tests in TestRESTCatalog for mismatching warehouse names and/or locations.

[gaborkaszab]

RESTSessionCatalog uses CatalogProperties.WAREHOUSE_LOCATION for the query param name. I'd recommend using the same param both at the client and the server side. My preference would be to replace the one in RESTSessionCatalog.fetchConfig() to use this one.

Additionally, when we populate this param on the client side for the request, the code comments talks about warehouse location. I'm a bit confused. Isn't this param a warehouse name, or is it a warehouse location?

[gaborkaszab]

nit: to keep this as simple as possible, can we simply do this and return early if the warehouse is null?

validateWarehouse(vars.get(RESTCatalogProperties.WAREHOUSE_QUERY_PARAMETER));

[gaborkaszab]

The reason we leave the implementation to RESTServerCatalogAdapter is because it has the catalog configs that we need to check for equality with the warehouse query param, right?
This seems a bit odd to me. Can't we pass the catalog configs too to RESTCatalogAdapter? Maybe pass CatalogContext via a new constructor?
Other option could be what the applyCredentials does. It doesn't go through inheritance but we do the checks for response types and enhance with credentials if needed. If we have to keep the warehouse checks on this level of abstraction and not pass the catalog configs to RESTCatalogAdapter, we might want to do the same: check for ConfigResponse and validate the param in vars.
I'd personally go for the former option and pass RESTCatalogAdapter whatever is needed to perform the check itself.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[github-actions]

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.