bharos commented on code in PR #9988:
URL: https://github.com/apache/gravitino/pull/9988#discussion_r2821624016
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java:
##########
@@ -122,13 +123,39 @@ public void dropTable(
@Override
public LoadTableResponse loadTable(
IcebergRequestContext context, TableIdentifier tableIdentifier) {
+ String catalogName = context.catalogName();
+
+ // Per Iceberg REST spec, /tables/ endpoint should only serve tables, not
views.
+ // Check if the identifier is a view and throw NoSuchTableException to
trigger
+ // Spark's fallback logic to use /views/ endpoint instead.
+ if
(icebergCatalogWrapperManager.getCatalogWrapper(catalogName).viewExists(tableIdentifier))
{
Review Comment:
@roryqi Actually, checking tableExists seems hard.
It then means, a user who doesn't have privilege to even check existence of
a table will get 404 (when they should get 403)
So I think it's better to check viewExists() then return 404, so that users
who have no permissions will get 403 for non-existent tables as well
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java:
##########
@@ -122,13 +123,39 @@ public void dropTable(
@Override
public LoadTableResponse loadTable(
IcebergRequestContext context, TableIdentifier tableIdentifier) {
+ String catalogName = context.catalogName();
+
+ // Per Iceberg REST spec, /tables/ endpoint should only serve tables, not
views.
Review Comment:
Done
This required a refactor, to be able to access the catalogWrapperManager in
LoadTableAuthzHandler.
For this, I had to inject catalogWrapperManager into IcebergRESTServerContext
But catalogWrapperManager and IcebergRESTServerContext had a circular
depenency, so I had to pass auxMode, metalakeName to the catalogWrapperManager
to avoid depending on the serverContext
Hope this is ok.. ?
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java:
##########
@@ -122,13 +122,15 @@ public void dropTable(
@Override
public LoadTableResponse loadTable(
IcebergRequestContext context, TableIdentifier tableIdentifier) {
+ String catalogName = context.catalogName();
Review Comment:
done
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java:
##########
@@ -122,13 +123,39 @@ public void dropTable(
@Override
public LoadTableResponse loadTable(
IcebergRequestContext context, TableIdentifier tableIdentifier) {
+ String catalogName = context.catalogName();
+
+ // Per Iceberg REST spec, /tables/ endpoint should only serve tables, not
views.
+ // Check if the identifier is a view and throw NoSuchTableException to
trigger
+ // Spark's fallback logic to use /views/ endpoint instead.
+ if
(icebergCatalogWrapperManager.getCatalogWrapper(catalogName).viewExists(tableIdentifier))
{
Review Comment:
done
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java:
##########
@@ -80,6 +80,10 @@ protected Map<Entity.EntityType, NameIdentifier>
extractNameIdentifierFromParame
NameIdentifierUtil.ofTable(
metalakeName, catalog, schema,
RESTUtil.decodeString(value)));
break;
+ case VIEW:
+ nameIdentifierMap.put(
+ EntityType.VIEW, NameIdentifierUtil.ofView(metalakeName,
catalog, schema, value));
Review Comment:
Decoded the table/view name correctly
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewOperations.java:
##########
@@ -89,6 +105,13 @@ public Response listView(
new IcebergRequestContext(httpServletRequest(), catalogName);
ListTablesResponse listTablesResponse =
Review Comment:
I think so, seems like we don't have view specific response ..
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java:
##########
@@ -113,6 +113,7 @@ private void initServer(IcebergConfig icebergConfig) {
}
IcebergTableEventDispatcher icebergTableEventDispatcher =
new IcebergTableEventDispatcher(icebergTableOperationDispatcher,
eventBus, metalakeName);
+
Review Comment:
done
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewOperations.java:
##########
@@ -131,10 +161,17 @@ public Response createView(
@Produces(MediaType.APPLICATION_JSON)
@Timed(name = "load-view." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
@ResponseMetered(name = "load-view", absolute = true)
+ @AuthorizationExpression(
+ expression =
+ "ANY(OWNER, METALAKE, CATALOG) || "
+ + "SCHEMA_OWNER_WITH_USE_CATALOG || "
+ + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (VIEW::OWNER ||
ANY_SELECT_VIEW)",
Review Comment:
The tests were passing without it.
But looking at the code, I am not 100% sure, but I do see in some places it
calls viewExists in createView
like here :
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/jdbc/JdbcViewOperations.java#L191C18-L191C28
So I think it should be added
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/LoadTableAuthzHandler.java:
##########
@@ -58,7 +65,7 @@ public void process(Map<EntityType, NameIdentifier>
nameIdentifierMap) {
IcebergAuthorizationMetadata icebergMetadata =
parameter.getAnnotation(IcebergAuthorizationMetadata.class);
if (icebergMetadata != null && icebergMetadata.type() ==
RequestType.LOAD_TABLE) {
- tableName = String.valueOf(args[i]);
+ tableName = RESTUtil.decodeString(String.valueOf(args[i]));
Review Comment:
I tried to change it now but it was hard to do it in a generic way.
Namespace parameters require RESTUtil.decodeNamespace()(special parsing) vs
table names need RESTUtil.decodeString(). Decoding in the handler keeps the
logic simple and localized - each handler decodes only what it needs with the
appropriate method.
Maybe we can revisit this in future PR if you have any idea ?
--
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]
