-
Notifications
You must be signed in to change notification settings - Fork 159
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
#907: Enable read the untyped collection #905
Conversation
/// This list is used to hold other items, for example a primitive or a collection, etc. | ||
/// In the next major release, we should combine 'Resources' and 'Items' together. | ||
/// </summary> | ||
public IList<ODataItemWrapper> Items { get; } |
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.
Is there supposed to be code somewhere that reads the Items
property to actually populate the CLR type?
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.
What do you mean? @corranrogue9
To add a Kind property in the base type and implement to return different kind in different wrapper?
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.
I also have a similar question:
- Why do we need to make it public? Do we expect customers to use this?
- If we make it public, do we expect customers be both read and update the collection?
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.
Since 'Resources' is open/public, we can't stop customers adding new resources into that.
Same reason, we can't stop customers to modify the 'Items'.
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.
Items
is being introduced in this PR, so it doesn't exist in existing code. So my question was whether we need to expose it to customers and why.
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.
Customer may override the 'CreateResourceSet' method in ODataResourceSetDeserializer to access the 'Items'.
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.
I see.
src/Microsoft.AspNetCore.OData/Formatter/Wrapper/ODataReaderExtensions.cs
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Formatter/Wrapper/ODataReaderExtensions.cs
Show resolved
Hide resolved
@@ -301,11 +310,18 @@ private static void ReadResourceSet(ODataReader reader, Stack<ODataItemWrapper> | |||
ODataResourceSetWrapper resourceSetWrapper = new ODataResourceSetWrapper(resourceSet); | |||
if (itemsStack.Count > 0) | |||
{ | |||
ODataNestedResourceInfoWrapper parentNestedResourceInfo = (ODataNestedResourceInfoWrapper)itemsStack.Peek(); | |||
Contract.Assert(parentNestedResourceInfo != null, "this has to be an inner resource set. inner resource sets always have a nested resource info."); |
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.
Can itemStack
contain null
entries? Or does this assertion (and others like it in this class) make the assumption that the cast will return null
if the conversion fails? If the item at the top of the stack is not the expected type, an InvalidCastException
will be thrown. If we expect null, we should consider using itemStack.Peek() as ODataNestedResourceInfoWrapper
.
But I don't mind throwing the exception if that was by design.
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.
I know this doesn't apply to the new code change, but I've seen it in other places in this class and thought it's worth pointing out.
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.
ODL reading logic guarantees so far.
In my plan, I will retrieve the reading as a service and customer can customize the reading process. It's a future feature.
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.
The guarantee makes sense. My comment was more on the fact that Contact.Assert
verifies that the parentNestedResourceInfo
is not null
. However, if the cast would have failed, parentNestedResourceInfo
would not have been set to null, an exception would have been thrown, and therefore the Contact.Assert
would not have been reached.
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.
itemsStack.Peek() could return 'null', I think.
ODataNestedResourceInfoWrapper parentNestedResourceInfo = (ODataNestedResourceInfoWrapper)null;
is valid. We can sync it more, but it's no scope of this PR. :)
src/Microsoft.AspNetCore.OData/Formatter/Wrapper/ODataReaderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Formatter/Wrapper/ODataReaderExtensions.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// Gets the nested items of this ResourceSet. | ||
/// Since we have 'Resources' to contain ODataResource items in the collection (we have to keep it avoid breaking changes). | ||
/// This list is used to hold other items, for example a primitive or a collection, etc. |
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.
By this you mean that Items
and Resources
have no overlap? Or does Items
also include Resources
?
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.
It depends on the question:
Does the items order in a collection matter?
- If it does matter, I will make Items to include all in 'Resources'.
- If it doesn't matter, I will sperate them.
So, the idea is to replace
-public ILIist<ODataResourceWrapper> Resources {get;}
+public ILIist<ODataItemWrapper> Resources {get;}
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.
What about the current implementation? Does the order matter? Does Items
in the current implementation of this PR include resources?
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.
The PR doesn't include 'Resources' items into "Items".
In my opinion, the order doesn't matter. But we can change it anytime if the order does matter.
/// This list is used to hold other items, for example a primitive or a collection, etc. | ||
/// In the next major release, we should combine 'Resources' and 'Items' together. | ||
/// </summary> | ||
public IList<ODataItemWrapper> Items { get; } |
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.
I also have a similar question:
- Why do we need to make it public? Do we expect customers to use this?
- If we make it public, do we expect customers be both read and update the collection?
test/Microsoft.AspNetCore.OData.Tests/Formatter/Wrapper/ODataReaderExtensionsTests.Untyped.cs
Show resolved
Hide resolved
test/Microsoft.AspNetCore.OData.Tests/Formatter/Wrapper/ODataReaderExtensionsTests.Untyped.cs
Show resolved
Hide resolved
test/Microsoft.AspNetCore.OData.Tests/Formatter/Wrapper/ODataReaderExtensionsTests.Untyped.cs
Show resolved
Hide resolved
…tensions.cs Co-authored-by: Clément Habinshuti <[email protected]>
… 'Items' also to keep it same order
fixes #907.
also part of fixes #885. (No breaking changes)
Enable read the untyped collection.
For example:
where, awsTag could be a dynamic property, it contains primitive value item, resource value item.
ODL reads it and returns:
This PR is to handle the above scenario.