Enable Base term for IEdmTerm #1962

xuzhg · 2020-12-17T19:36:22Z

Issues

This pull request fixes issue #1955. & #1816

Description

*14.1.1 Specialized Term

A term MAY specialize another term in scope by specifying it as its base type.

When applying a term with a base term, the base term MUST also be applied with the same qualifier, and so on until a term without a base term is reached.

Attribute BaseTerm

The value of BaseTerm is the qualified name of the base term.

This PR is to enable the base term.

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.

mikepizzo · 2020-12-17T20:56:28Z

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/Parsing/SchemaJsonParserTests.cs

        [Fact]
-        public void ParseCsdlTermWithMembersWorksAsExpected()
+        public void ParseCsdlTermWithBaseTermAndMembersWorksAsExpected()


ParseCsdlTermWithBaseTermAndMembersWorksAsExpected [](start = 20, length = 50)

Can you also add some XML test variants of reading/writing terms with base values? #Resolved

There's no related test cases to test part of an XML Element.

I added a whole write test cases to cover it.

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

mikepizzo · 2020-12-17T20:56:49Z

...lTests/Microsoft.OData.Edm.Tests/Csdl/Serialization/EdmModelCsdlSerializationVisitorTests.cs

@@ -815,6 +815,34 @@ public void VerifyTermWrittenCorrectly()
 }");
        }

+        [Fact]
+        public void VerifyTermWithBaseTermWrittenCorrectly()


VerifyTermWithBaseTermWrittenCorrectly [](start = 20, length = 38)

Please add XML Variation as well. #Resolved

Yes. Do you mean "Line 827 - 828"

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

Yes; missed that -- thanks.

In reply to: 545399808 [](ancestors = 545399808,545398455)

mikepizzo · 2020-12-17T21:01:28Z

src/Microsoft.OData.Edm/Vocabularies/Annotations/IEdmTerm.cs

+        /// <summary>
+        /// Gets the base term of this term.
+        /// </summary>
+        IEdmTerm BaseTerm { get; }


This is my only real concern with the PR. We need to convince ourselves that we are the only ones implementing this interface, as anyone else providign a custom implementation will break when they update to the new library and have to modify their code to implement this property.

The fallback would be to define an IEdmTermWithBase : IEdmTerm, but that would require logic each place we consume IEdmTerm that we care about base to handle base as well, if the instance of IEdmTerm can be cast to IEdmTermWithBase. We could add IEdmTermWithBase in 7.9 and clean it up in 8.0, or we can convince ourselves that there aren't any custom implementations of IEdmTerm that we would break.

That's also my concern. However, i have the following two reasons:

To add new into a public interface is less "harmful" than to remove or to change element in the public interface.

I can't find any reason why there's a customer who need to implement this interface.

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

I think I'm understand the high-level concern but missing the specifics for our case. Could we not have this property be of type BaseTerm : IEdmTerm that has an internal constructor so we have full control over the behavior?

mikepizzo · 2020-12-17T21:05:03Z

What are the ramifications of introducing a base Term? Do consumers of the API need to do anything different?

i.e., for structured types, a base type implies that all of the properties of the base type are available on the derived type. a client that looks only at the declared properties of the derived type will miss the properties inherited from the base type.

Are there similar ramifications for consumers of a derived term?

mikepizzo · 2020-12-17T21:07:03Z

public interface IEdmTerm : IEdmSchemaElement

Need to update public api

Refers to: src/Microsoft.OData.Edm/Vocabularies/Annotations/IEdmTerm.cs:12 in 22945d8. [](commit_id = 22945d8, deletion_comment = False)

xuzhg · 2020-12-17T21:31:10Z

Quick answer is that" It's the same usage of "Base type" and "Base Term".

When we create the annotation, we can create the PropertyValue for the "Property" defined in the base term.
However, OData.Edm lib doesn't have the process to verify that "PropertyName" of the Record is defined or not.
Let me add a test case to cover it.

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

gathogojr · 2020-12-18T06:46:30Z

src/Microsoft.OData.Edm/Validation/EdmErrorCode.cs

@@ -1383,6 +1383,11 @@ public enum EdmErrorCode
        /// </summary>
        AnnotationApplyToNotAllowedAnnotatable = 400,

+        /// <summary>
+        /// This ter, type is part of a cycle.


Fix docstring

gathogojr · 2020-12-18T06:47:28Z

src/Microsoft.OData.Edm/Schema/CyclicTerm.cs

+        {
+        }
+    }
+}


nit: Add newline

gathogojr · 2020-12-18T06:49:45Z

test/PublicApiTests/BaseLine/Microsoft.OData.PublicApi.net45.bsl

@@ -7742,21 +7756,6 @@ public abstract class Microsoft.OData.Client.OperationResponse {
    int StatusCode  { public get; }
 }

-[


This is from a merged PR. Have you rebased your branch?

gathogojr · 2020-12-18T06:49:58Z

test/PublicApiTests/BaseLine/Microsoft.OData.PublicApi.netstandard2.0.bsl

@@ -7774,21 +7788,6 @@ public abstract class Microsoft.OData.Client.OperationResponse {
    int StatusCode  { public get; }
 }

-[


This is from a merged PR. Have you rebased your branch?

gathogojr · 2020-12-18T06:51:52Z

test/PublicApiTests/BaseLine/Microsoft.OData.PublicApi.netstandard2.0.bsl

@@ -3080,6 +3080,7 @@ public enum Microsoft.OData.Edm.Validation.EdmErrorCode : int {
    BadCyclicComplex = 227
    BadCyclicEntity = 229
    BadCyclicEntityContainer = 228
+    BadCyclicTerm = 401


Assigning value of 401 seems to have affected other enum values. Is that okay?

corranrogue9 · 2021-10-13T22:09:43Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsTerm.cs

+                    // Evaluate the inductive step to detect cycles.
+                    // Overriding BaseTerm getter from concrete type implementing IEdmTerm will be invoked to
+                    // detect cycles. The object assignment is required by compiler only.
+                    IEdmTerm baseTerm2 = baseTerm.BaseTerm;


I'm not familiar, but does this mean that we expect baseTerm.BaseTerm to throw or something if there is a cycle?

corranrogue9 · 2021-10-13T22:10:45Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsTerm.cs

+        protected string GetCyclicBaseTermName(string baseTermName)
+        {
+            IEdmTerm schemaBaseTerm = this.Context.FindTerm(baseTermName);
+            return (schemaBaseTerm != null) ? schemaBaseTerm.FullName() : baseTermName;


super nit: I prefer the null propagation syntax here: return schemaBaseTerm?.FullName() ?? baseTermName

Enable Base term for IEdmTerm

22945d8

xuzhg requested review from gathogojr, mikepizzo and Sreejithpin December 17, 2020 19:37

mikepizzo reviewed Dec 17, 2020

View reviewed changes

Address the comments

fa3b2ff

gathogojr reviewed Dec 18, 2020

View reviewed changes

src/Microsoft.OData.Edm/Schema/CyclicTerm.cs

{

}

}

}

Copy link

Contributor

gathogojr Dec 18, 2020

Choose a reason for hiding this comment

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

nit: Add newline

gathogojr reviewed Dec 18, 2020

View reviewed changes

corranrogue9 reviewed Oct 13, 2021

View reviewed changes

xuzhg marked this pull request as draft January 4, 2022 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Base term for IEdmTerm #1962

Enable Base term for IEdmTerm #1962

xuzhg commented Dec 17, 2020

mikepizzo Dec 17, 2020 •

edited by xuzhg

Loading

xuzhg Dec 17, 2020

mikepizzo Dec 17, 2020 •

edited

Loading

xuzhg Dec 17, 2020

mikepizzo Dec 17, 2020

mikepizzo Dec 17, 2020

xuzhg Dec 17, 2020

corranrogue9 Oct 13, 2021

mikepizzo commented Dec 17, 2020

mikepizzo commented Dec 17, 2020

xuzhg commented Dec 17, 2020

gathogojr Dec 18, 2020

gathogojr Dec 18, 2020

gathogojr Dec 18, 2020

gathogojr Dec 18, 2020

gathogojr Dec 18, 2020

corranrogue9 Oct 13, 2021

corranrogue9 Oct 13, 2021

Enable Base term for IEdmTerm #1962

Are you sure you want to change the base?

Enable Base term for IEdmTerm #1962

Conversation

xuzhg commented Dec 17, 2020

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

mikepizzo Dec 17, 2020 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo commented Dec 17, 2020

mikepizzo commented Dec 17, 2020

xuzhg commented Dec 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo Dec 17, 2020 •

edited by xuzhg

Loading

mikepizzo Dec 17, 2020 •

edited

Loading