Skip to content

Fixes for issue 233 #236

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

Merged
merged 2 commits into from
Apr 5, 2018
Merged

Fixes for issue 233 #236

merged 2 commits into from
Apr 5, 2018

Conversation

darrelmiller
Copy link
Member

@darrelmiller darrelmiller commented Apr 4, 2018

See #233

  • Schema default is now parses as OpenApiAny instead of OpenApiString and so preserves typing information
  • V2 Header objects now render schema properties correctly
  • Casing of enum values has been corrected
  • collectionFormat is now serialized out in V2
  • Response bodies that have no schema are now rendered out and therefore correctly generate produces array in V2
  • externalDocs in Tags were not being parsed in either V2 or V3. This has been fixed.

The diff for the round-tripped Petstore V2 JSON document now has 12 differences. They include not rendering required: false because that is the default value and we don't render out default values for optional properties. Also, the input document has some empty parameter arrays. These are not rendered in the output. The final difference is that there are some request bodies that have a schema but no consumes. This is not possible to represent in OpenAPI V3. To work around this, we create a content object of type application/json. This ends up generating a consumes property in the output that isn't present in the input. This is the only semantic difference between the input and output versions and one that I believe is tolerable.

/cc @scott-lin

@@ -29,5 +29,6 @@ public enum SecuritySchemeType
/// Use OAuth2 with OpenId Connect URL to discover OAuth2 configuration value.
/// </summary>
[Display("openIdConnect")] OpenIdConnect

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit remove empty line

Copy link
Member

@Shwetap05 Shwetap05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

case "csv":
p.Style = ParameterStyle.Simple;
if (p.In == ParameterLocation.Query)
Copy link
Contributor

@PerthCharern PerthCharern Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cookie didn't exist in V2, so there is no need to try and translate csv for it,.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Totally forgot :)

{
writer.WriteProperty("collectionFormat", "pipes");
}
else if (this.Style == ParameterStyle.PipeDelimited)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PipeDelimited [](start = 58, length = 13)

SpaceDelimited

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

public static OpenApiTag LoadTag(ParseNode n)
{
var mapNode = n.CheckMapNode("tag");

var obj = new OpenApiTag();
var domainObject = new OpenApiTag();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

domainObject [](start = 16, length = 12)

Why is this called domainObject?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that when I cut and paste the code from one load to the next, I don't have to rename it :-)

Copy link
Contributor

@PerthCharern PerthCharern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@darrelmiller darrelmiller merged commit 12f1230 into master Apr 5, 2018
@darrelmiller darrelmiller deleted the dm/fix-issue233 branch April 5, 2018 20:45
@PerthCharern
Copy link
Contributor

@darrelmiller Darrel. I see that you already merged the commit but there are two issues above (1. Cookie 2. SpaceDelimited) which should be addressed for accuracy. Did you have a chance to take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants