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

Factor out query param serialization into classes #683

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ClaytonPassmore
Copy link

Problem

The query parameter serialization logic in the RequestFactory class is complicated and difficult to test well. In addition, it does not consistently escape query parameter names and values (depends on whether you're using OAS 2 vs OAS 3).

Solution

Split out the parameter serialization logic into classes where logic can be reused and more easily unit tested.

Notes

This change was made with the intention of being 100% compatible with the previous implementation. The only exception is that OAS 2 query parameters are now escaped with CGI.escape when previously they were not.

If the change is an issue, I can tweak my changes to preserve the previous escaping behaviour.

File Structure

I added a new query_serializers folder with the following structure:

rswag-specs/lib/rswag/specs/query_serializers
├── collections
│   ├── multi_serializer.rb
│   └── xsv_serializer.rb
├── primitive_serializer.rb
└── specifications
    ├── oas3_serializer.rb
    └── swagger_serializer.rb
  • The collections folder contains serializers that work on arrays.
  • The specifications folder contains serializers that adhere to specifications like OAS 2 or OAS 3

I'm definitely open to changing class names and/or moving files around!

The changes I made are compatible with:

  • OAS2
  • OAS3
  • OAS3.1

Related Issues

Links to any related issues.

#621 fixed query parameter encoding for OAS 3, but that was never back-ported to OAS 2 serialization.

Checklist

  • Added tests
  • Changelog updated
  • Added documentation to README.md N/A
  • Added example of using the enhancement into test-app N/A

Steps to Test

The existing specs were left to show that I didn't break compatibility with the original implementation. I added unit tests for each added class to ensure 100% coverage.

Motivation

While I truly believe this code is more maintainable, I didn't make this change without selfish motivations. 😄

I currently have a fork of this gem where I have made changes to support a custom query parameter format. By splitting the logic out into classes, I can change my application to monkey patch specific classes to support my custom format without needing to maintain a fork.

@ClaytonPassmore
Copy link
Author

ClaytonPassmore commented Oct 18, 2023

Didn't realize spelling mattered. Hopefully that latest change gets CI passing.

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

1 participant