Skip to content
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

DC-570: Update swagger to reflect new schema validation #157

Closed
wants to merge 3 commits into from

Conversation

Abulu123
Copy link
Contributor

@Abulu123 Abulu123 commented Dec 8, 2022

No description provided.

@Abulu123 Abulu123 changed the title Code checkin. Methods seem to look good tests are passing. DC-570: Update swagger to reflect new schema validation Dec 8, 2022
Copy link

@okotsopoulos okotsopoulos left a comment

Choose a reason for hiding this comment

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

Seeing a JSON schema, JSON example, and stringified JSON example all defined separately makes me concerned that a future change to the schema would not fully propagate to all of those places, leading to inconsistent documentation.

Had you considered consolidating to a single JSON schema with embedded examples, and/or referencing your example in common.openapi.yaml?

Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

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

I think a little more work can be done to clean this up, see my comments.

Also, did you try creating a catalog entry using this example? You should make sure it works OK before merging it up.

service/src/main/resources/api/schema.json Outdated Show resolved Hide resolved
common/openapi.yml Outdated Show resolved Hide resolved
Comment on lines +109 to +111
examples:
jsonObject:
summary: A sample object
Copy link
Member

Choose a reason for hiding this comment

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

I've looked into this more and I think we should be able to get externalValue to work. The problem is that the index.html that ships with swagger doesn't support it yet. See swagger-api/swagger-ui#5433

For now, you should look at the example HTML solutions on that GitHub issue and add one to our copy of index.html (which is in service/src/main/resources/templates).

@@ -0,0 +1,192 @@
{
"dct:title": "sint",
Copy link
Member

Choose a reason for hiding this comment

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

Since it wasn't obvious how to generate this example, can you document the steps to do it somewhere, such as in a file in the docs directory.

@@ -103,7 +103,7 @@
},
"contributors": {
"type": "array",
"items": { "$ref": "#/defs/contributor" }
"items": { "$ref": "#/$defs/contributor" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Deliberate change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

@lu-c lu-c left a comment

Choose a reason for hiding this comment

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

Mild approval - I do not know swagger that well, but nothing's jumping out as bad

@sonarcloud
Copy link

sonarcloud bot commented Dec 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@@ -103,7 +103,7 @@
},
Copy link
Member

@pshapiro4broad pshapiro4broad Dec 15, 2022

Choose a reason for hiding this comment

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

Can you fix this one too? The line above this should be

      "$ref": "#/$defs/samples"

Not sure why we're not seeing errors in our app due to typos like this. Maybe we need to be loading the schema in a stricter mode, if one exists.

@pshapiro4broad
Copy link
Member

I've moved these changes to #162

@pshapiro4broad pshapiro4broad deleted the DC-570-update-swagger branch January 12, 2023 21:14
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.

4 participants