-
Notifications
You must be signed in to change notification settings - Fork 350
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
Support optional facets on type definitions #2791
base: release-7.x
Are you sure you want to change the base?
Support optional facets on type definitions #2791
Conversation
89a2fe4
to
48c7ca7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this feature also be supported in CSDL JSON?
48c7ca7
to
ca7122d
Compare
@habbes Good catch. Made the relevant changes to ensure the facets get serialized for CSDL/JSON. The CSDL parser currently doesn't support deserializing CSDL/JSON. |
ca7122d
to
cd4b9ca
Compare
@@ -12,16 +12,48 @@ namespace Microsoft.OData.Edm.Csdl.Parsing.Ast | |||
internal class CsdlTypeDefinition : CsdlNamedElement | |||
{ | |||
private readonly string underlyingTypeName; | |||
private readonly int? maxLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't understand it, in my opinion, the facts should go to its underly type of the type definition, right?
For example, maxLength is for the underlying "Edm.String" primitive type.
@@ -754,6 +754,18 @@ internal override void WriteTypeDefinitionElementHeader(IEdmTypeDefinition typeD | |||
this.xmlWriter.WriteStartElement(CsdlConstants.Element_TypeDefinition); | |||
this.WriteRequiredAttribute(CsdlConstants.Attribute_Name, typeDefinition.Name, EdmValueWriter.StringAsXml); | |||
this.WriteRequiredAttribute(CsdlConstants.Attribute_UnderlyingType, typeDefinition.UnderlyingType, this.TypeDefinitionAsXml); | |||
|
|||
if (typeDefinition is IEdmFacetedTypeDefinition facetedTypeDefinition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use the underlyingType
to write the facts.
|
||
if (typeDefinition is IEdmFacetedTypeDefinition facetedTypeDefinition) | ||
{ | ||
this.jsonWriter.WriteOptionalProperty("$MaxLength", facetedTypeDefinition.MaxLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use the underlyingType
to write the facts
@@ -14,7 +14,7 @@ namespace Microsoft.OData.Edm.Csdl.CsdlSemantics | |||
/// <summary> | |||
/// Provides semantics for CsdlTypeDefinition. | |||
/// </summary> | |||
internal class CsdlSemanticsTypeDefinitionDefinition : CsdlSemanticsTypeDefinition, IEdmTypeDefinition, IEdmFullNamedElement | |||
internal class CsdlSemanticsTypeDefinitionDefinition : CsdlSemanticsTypeDefinition, IEdmTypeDefinition, IEdmFullNamedElement, IEdmFacetedTypeDefinition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since IEdmFacetedTypeDefinition
implements IEdmTypeDefinition
, do we need IEdmTypeDefinition
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KenitoInc I'd be more comfortable leaving it as is since there's no harm. If we merge IEdmFacetedTypeDefinition
into IEdmTypeDefinition
in the next major release, the action will be seamless.
cd4b9ca
to
35643dd
Compare
35643dd
to
1f59c49
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Issues
This pull request fixes #2246.
Description
This pull request is a follow up to #2779. It implements support for facets that appear on type definitions. For example,
Notes:
Unicode
facet, a default value oftrue
is implied. For the same reason, when serializing an string property, we currently don't write theUnicode
facet if the value istrue
ornull
. However, when it comes to aUnicode
facet specified on a type definition, we should be explicit when serializing the type definition, i.e., write theUnicode
facet if the value istrue
. This is because elements that use a type definition MUST NOT re-specify any facet specified on the type definition. Reading and writing the facet as explicitly specified makes it easy to validate this restriction.Checklist (Uncheck if it is not completed)
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.