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

[#443] Update ResponseValidator to support nullable and required #527

Merged
merged 7 commits into from
Oct 19, 2022

Conversation

lukecivantos
Copy link
Contributor

Problem

Currently running into issues similar to what is described in this issue. When I include nullable or required for response headers it errors out because the response validator checks for .nil?.

Solution

In this PR I add changes to validate_headers! checking for required and nullable. And changing the error return appropriately. I also added tests as well as a subsection to the readme.

Notably when I ran ./ci/build.sh and ./ci/test.sh everything passes, but some of the generated files changed a bit. Happy to change accordingly if there are any recommendations.

This concerns this parts of the Open API Specification:

The changes I made are compatible with:

  • OAS2 – I didn't see anything about nullable in OAS2, but this change shouldn't impact current OAS2 users
  • OAS3
  • OAS3.1

Related Issues

Ability to make response headers not mandatory: #443

Checklist

  • Added tests
  • Changelog updated
  • Added documentation to README.md

Steps to Test or Reproduce

Can see the line causing the issue here.

When I run ./ci/test.sh everything works

To replicate can add a nullable and optional header response here.

Copy link
Collaborator

@jtannas jtannas left a comment

Choose a reason for hiding this comment

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

I like it :) @BookOfGreg approve once the package-lock is sorted out?

@@ -1,8 +1,22 @@
{
"name": "rswag-ui",
"version": "1.0.0",
"lockfileVersion": 1,
"lockfileVersion": 2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

These package-lock changes seem unrelated to the header changes.
Accidental inclusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! Yeah they appeared when I ran ./ci/build.sh – wasn't sure if I should keep or remove so I included. Just removed though 👍🏼

@jtannas jtannas requested a review from BookOfGreg June 30, 2022 17:58
@lukecivantos
Copy link
Contributor Author

Hey @BookOfGreg – wanted to follow up and see if you had gotten a chance to take a look at the changes here. Let me know if there's anything I can help clarify – thanks!

@lukecivantos
Copy link
Contributor Author

just following up here to see if there were any updates or if you had a chance to look @BookOfGreg cc @jtannas – thanks!

@lukecivantos
Copy link
Contributor Author

Hey @BookOfGreg & @jtannas – wanted to follow up to see if you could potentially review this week. Thanks!

@lukecivantos
Copy link
Contributor Author

Hey @BookOfGreg just wanted to follow up on getting a review. We're hoping to remove the monkey patch we're currently using as soon as possible. Let me know if there are any lingering questions.

cc @jtannas

Copy link
Collaborator

@jtannas jtannas left a comment

Choose a reason for hiding this comment

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

@lukecivantos Sorry for the slow reply. I've been focusing on other things for the past few months.

The changes look good 🙂 If you could merge the latest master and add an accompanying changelog note I'd be happy to merge this and release it in 2.7

@lukecivantos
Copy link
Contributor Author

Hey @jtannas sorry I missed this message – is it too late to include in 2.7? I could merge the latest master today

@lukecivantos
Copy link
Contributor Author

@jtannas Just merged the latest master and added the update to the changelog. If there are any other changes needed to get this into the 2.7 release I can make them today.

@jtannas
Copy link
Collaborator

jtannas commented Oct 18, 2022

I've already tagged the 2.7 release commit. I'll push 2.7 out tonight and up the 2.8 process.

@lukecivantos
Copy link
Contributor Author

Sounds good – let me know if there are any additional changes I need to make

@jtannas jtannas merged commit 00d6da6 into rswag:master Oct 19, 2022
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.

None yet

2 participants