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

Expands navigation properties of derived types only if declaring navigation property is a containment #302

Merged
merged 6 commits into from
Nov 1, 2022

Conversation

irvinesunday
Copy link
Contributor

@irvinesunday irvinesunday commented Oct 26, 2022

Fixes #269

This PR:

  • Fixes the logic of expanding the navigation properties of derived types only if the declaring navigation property is a containment. Initially, expansion covered both containment and non-containment navigation properties.
  • Adds a check to ensure we don't add a derived type that has already been added in the path when expanding the type's navigation properties.
  • Update tests and integration files to validate the above.

To fully resolve the aforementioned issue, the below convert settings need to be set to true:

  • ExpandDerivedTypesNavigationProperties
  • AppendBoundOperationsOnDerivedTypeCastSegments

With the above fix and the above settings set to true, the result, for the examples provided in the issue, will be:

andrueastman
andrueastman previously approved these changes Oct 31, 2022
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

👍🏼

@andrueastman
Copy link
Member

Thanks @irvinesunday

@irvinesunday
Copy link
Contributor Author

Adding more information for context:

The table below shows the number of beta paths generated when the two convert settings ExpandDerivedTypesNavigationProperties and AppendBoundOperationsOnDerivedTypeCastSegments are enabled/disabled.

Convert Setting ExpandDerivedTypesNavigationProperties = true ExpandDerivedTypesNavigationProperties = false
AppendBoundOperationsOnDerivedTypeCastSegments = true 21,484 16,969
AppendBoundOperationsOnDerivedTypeCastSegments = false 19,342 14,846

@peombwa / @timayabi2020 we might need to verify the generated OpenAPI document with PowerShell to verify whether there are any modules that might break AutoREST due to the increase in size.

@peombwa
Copy link
Contributor

peombwa commented Oct 31, 2022

Adding more information for context:

The table below shows the number of beta paths generated when the two convert settings ExpandDerivedTypesNavigationProperties and AppendBoundOperationsOnDerivedTypeCastSegments are enabled/disabled.

Convert Setting ExpandDerivedTypesNavigationProperties = true ExpandDerivedTypesNavigationProperties = false
AppendBoundOperationsOnDerivedTypeCastSegments = true 21,484 16,969
AppendBoundOperationsOnDerivedTypeCastSegments = false 19,342 14,846
@peombwa / @timayabi2020 we might need to verify the generated OpenAPI document with PowerShell to verify whether there are any modules that might break AutoREST due to the increase in size.

Thanks @irvinesunday for the breakdown!

Since OData type cast is currently not enabled in DevX API, we may see an increase of ~4500 paths in beta. Can we have an experimental deployment of DevX API with ExpandDerivedTypesNavigationProperties and AppendBoundOperationsOnDerivedTypeCastSegments set to true to facilitate our validation?

Also, is ExpandDerivedTypesNavigationProperties and AppendBoundOperationsOnDerivedTypeCastSegments set to false by default? i.e., maintains the current nav property expansion logic?

@irvinesunday
Copy link
Contributor Author

Adding more information for context:
The table below shows the number of beta paths generated when the two convert settings ExpandDerivedTypesNavigationProperties and AppendBoundOperationsOnDerivedTypeCastSegments are enabled/disabled.
Convert Setting ExpandDerivedTypesNavigationProperties = true ExpandDerivedTypesNavigationProperties = false
AppendBoundOperationsOnDerivedTypeCastSegments = true 21,484 16,969
AppendBoundOperationsOnDerivedTypeCastSegments = false 19,342 14,846
@peombwa / @timayabi2020 we might need to verify the generated OpenAPI document with PowerShell to verify whether there are any modules that might break AutoREST due to the increase in size.

Thanks @irvinesunday for the breakdown!

Since OData type cast is currently not enabled in DevX API, we may see an increase of ~4500 paths in beta. Can we have an experimental deployment of DevX API with ExpandDerivedTypesNavigationProperties and AppendBoundOperationsOnDerivedTypeCastSegments set to true to facilitate our validation?

Also, is ExpandDerivedTypesNavigationProperties and AppendBoundOperationsOnDerivedTypeCastSegments set to false by default? i.e., maintains the current nav property expansion logic?

Yes @peombwa, both settings are currently set to false in DevX API (ExpandDerivedTypesNavigationProperties explicitly; AppendBoundOperationsOnDerivedTypeCastSegments implicitly by default). We can definitely update devxapitest with the bumped up version of the conversion lib. and configure both settings to true. We can use this for our validation.

@irvinesunday irvinesunday enabled auto-merge (squash) October 31, 2022 19:58
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.

I'm a bit worried by the increase in the number of paths here, but I think we need to validate it before making a final decision. It'd be interesting to have a comparison of the newly added paths and see whether the service actually understands them. Especially for "deep" paths.

@irvinesunday irvinesunday merged commit 9a39340 into master Nov 1, 2022
@irvinesunday irvinesunday deleted the fix/is/expand-derived-containment branch November 1, 2022 13:56
@baywet
Copy link
Member

baywet commented Nov 1, 2022

(sorry about the auto-merge)

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.

ExpandDerivedTypesNavigationProperties setting granularity support
4 participants