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

add guidance for transitioning from a structural collection to a navigation collection #551

Open
wants to merge 11 commits into
base: vNext
Choose a base branch
from

Conversation

corranrogue9
Copy link
Contributor

No description provided.

@corranrogue9 corranrogue9 requested a review from a team as a code owner July 5, 2024 17:14
```
Clients will now be able to refer to individual `bar`s using `prop1` as a key, and they can now remove those `bar`s using `DELETE` requests:
```http
DELETE /foos/{fooId}/bars/{some_prop1}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be barsAsEntities, right?

Suggested change
DELETE /foos/{fooId}/bars/{some_prop1}
DELETE /foos/{fooId}/barsAsEntities

</Key>
<Property Name="id" Type="Edm.String" Nullable="false" />
<Property Name="bars" Type="Collection(self.bar)" />
+ <NavigationProperty Name="barsAsEntities" Type="Collection(self.barAsEntity)" ContainsTarget="true" />
Copy link
Contributor

Choose a reason for hiding this comment

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

We wouldn't really want them to name barsAsEntities, would we? I feel like Entities isn't a concept the api consumer necessarily needs to understand.

```
The expectation is that `bars` and `barsAsEntities` are treated as two "views" into the same data.
To meet this expectation, workloads must:
1. Keep the properties consistent between `bar` and `barAsEntity`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to keep maintaining bar, or would it be better to deprecate bar and only add new properties to barAsEntity?

<PropertyRef Name="id" />
</Key>
<Property Name="id" Type="Edm.String" Nullable="false" />
<Property Name="bars" Type="Collection(self.bar)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, I would suggest deprecating bars in favor of barsAsEntities.

```

2. The default behavior for structural collections is to include them in the response payload for their containing entity. If this was the behavior of `foo` before, it must be preserved by **auto-expanding** the `bars` property now that it is a navigation property (because the default behavior for navigation properties is to **not** expand them).
3. Structural collections are updated using `PATCH` requests to replace the entire contents of the collection. The new navigation property must preserve this behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

to be clear, the patch request is to the containing entity (not the collection), and this is required only if the original structural property supported updating.

Suggested change
3. Structural collections are updated using `PATCH` requests to replace the entire contents of the collection. The new navigation property must preserve this behavior.
3. Structural collections can be updated using a `PATCH` request to the containing entity to replace the entire contents of the collection. If the service supported such updates to the structural collection, then updates to the new navigation property must preserve this behavior.


Collections of entity types are generally preferable to collections of structual types because collections of structural types must be updated as a single unit, meaning that they are overwritten entirely by new contents, rather than be updated relative to the existing contents.
Sometimes, structural collection properties are added to a type and then scenarios are discovered later that require a collection of entity types.
Take the following model with an entity type `foo` that has a collection of `bar`s:
Copy link
Collaborator

@OlgaPodo OlgaPodo Jul 11, 2024

Choose a reason for hiding this comment

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

These updates aim to address new issues in real-life modeling scenario. Can we use the actual Graph examples to inform our guidelines?

HTTP/1.1 204 No Content
```
The expectation is that `bars` and `barsAsEntities` are treated as two "views" into the same data.
To meet this expectation, workloads must:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't understand why we would enforce ambiguity of updating two places indefinitely? I think that we need to deprecate wrong modeling choice and have a single option to treat so called 'bars'.

@OlgaPodo OlgaPodo added Graph Guidelines This should be reviewed by Microsoft Grap team. Microsoft Graph This should be reviewed by Microsoft Grap team. labels Jul 11, 2024
"prop2": "another value"
},
...
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
]
],

</Key>
<Property Name="id" Type="Edm.String" Nullable="false" />
- <Property Name="bars" Type="Collection(self.bar)" />
+ <NavigationProperty Name="bars" Type="Collection(self.bar)" ContainsTarget="true" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

how does sdp look for workloads on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Graph Guidelines This should be reviewed by Microsoft Grap team. Microsoft Graph This should be reviewed by Microsoft Grap team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants