Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discriminator information for parent types used in properties #233

Closed
andrueastman opened this issue Jun 13, 2022 · 2 comments · Fixed by #238
Closed

Discriminator information for parent types used in properties #233

andrueastman opened this issue Jun 13, 2022 · 2 comments · Fixed by #238
Assignees
Labels
priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days type:bug A broken experience
Milestone

Comments

@andrueastman
Copy link
Member

Taking a look at the graph metadata for beta , there exists the userSet type which is abstract.

<ComplexType Name="userSet" Abstract="true">
  <Property Name="isBackup" Type="Edm.Boolean" />
</ComplexType>

It also has several other derived types (such as externalSponsors, groupMembers, internalSponsors) as shown below.

<ComplexType Name="externalSponsors" BaseType="graph.userSet" />
<ComplexType Name="groupMembers" BaseType="graph.userSet">
  <Property Name="description" Type="Edm.String" />
  <Property Name="id" Type="Edm.String" />
</ComplexType>
<ComplexType Name="internalSponsors" BaseType="graph.userSet" />
<ComplexType Name="logicAppTriggerEndpointConfiguration" BaseType="graph.customExtensionEndpointConfiguration">
  <Property Name="logicAppWorkflowName" Type="Edm.String" />
  <Property Name="resourceGroupName" Type="Edm.String" />
  <Property Name="subscriptionId" Type="Edm.String" />
</ComplexType>

The abstract userSet type is referenced through properties of other classes as shown below

<ComplexType Name="approvalStage">
  <Property Name="approvalStageTimeOutInDays" Type="Edm.Int32" />
  <Property Name="escalationApprovers" Type="Collection(graph.userSet)" />   -----> SEE HERE
  <Property Name="escalationTimeInMinutes" Type="Edm.Int32" />
  <Property Name="isApproverJustificationRequired" Type="Edm.Boolean" />
  <Property Name="isEscalationEnabled" Type="Edm.Boolean" />
  <Property Name="primaryApprovers" Type="Collection(graph.userSet)" />   -----> SEE HERE
</ComplexType>
<ComplexType Name="assignmentReviewSettings">
  <Property Name="accessReviewTimeoutBehavior" Type="graph.accessReviewTimeoutBehavior" />
  <Property Name="durationInDays" Type="Edm.Int32" />
  <Property Name="isAccessRecommendationEnabled" Type="Edm.Boolean" />
  <Property Name="isApprovalJustificationRequired" Type="Edm.Boolean" />
  <Property Name="isEnabled" Type="Edm.Boolean" />
  <Property Name="recurrenceType" Type="Edm.String" />
  <Property Name="reviewers" Type="Collection(graph.userSet)" />          -----> SEE HERE
  <Property Name="reviewerType" Type="Edm.String" />
  <Property Name="startDateTime" Type="Edm.DateTimeOffset" />
</ComplexType>

The generated OpenAPI description.

    microsoft.graph.userSet:
      title: userSet
      type: object
      properties:
        isBackup:
          type: boolean
          description: 'For a user in an approval stage, this property indicates whether the user is a backup fallback approver.'
          nullable: true

Question
Should the userSet type have the appropriate discriminator mappings for the derived types? Or is this only limited to types used in nav properties?
The current situation leads to Kiota failing to generate the derived type models of the userSet class.

cc @baywet, @irvinesunday , @darrelmiller

@baywet
Copy link
Member

baywet commented Jun 20, 2022

Thanks for reporting this!
The fact that a type is exposed through a navigation property or a "regular" property doesn't change the inheritance structure.
An approval stage could perfectly come back with a group members entry in the escalation approvers property. In that case the service should annotate the odata type at that level to help the client deserialize to the correct target type. And the client should know what to do with it.
So yes, even in that specific case, the conversion library should add that information to the component.

@baywet baywet added the type:bug A broken experience label Jun 20, 2022
@baywet baywet added this to the OData:1.0.11 milestone Jun 20, 2022
@andrueastman
Copy link
Member Author

Thanks for clarifying on this one @baywet

@CarolKigoonya CarolKigoonya added the priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days label Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days type:bug A broken experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants