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

Support for composable functions #431

Merged
merged 11 commits into from
Nov 28, 2023

Conversation

MartinM85
Copy link
Contributor

@MartinM85 MartinM85 commented Oct 6, 2023

fixes #402
fixes #404

Generate ODataPath for composable functions

Generate ODataPath for composable functions
@MartinM85
Copy link
Contributor Author

MartinM85 commented Oct 6, 2023

Proposal of a fix for #402.

Now, ODataPath is not generated for composable functions

../operation/operation

Example from Graph API v1.0 metadata.xml

      <Function Name="lastCell" IsBound="true" IsComposable="true">
        <Parameter Name="bindparameter" Type="graph.workbookRange" />
        <ReturnType Type="graph.workbookRange" />
      </Function>

      <Function Name="visibleView" IsBound="true" IsComposable="true">
        <Parameter Name="bindparameter" Type="graph.workbookRange" />
        <ReturnType Type="graph.workbookRangeView" />
      </Function>

      <Action Name="unmerge" IsBound="true">
        <Parameter Name="bindparameter" Type="graph.workbookRange" />
      </Action>

      <EntityType Name="workbookRangeView" BaseType="graph.entity">
        <Property Name="cellAddresses" Type="graph.Json" />
        <Property Name="columnCount" Type="Edm.Int32" Nullable="false" />
        <Property Name="formulas" Type="graph.Json" />
        <Property Name="formulasLocal" Type="graph.Json" />
        <Property Name="formulasR1C1" Type="graph.Json" />
        <Property Name="index" Type="Edm.Int32" Nullable="false" />
        <Property Name="numberFormat" Type="graph.Json" />
        <Property Name="rowCount" Type="Edm.Int32" Nullable="false" />
        <Property Name="text" Type="graph.Json" />
        <Property Name="values" Type="graph.Json" />
        <Property Name="valueTypes" Type="graph.Json" />
        <NavigationProperty Name="rows" Type="Collection(graph.workbookRangeView)" ContainsTarget="true" />
      </EntityType>

If functions are composable, they can be chained

../operation/operation/../operation

I have a restriction that only one operation segment can be appended

It's not a fix for #404 which is a case when a navigation segment is added to the operation segment

I can add the navigation segment

...
AppendPath(newOperationPath);

if (edmOperation.ReturnType != null && edmOperation.ReturnType.Definition is IEdmEntityType returnBindingEntityType)
{
    foreach (var navProperty in returnBindingEntityType.NavigationProperties())
    {
        ODataPath newNavigationPath = newOperationPath.Clone();
        newNavigationPath.Push(new ODataNavigationPropertySegment(navProperty));
        AppendPath(newNavigationPath);
    }
}

but newNavigationPath.Kind is calculated as Operation and it will break the code in

ODataOperationSegment operationSegment = path.LastSegment as ODataOperationSegment;

@MartinM85
Copy link
Contributor Author

The second commit contains fix for #404

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great contribution! In addition to my comments, could you add unit tests for the case you're implementing please?

Improve appending to composable functions
@MartinM85
Copy link
Contributor Author

Added unit test and small changes to ensure that operations are appended to all composable functions, and navigation properties are appended to functions.

@MartinM85 MartinM85 requested a review from baywet October 11, 2023 07:43
baywet
baywet previously approved these changes Oct 11, 2023
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes! @irvinesunday for final review and merge

@irvinesunday
Copy link
Contributor

Wanted to add this diff screenshot for informational purposes.

image

The diff image above shows the current number of paths per API generated using the Kiota-based CSDL for v1.0.
Left is current production figures, right is with the changes in this PR.

Below is a similar diff but using the non Kiota-based CSDL (PowerShell and Graph Explorer)

image

Left is current production figures, right is with the changes in this PR.

drives API seem to have grown by quite a huge margin. We may need to investigate the accuracy of the operations bound to the drives API and and fix them in the metadata if necessary before merging this PR.

@irvinesunday
Copy link
Contributor

@MartinM85 do you mind updating the release notes and bumping up the version in the csproj file?

@MartinM85
Copy link
Contributor Author

@irvinesunday Version and release notes updated

@irvinesunday
Copy link
Contributor

Wanted to add this diff screenshot for informational purposes.

image

The diff image above shows the current number of paths per API generated using the Kiota-based CSDL for v1.0. Left is current production figures, right is with the changes in this PR.

Below is a similar diff but using the non Kiota-based CSDL (PowerShell and Graph Explorer)

image

Left is current production figures, right is with the changes in this PR.

drives API seem to have grown by quite a huge margin. We may need to investigate the accuracy of the operations bound to the drives API and and fix them in the metadata if necessary before merging this PR.

Investigation to the growth in the drives API is tracked with this issue.

@irvinesunday
Copy link
Contributor

irvinesunday commented Nov 27, 2023

Tested this again, the changes are acceptable, and this PR can be merged.

image
v1.0

image
beta

@baywet
Copy link
Member

baywet commented Nov 27, 2023

@irvinesunday, thanks for double checking. Did you have to make any changes to the metadata? Any changes we should make to the test CSDLs here considering the bigger increase in path items? (see the integration tests numbers)

@irvinesunday
Copy link
Contributor

@irvinesunday, thanks for double checking. Did you have to make any changes to the metadata? Any changes we should make to the test CSDLs here considering the bigger increase in path items? (see the integration tests numbers)

No, I didn't make any changes to the metadata. The integration file is probably falling behind the main metadata file in terms of annotations.

@baywet
Copy link
Member

baywet commented Nov 27, 2023

@irvinesunday would you mind having a quick look at those to check if something obvious comes up? (like a wrong contains target or a missing annotation) so we can keep those numbers down?

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks everyone for the work on this one.
@irvinesunday for final review and merge.

@irvinesunday
Copy link
Contributor

irvinesunday commented Nov 28, 2023

@baywet our biggest culprit was a containment for a directoryObject type navigation property that also had a collection of derived types.

<NavigationProperty Name="deletedItems" Type="Collection(graph.directoryObject)" ContainsTarget="true">
  <Annotation Term="Org.OData.Validation.V1.DerivedTypeConstraint">
    <Collection>
      <String>microsoft.graph.user</String>
      <String>microsoft.graph.group</String>
      <String>microsoft.graph.application</String>
    </Collection>
  </Annotation>
  <Annotation Term="Org.OData.Core.V1.ExplicitOperationBindings">
    <Collection>
      <String>microsoft.graph.restore</String>
    </Collection>
  </Annotation>
</NavigationProperty>

Removing the containment (as we've done in our prod files) lowers the path numbers here significantly.
The integration file and our prod file are still not 100% a match but nothing else out of the ordinary stood out.
We can merge this.

@baywet baywet merged commit f97b4e3 into microsoft:master Nov 28, 2023
7 checks passed
@MartinM85 MartinM85 deleted the mm/composable-functions branch November 29, 2023 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/visibleView/rows missing in OpenAPI description /range/lastCell is missing in OpenAPI description
3 participants