-
Notifications
You must be signed in to change notification settings - Fork 262
Do not mark require property as read only #211
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
Conversation
@@ -549,7 +582,15 @@ internal void WriteAsSchemaProperties(IOpenApiWriter writer) | |||
writer.WriteProperty(OpenApiConstants.Discriminator, Discriminator?.PropertyName); | |||
|
|||
// readOnly | |||
writer.WriteProperty(OpenApiConstants.ReadOnly, ReadOnly, false); | |||
// In V2 schema if a property is part of required properties of schema, it cannot be marked as readonly. |
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.
schem [](start = 76, length = 5)
Nit. parent schema #Resolved
@@ -374,6 +374,27 @@ public void SerializeAsV3WithoutReference(IOpenApiWriter writer) | |||
/// Serialize <see cref="OpenApiSchema"/> to Open Api v2.0 | |||
/// </summary> | |||
public void SerializeAsV2(IOpenApiWriter writer) | |||
{ | |||
SerializeAsV2(writer, new List<string>(), null); |
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.
new List(), null [](start = 33, length = 25)
nit, suggest using named param for clarify #Resolved
/// </summary> | ||
public void SerializeAsV2WithoutReference(IOpenApiWriter writer) | ||
{ | ||
SerializeAsV2WithoutReference(writer, new List<string>(), null); |
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.
new List(), null) [](start = 50, length = 25)
nit, suggest using named param for clarify #Resolved
/// <param name="writer">The open api writer.</param> | ||
/// <param name="parentRequiredProperties">The list of required properties in parent schema.</param> | ||
/// <param name="propertyName">The property name that will be serialized.</param> | ||
internal void SerializeAsV2(IOpenApiWriter writer, IList<string> parentRequiredProperties, |
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.
IOpenApiWriter writer, IList parentRequiredProperties, [](start = 36, length = 62)
nit, suggest each parameter on its own line for readability #Resolved
/// <param name="writer">The open api writer.</param> | ||
/// <param name="parentRequiredProperties">The list of required properties in parent schema.</param> | ||
/// <param name="propertyName">The property name that will be serialized.</param> | ||
internal void SerializeAsV2WithoutReference(IOpenApiWriter writer, IList<string> parentRequiredProperties, |
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.
IOpenApiWriter writer, IList parentRequiredProperties, [](start = 52, length = 62)
nit, suggest each parameter on its own line for readability #Resolved
@@ -463,7 +494,8 @@ internal void WriteAsItemsProperties(IOpenApiWriter writer) | |||
writer.WriteExtensions(Extensions); | |||
} | |||
|
|||
internal void WriteAsSchemaProperties(IOpenApiWriter writer) | |||
internal void WriteAsSchemaProperties(IOpenApiWriter writer, IList<string> parentRequiredProperties, | |||
string propertyName) |
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.
nit, suggest each parameter on its own line for readability #Resolved
// In V2 schema if a property is part of required properties of schema, it cannot be marked as readonly. | ||
if (parentRequiredProperties.Contains(propertyName)) | ||
{ | ||
writer.WriteProperty(OpenApiConstants.ReadOnly, false, false); |
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.
writer.WriteProperty(OpenApiConstants.ReadOnly, false, false); [](start = 7, length = 71)
Let's just not write anything since the default value according to the spec is already false. #Resolved
} | ||
else | ||
{ | ||
writer.WriteProperty(OpenApiConstants.ReadOnly, ReadOnly, false); |
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.
false [](start = 73, length = 6)
nit, suggest named param for this last parameter #Resolved
// Arrange | ||
var outputStringWriter = new StringWriter(); | ||
var writer = new OpenApiJsonWriter(outputStringWriter); | ||
// Arrange |
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.
// Arrange [](start = 7, length = 15)
nit, not needed twice #Resolved
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.
Sorry I didn't get chance to review this last week, but I have a question. Why would we not do this as a validation rule? Putting validation constraints in the writer seems like an odd place to put them. A user may intentionally set a property as read-only and then be surprised when it doesn't get serialized. It does bring up the interesting question that maybe we should run the validator before writing. |
This comes back to my original question of V2 vs V3 rule. In V3, the document with required and readonly is valid; one will simply take precedence over the other. In V2, it is invalid.
The library should still write the best document it can that complies with the spec for V2 serialization. I don’t think we should write something that we know is wrong.
The correct workflow for the consumer should still be call the validator first, but then this is a V2 rule only. We can create the rule for our customer but we can’t add it to the Default set.
|
@PerthCharern My apologies. I didn't ready the V3 rules closely enough. I was not involved in discussion around that rule change. Preventing the V2 document from being invalid when writing seems like the best way of dealing with this. |
V2 serialization code changes to not allow a child property of a schema to be readonly if its inlcuded in the list of required properties of parent schema, to comply with V2 spec.