flyrain commented on code in PR #89:
URL: https://github.com/apache/polaris-tools/pull/89#discussion_r2616763509
##########
mcp-server/README.md:
##########
@@ -179,12 +179,12 @@ Realm-specific variables (e.g.,
`POLARIS_REALM_${realm}_CLIENT_ID`) override the
The server exposes the following MCP tools:
-* `polaris-iceberg-table` — Table operations (`list`, `get`, `create`,
`update`, `delete`).
-* `polaris-namespace-request` — Namespace lifecycle management.
-* `polaris-policy` — Policy lifecycle management and mappings.
-* `polaris-catalog-request` — Catalog lifecycle management.
-* `polaris-principal-request` — Principal lifecycle helpers.
-* `polaris-principal-role-request` — Principal role lifecycle and catalog-role
assignments.
-* `polaris-catalog-role-request` — Catalog role and grant management.
+* `polaris-iceberg-table-request` — Perform table-centric operations (`list`,
`get`, `create`, `update`, `delete`).
Review Comment:
Nit: to be consistent with other tool descriptions?
```suggestion
* `polaris-iceberg-table-request` — Perform table operations (`list`, `get`,
`create`, `update`, `delete`).
```
##########
mcp-server/README.md:
##########
@@ -179,12 +179,12 @@ Realm-specific variables (e.g.,
`POLARIS_REALM_${realm}_CLIENT_ID`) override the
The server exposes the following MCP tools:
-* `polaris-iceberg-table` — Table operations (`list`, `get`, `create`,
`update`, `delete`).
-* `polaris-namespace-request` — Namespace lifecycle management.
-* `polaris-policy` — Policy lifecycle management and mappings.
-* `polaris-catalog-request` — Catalog lifecycle management.
-* `polaris-principal-request` — Principal lifecycle helpers.
-* `polaris-principal-role-request` — Principal role lifecycle and catalog-role
assignments.
-* `polaris-catalog-role-request` — Catalog role and grant management.
+* `polaris-iceberg-table-request` — Perform table-centric operations (`list`,
`get`, `create`, `update`, `delete`).
+* `polaris-namespace-request` — Perform namespaces operations (`list`, `get`,
`create`, `exists`, `get-properties`, `delete`).
Review Comment:
Make it grammatically correct?
```suggestion
* `polaris-namespace-request` — Perform namespace operations (`list`, `get`,
`create`, `exists`, `get-properties`, `delete`).
```
##########
mcp-server/README.md:
##########
@@ -179,12 +179,12 @@ Realm-specific variables (e.g.,
`POLARIS_REALM_${realm}_CLIENT_ID`) override the
The server exposes the following MCP tools:
-* `polaris-iceberg-table` — Table operations (`list`, `get`, `create`,
`update`, `delete`).
-* `polaris-namespace-request` — Namespace lifecycle management.
-* `polaris-policy` — Policy lifecycle management and mappings.
-* `polaris-catalog-request` — Catalog lifecycle management.
-* `polaris-principal-request` — Principal lifecycle helpers.
-* `polaris-principal-role-request` — Principal role lifecycle and catalog-role
assignments.
-* `polaris-catalog-role-request` — Catalog role and grant management.
+* `polaris-iceberg-table-request` — Perform table-centric operations (`list`,
`get`, `create`, `update`, `delete`).
+* `polaris-namespace-request` — Perform namespaces operations (`list`, `get`,
`create`, `exists`, `get-properties`, `delete`).
+* `polaris-policy-request` — Perform policies operations (`list`, `get`,
`create`, `update`, `delete`, `attach`, `detach`, `applicable`).
Review Comment:
```suggestion
* `polaris-policy-request` — Perform policy operations (`list`, `get`,
`create`, `update`, `delete`, `attach`, `detach`, `applicable`).
```
##########
mcp-server/polaris_mcp/tools/catalog.py:
##########
@@ -111,41 +111,56 @@ def call(self, arguments: Any) -> ToolExecutionResult:
delegate_args["realm"] = realm
if normalized == "list":
- delegate_args["method"] = "GET"
- delegate_args["path"] = "catalogs"
+ self._handle_list(delegate_args)
elif normalized == "get":
- catalog_name = encode_path_segment(require_text(arguments,
"catalog"))
- delegate_args["method"] = "GET"
- delegate_args["path"] = f"catalogs/{catalog_name}"
+ self._handle_get(arguments, delegate_args)
elif normalized == "create":
- body = arguments.get("body")
- if not isinstance(body, dict):
- raise ValueError(
- "Create operations require a body matching
CreateCatalogRequest."
- )
- delegate_args["method"] = "POST"
- delegate_args["path"] = "catalogs"
- delegate_args["body"] = copy.deepcopy(body)
+ self._handle_create(arguments, delegate_args)
elif normalized == "update":
- catalog_name = encode_path_segment(require_text(arguments,
"catalog"))
- body = arguments.get("body")
- if not isinstance(body, dict):
- raise ValueError(
- "Update operations require a body matching
UpdateCatalogRequest."
- )
- delegate_args["method"] = "PUT"
- delegate_args["path"] = f"catalogs/{catalog_name}"
- delegate_args["body"] = copy.deepcopy(body)
+ self._handle_update(arguments, delegate_args)
elif normalized == "delete":
- catalog_name = encode_path_segment(require_text(arguments,
"catalog"))
- delegate_args["method"] = "DELETE"
- delegate_args["path"] = f"catalogs/{catalog_name}"
+ self._handle_delete(arguments, delegate_args)
else: # pragma: no cover
raise ValueError(f"Unsupported operation: {operation}")
raw = self._rest_client.call(delegate_args)
return self._maybe_augment_error(raw, normalized)
+ def _handle_list(self, delegate_args: JSONDict) -> None:
+ delegate_args["method"] = "GET"
+ delegate_args["path"] = "catalogs"
+
+ def _handle_get(self, arguments: dict, delegate_args: JSONDict) -> None:
+ catalog_name = encode_path_segment(require_text(arguments, "catalog"))
+ delegate_args["method"] = "GET"
+ delegate_args["path"] = f"catalogs/{catalog_name}"
+
+ def _handle_create(self, arguments: dict, delegate_args: JSONDict) -> None:
+ body = arguments.get("body")
+ if not isinstance(body, dict):
Review Comment:
Not a blocker at all—just brainstorming, but would it be helpful if the
error message pointed to the expected shape (e.g., required fields or a small
example) to help callers fix issues quickly. I’m slightly hesitant about it
here since the maintenance cost could be high, and any upstream schema change
would require change here.
--
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]
