dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2070887179
##########
spec/polaris-management-service.yml:
##########
@@ -1105,6 +1105,30 @@ components:
required:
- name
+ Principal:
+ description: A Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ clientId:
+ type: string
+ description: The output-only OAuth clientId associated with this
principal if applicable
+
+ FederatedPrincipal:
+ description: A federated Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ federated:
Review Comment:
TBH, this new state of the PR looks very confusing to me:
1) The OpenAPI spec attempts to describe that clients cannot create a
FederatedPrincipal, but actual Management endpoint accept such requests as
valid and only block those principals in the somewhat deep impl code. Is this
complication worth the effort? IMHO, the end result is more or less the same as
if we had the new `federated` property added to the old `Principal` type in the
API spec.
2) If clients have to rely on the presence of the `federated` property when
interpreting Principals' JSON, this looks to me that the API is defined using
effectively a flat type system, not a hierarchy. Again, having a separate
`FederatedPrincipal` seems redundant to me.
Apologies if my comments cause a lot of back-and-forth, but I'm trying to
reduce complexity.
To summarize: If we want to expose federated Principals in the API for reads
(and I'm fine with that). I'd prefer to just add a simple new `federated`
property to the single `Principal` type in the API spec.
--
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]
