Key Alias support #2411

ElizabethOkerio · 2022-05-19T06:41:01Z

Issues

This pull request fixes #xxx.

Key Alias support in ODL
Here's an example for Key alias:

"Category": {
  "$Kind": "EntityType",
  "$Key": [
    {
      "EntityInfoID": "Info/ID"
    }
  ],
  "Info": {
    "$Type": "self.EntityInfo"
  },
  "Name": {
    "$Nullable": true
  }
},
"EntityInfo": {
  "$Kind": "ComplexType",
  "ID": {
    "$Type": "Edm.Int32"
  },
  "Created": {
    "$Type": "Edm.DateTimeOffset",
    "$Precision": 0
  }
}

EntityInfoID is the alias and Info/ID is the path to the key.

Changes made in this PR:
New interfaces and Classes;

IEdmStructuralPropertyAlias : IEdmStructuralProperty - this interface has two properties: - IEnumerable Path { get; } and string PropertyAlias { get; }
EdmStructuralPropertyAlias : EdmProperty, IEdmStructuralPropertyAlias

EdmStructuredType
Added a new method
public EdmStructuralPropertyAlias AddStructuralPropertyAlias(IEdmTypeReference type, string alias, IEnumerable path) - This is the method that users will use to add key aliases.

type is the type of the complex property
alias is the key alias to use
path is a list of the path segments to the key property.

EdmEntityType
In the AddKeys(IEnumerable keyProperties) method, when looping through each property, we cast it into an IEdmStructuralPropertyAlias and check if the key has an alias or not. If it does, we loop through the keyProperties till we get to the last path segment which is the key and create an EdmStructuralPropertyAlias and add it to the declaredKeys IEnumerable.

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.

src/Microsoft.OData.Edm/Csdl/Parsing/SchemaJsonParser.cs

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

src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaJsonWriter.cs

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

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

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

src/Microsoft.OData.Edm/Csdl/Parsing/Ast/CsdlPropertyReference.cs

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

src/Microsoft.OData.Edm/EdmStructuralPropertyAlias.cs

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

gathogojr · 2022-05-31T07:32:05Z

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

@@ -157,6 +158,37 @@ public void AddKeys(IEnumerable<IEdmStructuralProperty> keyProperties)
                    this.declaredKey = new List<IEdmStructuralProperty>();
                }

+                //The Key property is the last segment in the path segment. 
+                IEdmStructuralPropertyAlias structuralPropertyAlias = property as IEdmStructuralPropertyAlias;
+                if (!string.IsNullOrEmpty(structuralPropertyAlias?.PropertyAlias))


Can the logic in this block be placed in a location where it can be shared given that it looks similar to that in src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsEntityTypeDefinition.cs?

Even better, could the new EdmStructuralPropertyAlias type take in the constructor the IEdmStructuralProperty that it's an alias for? Then we wouldn't have to traverse the Path sequence multiple times, and we could also not store the Path as an IEnumerable in EdmStructuralPropertyAlias, but instead as string. Except for this method, we always use Path in string.Join('/', Path), so if we could just store the original string instead, we would save all of those Join calls.

Actually, I think it makes the most sense to go one step further even: the EdmStructuralPropertyAlias constructor should do this work, otherwise there are far too many validations that need to happen in order to ensure the EdmStructuralPropertyAlias is not malformed.

See my comment on the constructor, there just so much validation that would need to happen

The EdmStructuralPropertyAlias is not the property with an alias in this case.

@ElizabethOkerio but PropertyAliasHelpers.GetKeyProperty() returns an IEdmStructuralPropertyAlias, right? And here you already have an IEdmStructuralPropertyAlias.

@habbes The IEdmStructuralPropertyAlias is for the first segment but this method PropertyAliasHelpers.GetKeyProperty() returns IEdmStructuralPropertyAlias for the last segment..

Okay, I see the IEdmStructuralPropertyAlias doesn't include the type of the target property, but EdmStructuralPropertyAlias. Makes sense to the traversal then.

But maybe we should have a fast path alternative when this happens to be an EdmStructuralPropertyAlias (which seems quite likely to be the case most of the times) to avoid creating a (possibly) dublicate EdmStructuralPropertyAlias

@habbes am not sure I understand this. So we are the ones who create the EdmStructuralPropertyAlias once we've gotten to the last segment on the path.

Am not sure there is any fast way of getting the last segment.. When we call the AddKeys method, we pass in the first segment property and the path. So to get to the last segment we have to traverse the whole path. If we decide on passing the last segment property to the AddKeys method, that will mean we are assuming the path is valid and no path traversal is needed.

corranrogue9 · 2022-06-01T20:46:14Z

src/Microsoft.OData.Edm/Csdl/Parsing/SchemaJsonParser.cs

+                    string pName = "";
+                    string pValue = "";
+                    v.ParseAsObject(p, (keyAlias, keyName) =>
+                    {
+                        pName = keyName.ParseAsString(p);
+                        pValue = keyAlias;
+                    });
+
+                    return new CsdlPropertyReference(pName, pValue, context.Location());


Is there any other scenario where we reach this branch? The ReportError message doesn't seem specific to this specific feature (even if the comment is specific to the feature). Maybe we should be more a little bit more defensive, something like:

Suggested change

string pName = "";

string pValue = "";

v.ParseAsObject(p, (keyAlias, keyName) =>

{

pName = keyName.ParseAsString(p);

pValue = keyAlias;

});

return new CsdlPropertyReference(pName, pValue, context.Location());

bool error = false;

string pName = null;

string pValue = null;

v.ParseAsObject(p, (keyAlias, keyName) =>

{

if (pName != null || pValue != null)

{

error = true;

return;

}

pName = keyName.ParseAsString(p);

pValue = keyAlias;

});

if (error || pName == null || pValue == null)

{

p.ReportError(EdmErrorCode.InvalidKeyValue, "It doesn't support the key object.");

}

else

{

return new CsdlPropertyReference(pName, pValue, context.Location());

}

Alternatively, we could do some Debug.Asserts if we want to be a little more flexible, but that might actually result in bigger issues for customers if something goes wrong.

so before we were returning the error because the feature was not supported. Now that the feature is being supported, there is no need to return the error. That's if an object is found then it is parsed. There is a check
if(v.ValueKind == JsonValueKind.Object).

I guess I'm just making sure that the only time we return that error is because the feature was not supported. Is that right?

Since the feature is supported, I think that error message would be misleading. If we want to validate against null key or value, I think the error message should be adjusted accordingly.

Can the key be null? I assume that would be invalid JSON?

the key cannot be null here.

corranrogue9 · 2022-06-01T21:13:48Z

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

+                    else
+                    {
+                        structuralProperty = this.FindProperty(keyProperty.PropertyName) as IEdmStructuralProperty;
+                    }


The nested if above means that we won't always reach this branch even if we should fallback to the existing behavior.

Suggested change

else

{

structuralProperty = this.FindProperty(keyProperty.PropertyName) as IEdmStructuralProperty;

}

structuralProperty = structuralProperty ?? this.FindProperty(keyProperty.PropertyName) as IEdmStructuralProperty;

src/Microsoft.OData.Edm/EdmStructuralPropertyAlias.cs

corranrogue9 · 2022-06-01T21:33:01Z

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

+                    IEdmStructuralProperty structuralProperty = null;
+                    if (!string.IsNullOrEmpty(keyProperty?.Alias))
+                    {
+                        string[] keyPropertySegments = keyProperty.PropertyName.Split('/');


How much can we trust the validation to have happened at this point? Is it possible we split a string like "foo//bar"? Because then we might have an empty string in keyPropertySegments

Because we are the ones creating this string and not the client - I think we should take care of all this things at the point of creating the string when writing to the csdl. I can add more validations there maybe.

corranrogue9 · 2022-06-01T21:41:46Z

src/Microsoft.OData.Edm/EdmStructuralPropertyAlias.cs

+        /// <param name="alias">The property alias.</param>
+        /// <param name="path">The path to the referenced property.</param>
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1720:IdentifiersShouldNotContainTypeNames", MessageId = "string", Justification = "defaultValue might be confused for an IEdmValue.")]
+        public EdmStructuralPropertyAlias(IEdmStructuredType declaringType, string name, IEdmTypeReference type, string alias, IEnumerable<string> path)


With all of this being public, we should probably validate everything. For example, don't we need to check the consistency of declaringType, name, and type? And shouldn't name always be the last element of path? And shouldn't path always have at least two elements?

@corranrogue9 Most of the validations are done when the base gets called. Like the declaringType and type. The name is the first element of the path. This is the complex property in the entity type that acts like a base for the path to the type that has the primitive type which will be used as key for the entity type.

I agree we need to add validations mostly on the path - if it has more than one element? etc.

corranrogue9 · 2022-06-01T21:42:56Z

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

+        /// <returns>Created structural property.</returns>
+        public EdmStructuralPropertyAlias AddStructuralPropertyAlias(IEdmTypeReference type, string alias, IEnumerable<string> path)
+        {
+            string propertyName = path.FirstOrDefault();


Shouldn't it be the last element in path?

@corranrogue9 This is the complex property that is being added to the entity type and not the key property. The key property is what is the last element in the path. This method was specifically added to support adding a complex property which is a link to a property(which in this case is the key property) with an alias

corranrogue9 · 2022-06-01T21:44:11Z

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

+        public EdmStructuralPropertyAlias AddStructuralPropertyAlias(IEdmTypeReference type, string alias, IEnumerable<string> path)
+        {
+            string propertyName = path.FirstOrDefault();
+            EdmStructuralPropertyAlias property = new EdmStructuralPropertyAlias(this, propertyName, type, alias, path);


We should probably confirm that this type does indeed have properties that follow path and result in a property with name propertyName and of type type.

Am not sure I understand this. So, here we are adding a property - propertyName of type type to this type

I'm sure most of my comments around this has been nonsensical regarding what an alias means, but I think
my points stands. Hopefully this example will help make sure I'm understanding correctly. There is a type buzz with a property foo/bar/fizz that is of type Edm.Decimal. The caller wishes to add an alias to buzz called frob that points to foo/bar/fizz.

In this case, doesn't frob have to be of type Edm.Decimal? If that's the case, then what I'm suggesting is that we need to walk the path parameter in this method to ensure that:

Each segment of the path is in fact valid for this

The type of the property pointed to by path must match type

The key for the type buzz is not in buzz. The key is in another type let's say fizz. But the type buzz has a complex property bar whose type is fizz and fizz has a property Id whose type is Edm.Int and which is the key for buzz. This is a 2-segment path. we could have a longer segment than that but that should be the flow. So the key for buzz will be represented as bar/Id. The alias is an identifier for this path bar/Id. Instead of using this path, we use an alias which does not have the slashes and that is easy to work with. The Alias will always be a string. The only segment of the path that is valid for this is the first segment. The other segments belong to other types.

so the method AddStructuralPropertyAlias is used to add bar to buzz when creating the model but because this property bar acts as a link to the actual key property for type buzz, and is not the key for type buzz, that's why we provide more information to it like the alias and the key path. The path is later processed to get to the last path segment to get the real key property for the type.

@ElizabethOkerio are there tests that verify that model validation is able to detect errors related to invalid property alias paths or types?

For performance, open/close principle, and single-responsibility reasons, even if the model validation catches it, the logic should move to the constructor.

@corranrogue9 I had @xuzhg mention that by design EDM does not validate the model unless the user explicitly chooses to do so. But maybe the context was different.

This logic is used in computing the declared key. I don't know whether it can move to the constructor.

@corranrogue9 So when we are adding the complex property to the model, we call AddStructuralPropertyAlias. Within this method we call EdmStructuralPropertyAlias. We can move the logic for checking the path and everything to the constructor. This will check for validity of the path. When we call AddKeys method for adding the key to the model, if we say that we pass in the last segment on the path and not do any validation because we already did it in the constructor, I think this will open up some loopholes where someone can just pass in something that is not even part of the path. That's why when we are adding the keys to the entity type's DeclaredKey, we still have to go through the path and make sure that whatever we are adding to the DeclaredKey is in the path and is the last segment on the path.

habbes · 2022-06-28T13:18:54Z

src/Microsoft.OData.Edm/Csdl/Parsing/SchemaJsonParser.cs

+                    string pName = "";
+                    string pValue = "";
+                    v.ParseAsObject(p, (keyAlias, keyName) =>
+                    {
+                        pName = keyName.ParseAsString(p);
+                        pValue = keyAlias;
+                    });
+
+                    return new CsdlPropertyReference(pName, pValue, context.Location());


Can the key be null? I assume that would be invalid JSON?

src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaJsonWriter.cs

habbes · 2022-06-28T14:06:13Z

src/Microsoft.OData.Edm/PropertyAliasHelpers.cs

+{
+    internal static class PropertyAliasHelpers
+    {
+        internal static IEdmStructuralProperty GetKeyProperty(IEdmStructuralProperty property, string[] keyPropertySegments, string propertyAlias)


nit: From the perspective of this method, it doesn't seem to make a difference whether the property is part of a key or not. I think a different name would be more appropriate. I don't have
a good candidate top of mind. Maybe something in the lines of CreatePropertyAliasFromSegments(startingProperty, propertySegments (or propertyPathSegments), alias)

This method loops through the path segments to get to the last segment which is the key property.

This method does not know that it's a key property, it only knows that it's a property at the end of the path. It's the caller of this method that has decided to use that property as part of a key. But nothing about this method (other than the parameter and method names) link it to a key.

habbes · 2022-06-28T14:14:36Z

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

@@ -157,6 +158,37 @@ public void AddKeys(IEnumerable<IEdmStructuralProperty> keyProperties)
                    this.declaredKey = new List<IEdmStructuralProperty>();
                }

+                //The Key property is the last segment in the path segment. 
+                IEdmStructuralPropertyAlias structuralPropertyAlias = property as IEdmStructuralPropertyAlias;
+                if (!string.IsNullOrEmpty(structuralPropertyAlias?.PropertyAlias))


@ElizabethOkerio what was the conclusion to this thread?

habbes · 2022-06-28T14:18:37Z

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

@@ -157,6 +158,37 @@ public void AddKeys(IEnumerable<IEdmStructuralProperty> keyProperties)
                    this.declaredKey = new List<IEdmStructuralProperty>();
                }

+                //The Key property is the last segment in the path segment. 
+                IEdmStructuralPropertyAlias structuralPropertyAlias = property as IEdmStructuralPropertyAlias;
+                if (!string.IsNullOrEmpty(structuralPropertyAlias?.PropertyAlias))


I'm confused a bit. If the property here is already an IEdmStructuralPropertyAlias why do you need to possibly duplicate EdmStructuralPropertyAlias via PropertyAliasHelpers.GetKeyProperty() instead of adding the property directly to the declared key?

@habbes so if it IEdmStructuralPropertyAlias then it means there is a path,, and the key is the last segment in that path. That's why we have to go through the whole path in this method PropertyAliasHelpers.GetKeyProperty() to get to the last segment which will be the key..And that's what we'll add to DeclaredKey

habbes · 2022-06-28T14:29:20Z

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

+        public EdmStructuralPropertyAlias AddStructuralPropertyAlias(IEdmTypeReference type, string alias, IEnumerable<string> path)
+        {
+            string propertyName = path.FirstOrDefault();
+            EdmStructuralPropertyAlias property = new EdmStructuralPropertyAlias(this, propertyName, type, alias, path);


@ElizabethOkerio are there tests that verify that model validation is able to detect errors related to invalid property alias paths or types?

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlWriterTests.cs

pull-request-quantifier-deprecated · 2022-07-06T12:50:36Z

This PR has 372 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +356 -16
Percentile : 77.2%

Total files changed: 17

Change summary by file extension:
.cs : +326 -16
.bsl : +30 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

gathogojr · 2022-07-19T05:11:42Z

src/Microsoft.OData.Edm/Csdl/Parsing/SchemaJsonParser.cs

                    // "$Key": [
                    //    {
                    //      "EntityInfoID": "Info/ID"
                    //    }
                    //  ]


Should have removed these lines as you removed the TODO, no?

gathogojr · 2022-07-19T05:13:00Z

src/Microsoft.OData.Edm/Csdl/Parsing/SchemaJsonParser.cs

                    // "$Key": [
                    //    {
                    //      "EntityInfoID": "Info/ID"
                    //    }
                    //  ]
-                    p.ReportError(EdmErrorCode.InvalidKeyValue, "It doesn't support the key object.");
+                    string pName = "";


Is there a variable by the name propertyName in scope such that we cannot use that in place of pName?

gathogojr · 2022-07-19T05:13:31Z

src/Microsoft.OData.Edm/Csdl/Parsing/SchemaJsonParser.cs

                    // "$Key": [
                    //    {
                    //      "EntityInfoID": "Info/ID"
                    //    }
                    //  ]
-                    p.ReportError(EdmErrorCode.InvalidKeyValue, "It doesn't support the key object.");
+                    string pName = "";
+                    string pValue = "";


Is there a variable by the name propertyValue in scope such that we cannot use that in place of pValue?

gathogojr · 2022-07-19T05:17:11Z

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

+                    }
+
+                }


Suggested change

}

}

}

}

gathogojr · 2022-07-19T05:19:17Z

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

+                            return null;
+                        }
+
+                        structuralProperty = PropertyAliasHelpers.GetKeyProperty(structuralProperty, keyPropertySegments, keyProperty.PropertyAlias);


Given that structuralProperty was assigned a value 5 or 6 line above here, could you add a comment above here to give someone an idea what happens when you call PropertyAliasHelpers.GetKeyProperty and what new value structuralProperty assumes.
On line 124 at least it's clear that, given Info/ID, structural property will be assigned a reference to the Info property... On this line it's not clear what is happening

gathogojr · 2022-07-19T05:42:35Z

src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaXmlWriter.cs

+            IEdmStructuralPropertyAlias prop = property as IEdmStructuralPropertyAlias;
+            if (prop == null)
+            {
+                this.WriteRequiredAttribute(CsdlConstants.Attribute_Name, property.Name, EdmValueWriter.StringAsXml);
+            }
+            else
+            {
+                this.WriteRequiredAttribute(CsdlConstants.Attribute_Name, string.Join("/", prop.Path), EdmValueWriter.StringAsXml);
+                this.WriteRequiredAttribute(CsdlConstants.Attribute_Alias, prop.PropertyAlias, EdmValueWriter.StringAsXml);
+            }


Suggested change

IEdmStructuralPropertyAlias prop = property as IEdmStructuralPropertyAlias;

if (prop == null)

{

this.WriteRequiredAttribute(CsdlConstants.Attribute_Name, property.Name, EdmValueWriter.StringAsXml);

}

else

{

this.WriteRequiredAttribute(CsdlConstants.Attribute_Name, string.Join("/", prop.Path), EdmValueWriter.StringAsXml);

this.WriteRequiredAttribute(CsdlConstants.Attribute_Alias, prop.PropertyAlias, EdmValueWriter.StringAsXml);

}

if (property is IEdmStructuralPropertyAlias prop)

{

this.WriteRequiredAttribute(CsdlConstants.Attribute_Name, string.Join("/", prop.Path), EdmValueWriter.StringAsXml);

this.WriteRequiredAttribute(CsdlConstants.Attribute_Alias, prop.PropertyAlias, EdmValueWriter.StringAsXml);

}

else

{

this.WriteRequiredAttribute(CsdlConstants.Attribute_Name, property.Name, EdmValueWriter.StringAsXml);

}

gathogojr · 2022-07-19T05:52:12Z

src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaJsonWriter.cs

+            {
+                this.jsonWriter.WriteStartObject();
+                this.jsonWriter.WritePropertyName(propertyAlias.PropertyAlias);
+                this.jsonWriter.WriteStringValue(string.Join("/", propertyAlias.Path));


This join operation is done in quite a few places. Could we have taken care of it by including a readonly property to the IEdmStructuralPropertyAlias interface and it's derivations that returns the concatenated thing? We could even apply an optimization there by doing the concatenation only once when the getter is first called and saving the result in a private field

gathogojr · 2022-07-19T05:53:30Z

src/Microsoft.OData.Edm/EdmStructuralPropertyAlias.cs

+    /// </summary>
+    public class EdmStructuralPropertyAlias : EdmProperty, IEdmStructuralPropertyAlias
+    {
+        private string propertyAlias;


Make propertyAlias readonly

gathogojr · 2022-07-19T05:53:46Z

src/Microsoft.OData.Edm/EdmStructuralPropertyAlias.cs

+    public class EdmStructuralPropertyAlias : EdmProperty, IEdmStructuralPropertyAlias
+    {
+        private string propertyAlias;
+        private IEnumerable<string> path;


Make path readonly

gathogojr · 2022-07-19T05:54:45Z

src/Microsoft.OData.Edm/EdmStructuralPropertyAlias.cs

+        public EdmStructuralPropertyAlias(IEdmStructuredType declaringType, string name, IEdmTypeReference type, string propertyAlias, IEnumerable<string> path)
+            : base(declaringType, name, type)
+        {
+            if (propertyAlias == null)


Is an empty string valid as property alias?

gathogojr · 2022-07-19T05:55:02Z

src/Microsoft.OData.Edm/EdmStructuralPropertyAlias.cs

+                throw new ArgumentNullException(nameof(propertyAlias));
+            }
+
+            if (path == null)


Is an empty collection valid as path?

gathogojr · 2022-07-19T05:57:44Z

src/Microsoft.OData.Edm/PropertyAliasHelpers.cs

+{
+    internal static class PropertyAliasHelpers
+    {
+        internal static IEdmStructuralProperty GetKeyProperty(IEdmStructuralProperty property, string[] keyPropertySegments, string propertyAlias)


Document this method so it's clear what it does

Is this method called from multiple places? If no, would it have been better for it to reside in the class from where it's called seeing it's the only method in this class and file...

gathogojr · 2022-07-19T06:00:49Z

src/Microsoft.OData.Edm/PropertyAliasHelpers.cs

+            IEdmStructuralProperty structuralProperty = property;
+            for (int i = 1; i < keyPropertySegments.Length; i++)
+            {
+                if (structuralProperty.Type.Definition.TypeKind == EdmTypeKind.Complex)


Could have used a switch...

gathogojr · 2022-07-19T06:01:37Z

src/Microsoft.OData.Edm/PropertyAliasHelpers.cs

+            return structuralProperty;
+
+        }


Suggested change

return structuralProperty;

}

return structuralProperty;

}

gathogojr · 2022-07-19T06:08:07Z

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

+                    if (structuralProperty != null)
+                    {
+                        IEdmStructuralProperty prop = PropertyAliasHelpers.GetKeyProperty(structuralProperty, keyPropertySegments, structuralPropertyAlias.PropertyAlias);
+                        if (prop != null)


There's nothing we should do if prop is null?

gathogojr · 2022-07-19T06:10:37Z

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

+            }
+            string propertyName = path.FirstOrDefault();


Suggested change

}

string propertyName = path.FirstOrDefault();

}

string propertyName = path.FirstOrDefault();

gathogojr · 2022-07-19T06:11:08Z

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

+        /// <returns>Created structural property.</returns>
+        public EdmStructuralPropertyAlias AddStructuralPropertyAlias(IEdmTypeReference type, string alias, IEnumerable<string> path)
+        {
+            if (path == null)


Is an empty collection valid as path?

xuzhg · 2023-02-01T18:50:20Z

@ElizabethOkerio Let's kick it off again:

here's my thought:

Basically, we can add a new interface:

    public interface IEdmPropertyRef: IEdmNamedElement
    {
        IEdmProperty Property { get; set; }

        string Path { get; set; }  // or IEdmPathExpression

        string Alias { get; set; }
    }

So, change:

public class EdmEntityType
{
-       private List<IEdmStructuralProperty> declaredKey;
+       private List<IEdmPropertyRef> declaredKey;

+       public void AddKeys(params IEdmPropertyRef[] keyProperties)
+       {}

+       public void AddKeys(params IEdmPropertyRef[] keyProperties)
+       {}
}

Change the existing AddKeys, and add others.

pull-request-quantifier-deprecated bot added the Large label May 19, 2022

ElizabethOkerio requested review from habbes, gathogojr, KenitoInc, Nthemba and xuzhg May 19, 2022 08:54

ElizabethOkerio force-pushed the feature/alias_key_spuurt branch 2 times, most recently from 9589777 to c282be8 Compare May 19, 2022 15:28

habbes reviewed May 26, 2022

View reviewed changes

ElizabethOkerio requested a review from corranrogue9 May 27, 2022 09:01

ElizabethOkerio force-pushed the feature/alias_key_spuurt branch from ed1f40b to aa2c3a1 Compare May 30, 2022 06:59

ElizabethOkerio requested a review from mikepizzo May 30, 2022 07:02

habbes reviewed May 30, 2022

View reviewed changes