dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2070556405
##########
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:
Do we need this property when the type already implies the Principal is
federated?
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalEntity.java:
##########
@@ -78,6 +95,13 @@ public Builder setCredentialRotationRequiredState() {
return this;
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
+ internalProperties.put(FederatedEntities.FEDERATED_ENTITY, "true");
+ }
+ return this;
+ }
Review Comment:
Why is this setter impl different from a similar setter in
`PrincipalRoleEntity`?
https://github.com/apache/polaris/pull/1353/files#diff-f746c88d518ffc25b33cbd1052fe77d99e0a35711955d2f544a822b02a9e6f09R69
##########
api/management-model/build.gradle.kts:
##########
@@ -57,6 +57,8 @@ openApiGenerate {
additionalProperties.put("apiNameSuffix", "Api")
additionalProperties.put("metricsPrefix", "polaris")
serverVariables = mapOf("basePath" to "api/v1")
+ openapiNormalizer.put("REFACTOR_ALLOF_WITH_PROPERTIES_ONLY", "true")
Review Comment:
Could you add a comment about what these options mean?
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -871,6 +874,47 @@ public void testCreatePrincipalAndRotateCredentials() {
// rotation that makes the old secret fall off retention.
}
+ @Test
+ public void testCreateFederatedPrincipalFails() throws
JsonProcessingException {
+ // Create a federated Principal
+ FederatedPrincipal federatedPrincipal =
+ new FederatedPrincipal(
+ true, client.newEntityName("federatedPrincipal"), Map.of(), 0L,
0L, 1);
+
+ // Attempt to create the federated Principal using the managementApi
+ try (Response createPResponse =
+ managementApi
+ .request("v1/principals")
+ .post(
+ Entity.json(
+ "{\"principal\":"
+ + new
ObjectMapper().writeValueAsString(federatedPrincipal)
+ + "}"))) {
+ assertThat(createPResponse).returns(CREATED.getStatusCode(),
Response::getStatus);
Review Comment:
The test's name implies that this request should fail, but it succeeds?
##########
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:
If we only want to treat then as sub-types in server code, but not in JSON,
I believe we can still handle that in deserializers without complicating the
Open API spec... WDYT?
##########
spec/polaris-management-service.yml:
##########
@@ -1151,6 +1175,10 @@ components:
maxLength: 256
pattern: '^(?!\s*[s|S][y|Y][s|S][t|T][e|E][m|M]\$).*$'
description: The name of the role
+ federated:
Review Comment:
We use sub-types for federated Principals, but not for federated Principal
Roles?
--
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]
