-
Notifications
You must be signed in to change notification settings - Fork 350
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 HasExplicitODataId to ODataResourceBase #2620
base: release-7.x
Are you sure you want to change the base?
Conversation
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
others look good to me. |
} | ||
|
||
[Fact] | ||
public void ReadSingletonWithIdSpecifiedTest() | ||
public void ReadSingletonWithODataIdSpecifiedTest() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no @id
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check line 81
@@ -213,6 +213,14 @@ public ICollection<ODataInstanceAnnotation> InstanceAnnotations | |||
set { this.SetInstanceAnnotations(value); } | |||
} | |||
|
|||
/// <summary>Gets or sets the value that shows if the resource has @odata.id or @id.</summary> | |||
/// <returns>true if the resource has @odata.id or @id, false otherwise.</returns> | |||
public bool HasExplicitODataId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal set? or totally as internal property?
Change "Gets or sets" for Internal set
@@ -213,6 +213,14 @@ public ICollection<ODataInstanceAnnotation> InstanceAnnotations | |||
set { this.SetInstanceAnnotations(value); } | |||
} | |||
|
|||
/// <summary>Gets or sets the value that shows if the resource has @odata.id or @id.</summary> | |||
/// <returns>true if the resource has @odata.id or @id, false otherwise.</returns> | |||
public bool HasExplicitODataId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
Issues
When deserializing a resource, we set the
Id
property in 2 ways:@odata.id
Id=1
and the resource is of typeCustomer
we generate an IdCustomers(1)
using theMetadataBuilder
.Also, since the Id can be set publicly, we could do as follows:
Considering the above, it's not possible to determine if
resource.Id
was populated from@odata.id
or not.In bulk operation handlers, if
resource.Id
was populated from@odata.id
we should callTryAddRelatedObject
since we are linking an existing object.Description
Briefly describe the changes of this pull request.
Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.