HonahX commented on code in PR #808:
URL: https://github.com/apache/polaris/pull/808#discussion_r1931677541
##########
spec/rest-catalog-open-api.yaml:
##########
@@ -3977,6 +4241,151 @@ components:
items:
type: integer
description: "List of equality field IDs"
+
+ Policy:
+ type: object
+ required:
+ - owner_id
+ - policy-id
+ - policy-type
+ - name
+ - content
+ - version
+ properties:
+ owner-id:
+ type: string
+ policy-id:
+ type: string
+ policy-type:
+ type: string
+ name:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+ version:
+ type: integer
+ created-at-ms:
+ type: integer
+ format: int64
+ updated-at-ms:
+ type: integer
+ format: int64
+
+ PolicyContent: {}
+
+ CreatePolicyRequest:
+ type: object
+ required:
+ - name
+ - type
+ - content
+ properties:
+ name:
+ type: string
+ type:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+
+ LoadPolicyResult:
+ type: object
+ properties:
+ policy:
+ $ref: '#/components/schemas/Policy'
+
+ UpdatePolicyRequest:
+ type: object
+ properties:
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+
+ SetPolicyRequest:
+ type: object
+ required:
+ - entity
+ properties:
+ entity:
+ $ref: '#/components/schemas/EntityIdentifier'
+ parameters:
+ type: object
+ additionalProperties:
+ type: string
+
+ UnsetPolicyRequest:
+ type: object
+ required:
+ - entity
+ properties:
+ entity:
+ $ref: '#/components/schemas/EntityIdentifier'
+
+ CatalogIdentifier:
+ allOf:
+ - $ref: '#/components/schemas/EntityIdentifier'
+ - type: object
+ required:
+ - catalog
+ properties:
+ catalog:
+ type: string
+
+ NamespaceIdentifier:
+ allOf:
+ - $ref: '#/components/schemas/EntityIdentifier'
+ - type: object
+ required:
+ - catalog
+ - namespace
+ properties:
+ catalog:
+ type: string
+ nullable: false
+ namespace:
+ $ref: '#/components/schemas/Namespace'
+
+ TableLikeIdentifier:
+ allOf:
+ - $ref: '#/components/schemas/EntityIdentifier'
+ - type: object
+ required:
+ - catalog
+ - namespace
+ - name
+ properties:
+ catalog:
+ type: string
+ nullable: false
+ namespace:
+ $ref: '#/components/schemas/Namespace'
+ name:
+ type: string
+ nullable: false
+
+ EntityIdentifier:
+ type: object
+ discriminator:
+ propertyName: type
+ mapping:
+ catalog: '#/components/schemas/CatalogIdentifier'
+ namespace: '#/components/schemas/NamespaceIdentifier'
+ table-like: '#/components/schemas/TableLikeIdentifier'
+ properties:
Review Comment:
> There are also Polaris specific notions like "catalog" added - that
concept doesn't exist in the Iceberg REST API. Referencing one catalog from
another catalog from the Iceberg REST API looks strange to me.
I agree. I think this also aligns with the need for YAML separation.
Separating Polaris APIs from the IRC spec file will also help extend the
concept of 'catalog' in Polaris, that it is not just the Iceberg Rest Catalog,
but Iceberg REST Catalog + Polaris-powered capabilities like policy management
and volumes/directory tables. This approach will make more sense once it's
moved out of the IRC YAML.
##########
spec/rest-catalog-open-api.yaml:
##########
@@ -1688,6 +1943,15 @@ components:
type: integer
minimum: 1
+ policy:
+ name: policy
+ in: path
+ description: a policy name
Review Comment:
> Values, especially those used for path-parameters, should have validation
patterns and only allow unproblematic characters.
Thanks for pointing out! I will add relevant restrictions to these values
##########
spec/rest-catalog-open-api.yaml:
##########
@@ -3977,6 +4241,151 @@ components:
items:
type: integer
description: "List of equality field IDs"
+
+ Policy:
+ type: object
+ required:
+ - owner_id
+ - policy-id
+ - policy-type
+ - name
+ - content
+ - version
+ properties:
+ owner-id:
+ type: string
+ policy-id:
+ type: string
+ policy-type:
+ type: string
+ name:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+ version:
Review Comment:
> I also recommend to not rely on a "version" attribute. The intent of it is
undefined. Similar for created/updated-at-ms.
For `version` do you think a more meaningful name, such as `policy-version`
and some additional description of what policy-version looks like and its usage
make it better here? In the future, we will also introduce
API`RestorePolicyVersion` to help rollback.
For `created/updated-at-ms`, I thought they were for informative purpose
only. Do you recommend to switch to some other format to represent this
information or to not include these information in the object?
Thanks in advance!
##########
spec/rest-catalog-open-api.yaml:
##########
@@ -3977,6 +4241,151 @@ components:
items:
type: integer
description: "List of equality field IDs"
+
+ Policy:
+ type: object
+ required:
+ - owner_id
+ - policy-id
+ - policy-type
+ - name
+ - content
+ - version
+ properties:
+ owner-id:
+ type: string
+ policy-id:
+ type: string
+ policy-type:
+ type: string
+ name:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+ version:
+ type: integer
+ created-at-ms:
+ type: integer
+ format: int64
+ updated-at-ms:
+ type: integer
+ format: int64
+
+ PolicyContent: {}
Review Comment:
> A policy is defined as PolicyContent: {}, which means that anything can go
in there. It is rather impossible for a service to properly handle "concurrent"
updates to a policy - the latest one would blindly win and silently overwrite
all other changes. Why not use "patch" here?
The design is that PolicyContent can be any format, as long as it can be
validated by the validator of the corresponding policy type.
([doc](https://docs.google.com/document/d/1kIiVkFFg9tPa5SH70b9WwzbmclrzH3qWHKfCKXw5lbs/edit?tab=t.0#heading=h.w2d2s7dd5uc)).
For example, a string of some SQL string can be the whole "PolicyContent" so
we will need to replace the whole thing in every update. For user-defined
custom policy type, we will not know what's in the policy content, so we will
rely on the provided validator to ensure the policy is in a good shape.
I think I should add some basic introduction of Policy Validation in the
spec to avoid confusion.
##########
spec/rest-catalog-open-api.yaml:
##########
@@ -3977,6 +4241,151 @@ components:
items:
type: integer
description: "List of equality field IDs"
+
+ Policy:
+ type: object
+ required:
+ - owner_id
+ - policy-id
+ - policy-type
+ - name
+ - content
+ - version
+ properties:
+ owner-id:
+ type: string
+ policy-id:
+ type: string
+ policy-type:
+ type: string
+ name:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+ version:
+ type: integer
+ created-at-ms:
+ type: integer
+ format: int64
+ updated-at-ms:
+ type: integer
+ format: int64
+
+ PolicyContent: {}
+
+ CreatePolicyRequest:
+ type: object
+ required:
+ - name
+ - type
+ - content
+ properties:
+ name:
+ type: string
+ type:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+
+ LoadPolicyResult:
+ type: object
+ properties:
+ policy:
+ $ref: '#/components/schemas/Policy'
+
+ UpdatePolicyRequest:
+ type: object
+ properties:
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+
+ SetPolicyRequest:
+ type: object
+ required:
+ - entity
+ properties:
+ entity:
+ $ref: '#/components/schemas/EntityIdentifier'
+ parameters:
+ type: object
+ additionalProperties:
+ type: string
+
+ UnsetPolicyRequest:
+ type: object
+ required:
+ - entity
+ properties:
+ entity:
+ $ref: '#/components/schemas/EntityIdentifier'
+
+ CatalogIdentifier:
+ allOf:
+ - $ref: '#/components/schemas/EntityIdentifier'
+ - type: object
+ required:
+ - catalog
+ properties:
+ catalog:
+ type: string
+
+ NamespaceIdentifier:
+ allOf:
Review Comment:
> The new types NamespaceIdentifier and TableLikeIdentifier already exist in
the same spec. Not sure whether duplicating those types makes sense.
These are added to support attaching policies to an entity in a different
catalog. I think @flyrain can provide more context on this use case. So the
additional `catalog` field here is what it differentiate from the existing
`TableIdentifier` and `Namespace`.
##########
spec/rest-catalog-open-api.yaml:
##########
@@ -1595,6 +1595,261 @@ paths:
$ref: '#/components/responses/ServiceUnavailableResponse'
5XX:
$ref: '#/components/responses/ServerErrorResponse'
+
+ /v1/{prefix}/namespaces/{namespace}/policies:
+ parameters:
+ - $ref: '#/components/parameters/prefix'
+ - $ref: '#/components/parameters/namespace'
+
+ post:
+ tags:
+ - Catalog API
+ summary: 'Create a policy in the given namespace'
+ operationId: createPolicy
+ description:
+ Create a policy in the given namespace
Review Comment:
> Documentation for types, operations, fields should be more verbose -
examples should also be added.
Good point! Will add those
##########
spec/rest-catalog-open-api.yaml:
##########
@@ -3977,6 +4241,151 @@ components:
items:
type: integer
description: "List of equality field IDs"
+
+ Policy:
+ type: object
+ required:
+ - owner_id
+ - policy-id
+ - policy-type
+ - name
+ - content
+ - version
+ properties:
+ owner-id:
+ type: string
+ policy-id:
+ type: string
+ policy-type:
+ type: string
+ name:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+ version:
+ type: integer
+ created-at-ms:
+ type: integer
+ format: int64
+ updated-at-ms:
+ type: integer
+ format: int64
+
+ PolicyContent: {}
+
+ CreatePolicyRequest:
+ type: object
+ required:
+ - name
+ - type
+ - content
+ properties:
+ name:
+ type: string
+ type:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+
+ LoadPolicyResult:
+ type: object
+ properties:
+ policy:
+ $ref: '#/components/schemas/Policy'
+
+ UpdatePolicyRequest:
+ type: object
+ properties:
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+
+ SetPolicyRequest:
+ type: object
+ required:
+ - entity
+ properties:
+ entity:
+ $ref: '#/components/schemas/EntityIdentifier'
+ parameters:
+ type: object
+ additionalProperties:
+ type: string
+
+ UnsetPolicyRequest:
+ type: object
+ required:
+ - entity
+ properties:
+ entity:
+ $ref: '#/components/schemas/EntityIdentifier'
+
+ CatalogIdentifier:
+ allOf:
+ - $ref: '#/components/schemas/EntityIdentifier'
+ - type: object
+ required:
+ - catalog
+ properties:
+ catalog:
+ type: string
+
+ NamespaceIdentifier:
+ allOf:
+ - $ref: '#/components/schemas/EntityIdentifier'
+ - type: object
+ required:
+ - catalog
+ - namespace
+ properties:
+ catalog:
+ type: string
+ nullable: false
+ namespace:
+ $ref: '#/components/schemas/Namespace'
+
+ TableLikeIdentifier:
+ allOf:
+ - $ref: '#/components/schemas/EntityIdentifier'
+ - type: object
+ required:
+ - catalog
+ - namespace
+ - name
+ properties:
+ catalog:
+ type: string
+ nullable: false
+ namespace:
+ $ref: '#/components/schemas/Namespace'
+ name:
+ type: string
+ nullable: false
+
+ EntityIdentifier:
+ type: object
+ discriminator:
+ propertyName: type
+ mapping:
+ catalog: '#/components/schemas/CatalogIdentifier'
+ namespace: '#/components/schemas/NamespaceIdentifier'
+ table-like: '#/components/schemas/TableLikeIdentifier'
+ properties:
+ type:
+ type: string
+ enum:
Review Comment:
> I'd prefer to omit using any enum - that translates to a Java enum, which
will break with later added enum values very early during request processing.
(Not safe for future development.)
I got the idea from
https://github.com/apache/polaris/blob/5c38780e6130dd5c18346437bc2a6a27e41e9bf6/spec/polaris-management-service.yml#L1320-L1338
Do we also want to update that one?
##########
spec/rest-catalog-open-api.yaml:
##########
@@ -3977,6 +4241,151 @@ components:
items:
type: integer
description: "List of equality field IDs"
+
+ Policy:
+ type: object
+ required:
+ - owner_id
+ - policy-id
+ - policy-type
+ - name
+ - content
+ - version
+ properties:
+ owner-id:
+ type: string
+ policy-id:
+ type: string
+ policy-type:
+ type: string
+ name:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+ version:
+ type: integer
+ created-at-ms:
+ type: integer
+ format: int64
+ updated-at-ms:
+ type: integer
+ format: int64
+
+ PolicyContent: {}
+
+ CreatePolicyRequest:
+ type: object
+ required:
+ - name
+ - type
+ - content
+ properties:
+ name:
+ type: string
+ type:
+ type: string
+ description:
+ type: string
+ content:
+ $ref: '#/components/schemas/PolicyContent'
+
+ LoadPolicyResult:
+ type: object
+ properties:
+ policy:
+ $ref: '#/components/schemas/Policy'
+
+ UpdatePolicyRequest:
+ type: object
+ properties:
+ description:
Review Comment:
> A policy has an ID, a name and description. It seems like ID is a unique,
technical ID and name a human friendly name. However, only the description can
be updated.
The ID is unique. The current design is that policy name is also required to
be unique within a namespace so that we can let user provide policy identifier
(namespace + name) when loading. I am thinking of adding another endpoint to
rename policy, similar as `/v1/{prefix}/tables/rename`. WDYT?
--
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]
