ExpandDerivedTypesNavigationProperties setting granularity support #269

Activity

1. irvinesunday commented on Aug 30, 2022

   

   Irvine Sunday (irvinesunday)

   on Aug 30, 2022

   Contributor

   More actions

   The last time I tested this by generating derived types on navigation properties by going just one level deep I ended up with a significant number of paths generated.
   Maybe we can figure out the possibility of having a custom annotation that we can attach to the derived types that require expansion?
2. millicentachieng commented on Sep 14, 2022

   

   Millicent Achieng (millicentachieng)

   on Sep 14, 2022

   Contributor

   More actions

   Should we also consider having a list of entity types to expand in the settings?
3. Irvine Sunday (irvinesunday)

   self-assigned this

   on Sep 28, 2022
4. irvinesunday commented on Sep 28, 2022

   

   Irvine Sunday (irvinesunday)

   on Sep 28, 2022

   Last edited by irvinesunday

   Contributor

   More actions

   Should we also consider having a list of entity types to expand in the settings?

   That should probably be configured in the metadata as an annotation, as this would ideally be handled by the workload when they are updating their API.

   [Edited]
   On 2nd thought, having this provided via a config value in the convert settings doesn't also seem like a terrible idea. A list of the fully qualified names of derived types that should be expanded on can be provided via this config.
5. Irvine Sunday (irvinesunday)

   added this to the OData:1.2 milestone on Sep 29, 2022
6. irvinesunday commented on Sep 29, 2022

   

   Irvine Sunday (irvinesunday)

   on Sep 29, 2022

   Contributor

   More actions

   For context, at the moment, we generate about 7K paths for v1.0. Expanding the navigation properties of derived types for the single type cast segment only generates about 126K paths.

   Darrel (@darrelmiller) Vincent Biret (@baywet)
7. baywet commented on Sep 30, 2022

   

   Vincent Biret (baywet)

   on Sep 30, 2022

   Member

   More actions

   Should we also consider having a list of entity types to expand in the settings?

   I don't think this would be the best approach here, because I'd require the code that does the conversion (eg hidi, other tools) to know about the content of the CSDL in advance, which is not always the case (hidi, again).

   I believe this problem of over expansion of methods has multiple facets:

   • type cast segments should only have the methods of the target cast type under them, not the ones from the base type as those can be accessed by simply not casting.
   • methods should only be added to singletons, entity sets, entity sets entries, single contained navigation properties, collection contained navigation properties and collection contained navigation properties entries.
   • I believe Darrel (@darrelmiller) introduced an annotation to support unbound methods, but I don't remember the specifics.

   Then there's also the over expansion of navigation properties themselves:

   • Contains target is wrong at a bunch of places and we should either ask workloads to fix it or "fix it in post".
   • Only contained properties should be expanded, since the non-contained ones can be addressed via their canonical path.
   • I believe Darrel (@darrelmiller) introduced an annotation to force expansion or stop expansion in edge cases, but I don't remember the specifics.

   Once we iron out those implementation aspects, we should be in a position where, when the metadata is correct, we generate an accurately expanded description for it.
8. darrelmiller commented on Oct 6, 2022

   

   Darrel (darrelmiller)

   on Oct 6, 2022

   Member

   More actions

   I agree that we want to enable ExpandDerivedTypesNavigationProperties. However, before we do that we must ensure the following things:

   • Where only a subset of derived types are supported on a navigation property we need to add the DerivedTypeConstraint annotation to limit the surface area generated to only valid types. e.g. Manager is declared as directoryObject but can only be a user.
   • ContainsTargets that are wrong should be corrected to prevent auto expansion of navigation properties
   • Where we want non-contained navigation properties to have child paths, we must explicitly add annotations to describe the supported capabilities
   • As mentioned by Vincent, we must ensure that Operations bound to the base type are not regenerated for the path with the cast to the derived type.

   Vincent Biret (@baywet) There is no support for unbound operations.
9. irvinesunday commented on Oct 26, 2022

   

   Irvine Sunday (irvinesunday)

   on Oct 26, 2022

   Contributor

   More actions

   Andrew Omondi (@andrueastman) to get operations bound on derived types of contained navigation properties, you just need to set AppendBoundOperationsOnDerivedTypeCastSegments = true
   For the first example provided: https://docs.microsoft.com/en-us/graph/api/ediscovery-caseexportoperation-getdownloadurl?view=graph-rest-beta, the path can be generated as expected:

   

   👍React with 👍1Reacted by Andrew Omondi
10. andrueastman commented on Oct 31, 2022

    

    Andrew Omondi (andrueastman)

    on Oct 31, 2022

    Last edited by andrueastman

    ContributorAuthor

    More actions

    For context, at the moment, we generate about 7K paths for v1.0. Expanding the navigation properties of derived types for the single type cast segment only generates about 126K paths.

    With the change made in #302, does this mean that this is no longer valid/correct as we were unnecessarily expanding path with ContainsTarget=False?.
11. irvinesunday commented on Oct 31, 2022

    

    Irvine Sunday (irvinesunday)

    on Oct 31, 2022

    Last edited by irvinesunday

    Contributor

    More actions

    For context, at the moment, we generate about 7K paths for v1.0. Expanding the navigation properties of derived types for the single type cast segment only generates about 126K paths.

    With the change made in #302, does this mean that this is no longer valid/correct as we were unnecessarily expanding path with ContainsTarget=False?.

    Correct Andrew Omondi (@andrueastman). Please check out this table for the appr. path figures:

    👍React with 👍1Reacted by Andrew Omondi
12. Irvine Sunday (irvinesunday)

    closed this as completedin #302on Nov 1, 2022