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

Resource value build test #1346

Merged
merged 6 commits into from Dec 18, 2018

Conversation

Projects
None yet
3 participants
@xuzhg
Copy link
Member

commented Nov 29, 2018

Issues

This pull request fixes issue #xxx.

Description

Briefly describe the changes of this pull request.

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

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.

@xuzhg xuzhg force-pushed the xuzhg:ResourceValue branch 2 times, most recently from ceeac06 to 4ed9079 Nov 30, 2018

}
else if ((collection = property.Value as ODataCollectionValue) != null)
{
if (collection.Items != null && collection.Items != null && collection.Items.Any(t => t is ODataResourceValue))

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 13, 2018

Member

collection.Items != null [](start = 56, length = 24)

collection.Items != null [](start = 56, length = 24)

You check this twice -- you really don't want collection.Items to be null! :-) #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 13, 2018

Author Member

copied and pasted in the wrong place. Thanks for catching #Resolved

/// JsonNodeType.EndObject - the end object of the resource value object.
/// Post-Condition: almost anything - the node after the resource value (after the EndObject)
/// </remarks>
private ODataResourceValue ReadResourceValue(

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 13, 2018

Member

private ODataResourceValue ReadResourceValue [](start = 8, length = 44)

This is the duplicated code that concerns me. What changes to regular resource would need to be made here as well? #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 14, 2018

Author Member

I had your concerns, but I think they are two different using scenarios.
Basically, reading a value is very low level process, if we delegate the value reading to high level, in return, the high level will also call the low level process again. So, I am worry about the feasibility.

Of course, I did experiment using the top-level ODataReader. For example as below:

private ODataResourceValue ReadResourceValue(...)
{
   ODataReader reader = this.JsonLightInputContext.CreateUriParameterResourceReader(null, 
   structuredTypeReference == null ? null : structuredTypeReference.StructuredDefinition());
   return ReadResourceValue(reader);
 }

private static ODataResourceValue ReadResourceValue(ODataReader reader)
{
    while (reader.Read())
    {
         switch (reader.State)
          {
               case ODataReaderState.ResourceStart:
                   ....
          }
   }
}

but, So far, i can't make the above to work.

I think we are on the same page that we should have a method named "ReadResourceValue", the concern is about how to implement this private method.

Maybe we can postpone this detail implementation and focus on the step1 (adding class) and step2 (writing), hope we are on the same page about step1 and step2.
#Resolved

ODataResourceValue resourceValue,
bool isOpenPropertyType)
{
if (!this.currentPropertyInfo.IsTopLevel)

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 13, 2018

Member

would an ODataResourceValue ever be toplevel? #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 13, 2018

Author Member

We don't limit end user to create a property with value as ODataResourceValue.
In my opinion, We should not limit end user to write a top level property with value as "ODataResourceValue". #Resolved

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 18, 2018

Member

Now that we prohibit writing a resource value at the top level, can we get rid of this code that checks to see if it's top level?


In reply to: 241587425 [](ancestors = 241587425)

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 18, 2018

Author Member

@mikepizzo removed. #Resolved. #Resolved

public virtual void WriteResourceValue(
ODataResourceValue resourceValue,
IEdmTypeReference metadataTypeReference,
bool isTopLevel,

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 13, 2018

Member

is an ODataResource ever top level? #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 13, 2018

Author Member

Same. We don't limit end user to create a property with value as ODataResourceValue.
In my opinion, We should not limit end user to write a top level property with value as "ODataResourceValue". #Resolved

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 18, 2018

Member

Now that we've prohibited writing resource values at the top level, can we get rid of this code that handles writing a resource value at the top level?


In reply to: 241587501 [](ancestors = 241587501)

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 18, 2018

Author Member

@mikepizzo removed. #Resolved. #Resolved

@@ -61,6 +61,22 @@ internal static string ConvertToUriEnumLiteral(ODataEnumValue value, ODataVersio
return string.Format(CultureInfo.InvariantCulture, "{0}'{1}'", value.TypeName, value.Value);
}

/// <summary>
/// Converts the given string <paramref name="value"/> to an ODataResourceValue and returns it.
/// Tries in both JSON light and Verbose JSON.

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 13, 2018

Member

We should remove support for Verbose JSON (and certainly not advertise it in documentation) #Resolved

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 13, 2018

Author Member

#removed. #Resolved

@madansr7 madansr7 added this to the 7.6 milestone Dec 17, 2018

@madansr7 madansr7 requested a review from KanishManuja-MS Dec 17, 2018

@xuzhg xuzhg force-pushed the xuzhg:ResourceValue branch from 4ed9079 to f636300 Dec 18, 2018

@xuzhg

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

@mikepizzo I added the validation for the top level property. #Resolved

@mikepizzo

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

Code to handle validation looks good, but the methods that we call still seem to have code to handle writing a resource value at the top level; can we now safely remove that code?


In reply to: 448054198 [](ancestors = 448054198)

xuzhg added some commits Nov 13, 2018

Support OData resource (complex and entity) value
1. Move all ODataValue related classes into Value subfolder
2. Add ODataResourceValue class derived from ODataValue into Value
subfolder
3. Update the PublicApi
4. Verify the property value in ODataResource
5. Add the unit tests related to ODataResourceValue
Filter the Properties in ODataResource for property whose value is OD…
…ataResourceValue or Collection of ODataResourceValue
Support to write ODataResourceValue
    1. Write property value as ODataResourceValue
    2. Write Instance annotaiton value as ODataResourceValue
    3. Write Collection value item as ODataResourceValue
    4. Convert ODataResource to Uri literal
    5. Add the Unit test cases

Be noted, for Parameter value writing, so far for the resource,
resourceset and collection, ODL recommends to use corresponding writer
to write. so, in the changeset, we don't include the support for the
parameter value as ODataResourceValue writing.
Support read ODataResourceValue
  1. Read the value as ODataResourceValue
  2. Read the instance annotation value as ODataResourceValue
  3. Read the collection value item as ODataResourceValue
  4. Add reading Unit tests

Be noted:

1) for the parameter value reading, ODL recommends to use the
   corresonding resource, resourceset, collection reader to read the
   parameter value if the value is not primitive or enum. So, this change
   doesn't change the parameter reader.

2) for the collection reader, for the collection of resource, ODL
recommends to use the ODataResourceSet reader.So, not change for
collection reader.

@xuzhg xuzhg force-pushed the xuzhg:ResourceValue branch from f636300 to 774b562 Dec 18, 2018

@mikepizzo
Copy link
Member

left a comment

:shipit:

@xuzhg xuzhg merged commit eb0fc9a into OData:master Dec 18, 2018

2 checks passed

continuous-integration/vsts The build trigger has fired.
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.