Add annotation term for Org.OData.Community.UrlEscape.V1.UrlEscapeFunction. by biaol-odata · Pull Request #1307 · OData/odata.net

biaol-odata · 2018-10-27T00:28:04Z

Issues

This pull request is to add OData.Community.UrlEscape.V1.UrlEscapeFunction annotation, so that it is available for use in URI parsing (pending).

Description

Refactor community annotation schema with the additional schema ns "OData.Community.UrlEscape.V1".

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 · 2018-10-29T16:33:29Z

The name space and the alias should be better, not only for the "UrlEscape". I think we will add more into this schema for more community terms. Check with @mikepizzo for better naming. #Closed

As we discussed, I have moved the UrlEscapeFunction term into a new CommunityVocabularies.xml, keeping existing AlternativeKeysVocabularies.xml unchanged (will be deprecated later). #Closed

Resolved: have updated namespace and alias to community.

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

xuzhg · 2018-10-29T16:43:15Z

+        /// Parse Alternate Keys Vocabulary Model from CommunityVocabularies.xml
        /// 
        static AlternateKeysVocabularyModel()
+        {


From the C# spec:

A static constructor is called automatically to initialize the class before the first instance is created or any static members are referenced.

I am wondering about the order of execution about the static field "Instance" and the static constructor. For example:

If I access the "AlternateKeysVocabularyModel.Instance", before any code, what will happen?
If static constructor executed before field access, "Instance" is a null reference.

Maybe I am wrong, please make sure the calling is in correct order.
#Resolved

As we checked off line, the AlternateKeysVocabularyModel.Instance could be a null reference depends on initialization order of static intances / fields. But when it is null, it will be initialized first before it's used. So the static field should not be null when it is used. #Resolved

xuzhg · 2018-10-29T16:43:40Z

+    /// 

            
+    /// Representing Url Escape Vocabulary Model.
+    /// 
+    public static class UrlEscapeVocabularyModel


Maybe it's better in a new file. #Closed

xuzhg · 2018-10-29T16:44:05Z

+
+        static UrlEscapeVocabularyModel()
+        {
+            UrlEscapeFunctionTerm = Instance.FindDeclaredTerm(UrlEscapeVocabularyConstants.UrlEscapeFunction);


Same as the execution of the order. #Resolved

xuzhg · 2018-10-29T16:45:10Z

+            StringWriter swAlternateKeys = new StringWriter();
            IEnumerable errors;
-            using (var xw = XmlWriter.Create(sw, new XmlWriterSettings { Indent = true, Encoding = Encoding.UTF8 }))
+            using (var xwAlternateKeys     = XmlWriter.Create(swAlternateKeys,     new XmlWriterSettings { Indent = true, Encoding = Encoding.UTF8 }))


do "whitespace" clean

xuzhg · 2018-10-29T16:45:39Z

+
+            string output = swUrlEscape.ToString();
+            Assert.True(expectedUrlEscape == output, $"expectedUrlEscape not matching");
+


xuzhg · 2018-10-29T16:48:06Z

+        }
+
+        [Fact]
+        public void TestUrlEscapeVocabularyModel()


Add test cases in:

Create Edm function with the "urlEscape" annotation and can write it to CSDL ((inline and outline)

Can reader the CSDL with the Edm function decorated with "UrlEscape" annotation, (inline and outline)

xuzhg · 2018-10-29T16:50:47Z

@@ -1,5 +1,5 @@
 //---------------------------------------------------------------------
-// 


Maybe it's better to "[Obsolete]" for the existing "AlternateKeys*" classes and files.

Meantime, to create new for "Community*" classes and files?

biaol-odata · 2018-10-29T21:44:27Z

@xuzhg @mikepizzo I have created a new CommunityVocabularies.xml to contain the UrlEscapeFunction term in the commit above. Can you please take a look? #Resolved

- Move the UrlEscapeFuntion into new Community vocabularies; AlternateKeys will be moved later.

xuzhg · 2018-10-31T22:09:24Z

+namespace Microsoft.OData.Edm.Vocabularies.Community.V1
+{
+    /// 

            
+    /// Representing Url Escape Vocabulary Model.


Url Escape [](start = 21, length = 10)

it should be "Community" #Closed

fixed. #Closed

xuzhg · 2018-10-31T22:11:48Z

+        private readonly IEdmModel model = CommunityVocabularyModel.Instance;
+
+        [Fact]
+        public void TestCommunitySchemaModel()


Schema [](start = 33, length = 6)

TestCommunityVocabularyModel ? #Resolved

updated. #Resolved

xuzhg · 2018-10-31T22:14:50Z

+
+namespace Microsoft.OData.Edm.Tests.Vocabularies
+{
+    public class CommunityVocabularyTests


CommunityVocabularyTests [](start = 17, length = 24)

Maybe add test cases:

In the CsdlWriter, create the model with function decorated with this term?

In the CsdlReader, read the csdl with function decorated with this term. #Resolved

Yes these tests will be added, as part of EDM model validation test along with bound function, in the task for upper layer and as a separate PR. This PR here is just to add the CommunityVocabulary. #Resolved

xuzhg

xuzhg · 2018-10-31T22:18:49Z

The vocabulary and related codes looks good to me. Biao said he will add more test cases when he implements the validation rules. So, I approved it.

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

mikepizzo · 2018-11-07T18:09:39Z

converting [](start = 77, length = 11)

The function doesn't do the conversion; maybe "Annotates a function to be substituted for a colon-escaped segment in a Url path." #Closed

mikepizzo · 2018-11-07T18:15:40Z

+    public static class CommunityVocabularyConstants
+    {
+        /// OData.Community.V1.UrlEscapeFunction 
+        internal const string UrlEscapeFunction = "Org.OData.Community.V1.UrlEscapeFunction";


internal [](start = 8, length = 8)

Either make this public or make the class internal. Unless we think there is a client scenario that requires it, I would make the class internal -- we can always make it public later. #Closed

updated.

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

mikepizzo

🕐

mikepizzo

biaol-odata · 2018-11-14T19:08:09Z

@mikepizzo and @xuzhg: Thanks for the CR! I will add additional terms per incoming requests.

…on. (OData#1307) * Add annotation term for OData.Community.UrlEscape.V1.UrlEscapeFunction. - Move the UrlEscapeFuntion into new Community vocabularies; AlternateKeys will be moved later. * Update per CR comments. * CR updates from Mike's comments.

…on. (#1307) * Add annotation term for OData.Community.UrlEscape.V1.UrlEscapeFunction. - Move the UrlEscapeFuntion into new Community vocabularies; AlternateKeys will be moved later. * Update per CR comments. * CR updates from Mike's comments.

biaol-odata requested review from madansr7, mikepizzo, raheph and xuzhg October 27, 2018 00:28

biaol-odata force-pushed the oneDriveGetByPathSegment branch 6 times, most recently from 7dd5ebd to c85867d Compare October 27, 2018 21:08

biaol-odata self-assigned this Oct 27, 2018

biaol-odata added under-review and removed under-review labels Oct 27, 2018

xuzhg reviewed Oct 29, 2018

View reviewed changes

biaol-odata force-pushed the oneDriveGetByPathSegment branch from c85867d to 30bfbd2 Compare October 29, 2018 21:38

biaol-odata closed this Oct 29, 2018

biaol-odata reopened this Oct 29, 2018

biaol-odata force-pushed the oneDriveGetByPathSegment branch 4 times, most recently from 85ab2a5 to 91e371f Compare October 31, 2018 08:18

Add annotation term for OData.Community.UrlEscape.V1.UrlEscapeFunction.

4783300

- Move the UrlEscapeFuntion into new Community vocabularies; AlternateKeys will be moved later.

biaol-odata force-pushed the oneDriveGetByPathSegment branch from 91e371f to 4783300 Compare October 31, 2018 18:44

xuzhg reviewed Oct 31, 2018

View reviewed changes

xuzhg approved these changes Oct 31, 2018

View reviewed changes

Update per CR comments.

7db8bf7

mikepizzo reviewed Nov 7, 2018

View reviewed changes

mikepizzo suggested changes Nov 7, 2018

View reviewed changes

biaol-odata added the Ready for review Use this label if a pull request is ready to be reviewed label Nov 7, 2018

biaol-odata force-pushed the oneDriveGetByPathSegment branch from 571f620 to 16e3908 Compare November 8, 2018 00:40

CR updates from Mike's comments.

00d649e

biaol-odata force-pushed the oneDriveGetByPathSegment branch from 16e3908 to 00d649e Compare November 8, 2018 01:10

mikepizzo approved these changes Nov 14, 2018

View reviewed changes

biaol-odata closed this Nov 14, 2018

biaol-odata reopened this Nov 14, 2018

biaol-odata merged commit 4c9c57e into OData:master Nov 14, 2018

biaol-odata deleted the oneDriveGetByPathSegment branch November 14, 2018 20:09

biaol-odata changed the title ~~Add annotation term for OData.Community.UrlEscape.V1.UrlEscapeFunction.~~ Add annotation term for Org.OData.Community.UrlEscape.V1.UrlEscapeFunction. Nov 19, 2018


		string output = swUrlEscape.ToString();
		Assert.True(expectedUrlEscape == output, $"expectedUrlEscape not matching");

		@@ -1,5 +1,5 @@
		//---------------------------------------------------------------------
		//

Conversation

biaol-odata commented Oct 27, 2018

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

Uh oh!

xuzhg Oct 29, 2018 • edited by mikepizzo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biaol-odata Oct 29, 2018 • edited by mikepizzo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikepizzo Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 29, 2018 • edited by biaol-odata Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biaol-odata Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 29, 2018 • edited by mikepizzo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 29, 2018 • edited by biaol-odata Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

biaol-odata commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuzhg Oct 31, 2018 • edited by mikepizzo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biaol-odata Oct 31, 2018 • edited by mikepizzo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 31, 2018 • edited by biaol-odata Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biaol-odata Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 31, 2018 • edited by biaol-odata Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biaol-odata Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg left a comment

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Oct 31, 2018

Uh oh!

mikepizzo Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

xuzhg Oct 29, 2018 •

edited by mikepizzo

Loading

biaol-odata Oct 29, 2018 •

edited by mikepizzo

Loading

xuzhg Oct 29, 2018 •

edited by biaol-odata

Loading

biaol-odata Oct 29, 2018 •

edited

Loading

xuzhg Oct 29, 2018 •

edited by mikepizzo

Loading

xuzhg Oct 29, 2018 •

edited by biaol-odata

Loading

biaol-odata commented Oct 29, 2018 •

edited

Loading

xuzhg Oct 31, 2018 •

edited by mikepizzo

Loading

biaol-odata Oct 31, 2018 •

edited by mikepizzo

Loading

xuzhg Oct 31, 2018 •

edited by biaol-odata

Loading

biaol-odata Oct 31, 2018 •

edited

Loading

xuzhg Oct 31, 2018 •

edited by biaol-odata

Loading

biaol-odata Oct 31, 2018 •

edited

Loading

mikepizzo Nov 7, 2018 •

edited

Loading

mikepizzo Nov 7, 2018 •

edited

Loading