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

fix(execute): empty field added to multi-value parameters #3367

Merged
merged 5 commits into from
Feb 12, 2024

Conversation

glowcloud
Copy link
Contributor

Refs swagger-api/swagger-ui#5176

Fixes the issue for when no value is selected, sending an empty string in an array after selecting the '--' option is a separate issue in Swagger UI.

@glowcloud glowcloud requested a review from char0n February 9, 2024 11:13
@glowcloud glowcloud self-assigned this Feb 9, 2024
@glowcloud
Copy link
Contributor Author

@char0n char0n self-assigned this Feb 9, 2024
@char0n
Copy link
Member

char0n commented Feb 9, 2024

Please add a test as well for the cases we're trying to fix with this change.

@char0n
Copy link
Member

char0n commented Feb 9, 2024

Added the same check for query despite it working without it - when we had an empty array, it would go inside the if block but we somehow wouldn't have the empty field. Not sure if this change is needed

We need to know for sure, why this happens.

Let me know if the same changes should also be made for query and header here https://github.com/swagger-api/swagger-js/blob/fix-empty-field-for-multi-value-parameters/src/execute/swagger2/parameter-builders.js

I think it will be related to the result of analysis related to the previous quote

@glowcloud
Copy link
Contributor Author

glowcloud commented Feb 12, 2024

The if (value) check is fine for query and OAS3, because of how we format queries later. At some point we use formatKeyValue, which in case of OAS3 will also go into formatKeyValueBySerializationOption, where here

if (Array.isArray(value)) {
it will format the params as [ 'fields', [] ].

In case of OAS2, we will go in here

if (Array.isArray(value)) {
where the params will become [ 'fields', '' ].

Because of this, the string that we get to add to the base URL in encodeFormOrQuery, the result of this line

return qs.stringify(encodedQuery, { encode: false, indices: false }) || '';
in case of OAS3 will be just an empty string, while for OAS2 it will be fields=.

Perhaps we could change this check here

if (collectionFormat === 'multi') {
to if (collectionFormat === 'multi' || value.length === 0) to get an empty array for OAS2 as well? Otherwise we can stay with if (value && !(Array.isArray(value) && value.length === 0)) in query parameter builder.

Copy link
Member

@char0n char0n left a comment

Choose a reason for hiding this comment

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

LGTM. Nicely done!

@char0n char0n merged commit 500dc17 into master Feb 12, 2024
5 checks passed
@char0n char0n deleted the fix-empty-field-for-multi-value-parameters branch February 12, 2024 13:51
swagger-bot pushed a commit that referenced this pull request Feb 12, 2024
## [3.25.1](v3.25.0...v3.25.1) (2024-02-12)

### Bug Fixes

* **execute:** detect empty value for multi-value parameters ([#3367](#3367)) ([500dc17](500dc17))
@swagger-bot
Copy link
Contributor

🎉 This PR is included in version 3.25.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants