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

Add KeyAsSegmentSupported annotation term to Capabiliites vocabulary #1056

Merged
merged 2 commits into from Jan 30, 2018

Conversation

Projects
None yet
3 participants
@xuzhg
Member

xuzhg commented Jan 29, 2018

Issues

See issue at: https://issues.oasis-open.org/browse/ODATA-1134

Description

In OData 4.01 we introduced semantics around supporting the popular key-as-segment URL syntax, but we have no generic way for clients to know whether or not the service supports this sytnax.

This PR is to add a new boolean term to the capabilities vocabulary, KeyAsSegmentSupported

Checklist (Uncheck if it is not completed)

  • Test cases added
  • [x ] 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

This comment has been minimized.

Member

mikepizzo commented Jan 29, 2018

CapabilitiesVocabularyModel.cs only defines IEdmTerms for ChangeTrackingTerm (where-as CoreVocabularyModel.cs defines IEdmTerms for most/all of the terms defined in the vocabulary). Should we define a term for KeyAsSegmentTerm (and the other capabilities terms) or do we not think there is value in defining these IEdmTerms? #Closed

@xuzhg

This comment has been minimized.

Member

xuzhg commented Jan 29, 2018

@mikepizzo

Actually, we don't need to do that, because:

  1. We designed CapabilitiesVocabularyModel as an internal classes. Define KeyAsSegmentTerm can't help customer.

  2. We don't use it in our product codes. The customers should call "FindTerm("Org.OData...KeyAsSegmentSupported") to get the term.

  3. We planned to remove all the term variables defined in CoreVocabularyModel.cs, But, it's a breaking change because this class is public class.

Any thoughts?

@mikepizzo

This comment has been minimized.

Member

mikepizzo commented Jan 30, 2018

That's fine with me; I just wanted to be sure we had a good reason for the difference.

It appears that we do use ChangeTrackingTerm from the capabilities vocabulary, and presumably get a performance improvement for having it a static value and not having to look it up each time we use it.

Maybe consider adding a code comment to CapabilitiesVocabularyModel.cs calling out why we define an IEdmTerm for Changetracking but intentionally don't define IEdmTerm values for the other terms.


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

@xuzhg xuzhg merged commit c1507fd into OData:master Jan 30, 2018

2 checks passed

continuous-integration/vsts The build trigger has fired.
Details
license/cla All CLA requirements met.
Details

@xuzhg xuzhg deleted the xuzhg:AddKeyAsSegmentSupportedCapability branch Feb 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment