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

Omit empty and optional values in form data #5303

Closed
propi opened this issue Apr 15, 2019 · 16 comments · Fixed by #5830
Closed

Omit empty and optional values in form data #5303

propi opened this issue Apr 15, 2019 · 16 comments · Fixed by #5830

Comments

@propi
Copy link

propi commented Apr 15, 2019

I have this request body:

...
requestBody:
        required: true
        content:
          application/x-www-form-urlencoded:
            schema:
              type: object
              properties:
                name:
                  type: string
                nick:
                  type: string
...

If I send a request wihin Swagger UI with some name but with empty value in the nick field, the UI evaluates the request like this:

curl -X POST "address" -H "accept: application/json" -H "Content-Type: application/x-www-form-urlencoded" -d "name=vaclav&nick="

But I need not to have the nick field contained in the request body if the field is empty, like this:

curl -X POST "address" -H "accept: application/json" -H "Content-Type: application/x-www-form-urlencoded" -d "name=vaclav"

Can I mark a property field as optional and not sending if the field is empty? Unfortunately, my API does not accept empty values, but it is able to omit some post field if it is not included in a request.

@propi propi changed the title Sending of empty and optional values in form data Omit empty and optional values in form data Apr 15, 2019
@leggsimon
Copy link
Contributor

This doesn’t really solve the problem completely but, in case you didn’t know, you can provide another required property under schema which is a list of required properties e.g.

      requestBody:
        required: true
        content:
          application/x-www-form-urlencoded:
            schema:
              type: object
              properties:
                name:
                  type: string
                nick:
                  type: string
+              required:
+                - name

Which will render *required next to the required “name” field
localhost_3230__url=_documents_bugs_5303 yaml

@leggsimon
Copy link
Contributor

Having said that, how would we expect this to work?

Consider this html form

<form action="https://httpstat.us/200" method="get">
	<input type="text" name="name" />
	<input type="text" name="nick" />
	<input type="submit" value="Submit" />
</form>

If you submitted that, any values you hadn’t entered still get added as query parameters so it still ends up making a request to https://httpstat.us/200?name=blah&nick= including the empty nick parameter.

So while I think there is the potential to have an “Ignore empty values” option I don’t think swagger-ui should assume that because it’s an optional parameter and it’s empty that it shouldn’t be sent at all.

Does that make sense?

cc/ @milanlatinovic

@archivelm
Copy link

Hi Simon,

These are my thoughts related to current issue (maybe it will be helpful).

From the business process view:

  • Swagger UI should have some kind of configuration (for OpenApi 3.0) where we can configure it to either "ignore empty values" or "keep empty values". Without this option, Swagger UI will not be useful for lots of APIs.
  • Swagger UI (Swagger 2.0.0) already ignores empty values (empty parameters) when they are not inserted. Example here https://api.timetac.com
  • It is not important which behavior is default (ignore or keep) as long as there is an option to choose and configure it.

Ideal case:

  • Postman handles this by having a checkboxes where you can have all the parameters defined, but use checkbox to simply choose which of them will actually be included in request

From a technical side:

  • I did some research and found out that there were already tickets related to the opposite issue (people actually wanted to keep parameters). There was a discussion why allowEmptyValues is not supported, etc, ticket allowEmptyValue not implemented? #1934
  • Then someone made some kind of fix and merged it in master (where checkbox look implemented), pull request allowEmptyValue controls #4788
  • Then there was a request to update documentation, but in Swagger UI documentation it is stated that "allowEmptyValues field should not be used because it will be removed in future".

Maybe I missed something here or I understood it wrong.
To me, it seems like there was already an implementation of "Send empty values" feature, but something might be wrong here. At this moment, there are no checkboxes and empty values are always sent (which was not a case when we used Swagger 2.0.0 format).

What would be the best thing to do here?
Swagger UI is an beautiful library with fantastic preview, but this is a serious blocker. :)

I am open to help in any kind of testing from our side to sort this out.

Kind regards,
Milan

@propi
Copy link
Author

propi commented Apr 20, 2019

I try to describe the API problem in depth. For my case, the API has POST operation /user-update which is able to update user attributes like name or nick. But when I call this operation I should have choice whether to update both attributes or only one of them. Therefore I send only name parameter without nick parameter if I want to update only the name attribute, like this:

curl -X POST "address" -H "accept: application/json" -H "Content-Type: application/x-www-form-urlencoded" -d "name=vaclav"

In the case when I want to choose both attributes, I send this request:

curl -X POST "address" -H "accept: application/json" -H "Content-Type: application/x-www-form-urlencoded" -d "name=vaclav&nick="

It means that the name attribute should be changed to value vaclav and nick attribute should be changed to empty string.

Unfortunately, I can not simulate this behavior within Swagger-UI. It is not possible to omit any defined optional parameter in a HTTP request and if the value is empty my API accepts it as empty string - it is a valid value, but I also need to have a choice to omit this parameter.

@archivelm
Copy link

Hi everyone,
@leggsimon @propi

Just wanted to quickly check the state of this issue? Was there any idea on how and when this would be resolved? Did this make it to some refined backlog or maybe even sprint plan? :)

Quick note, this issue has also appeared in #5314

Let me know if I can provide any further info to make resolving of this issue easier.

Kind regards,
Milan

@propi
Copy link
Author

propi commented May 6, 2019

I would vote for adding a checkbox to all optional parameters within the Swagger UI (it could be checked by default). But, I need to have the option to uncheck this checbox and tell Swagger UI, that I don't want to send this parameter in the HTTP request...

@archivelm
Copy link

@propi agreed, checkbox to turn on / turn off empty parameters (like Postman interface does) would be a quick and good solution.

But in any case, I think this should be handled as soon as possible, because at this point (at least for my company) Swagger UI is not useful, because our API execution does not work.

Meanwhile: @leggsimon if you know some kind of simple/dirty patch to completely turn off empty parameters, I would be very thankful. This would solve our issue and help us continue using Swagger UI.

All best,
Milan

@leggsimon
Copy link
Contributor

leggsimon commented May 11, 2019

Sorry @milanlatinovic I’ve been on holiday which was why I haven’t got back to this.

Was there any idea on how and when this would be resolved? Did this make it to some refined backlog or maybe even sprint plan?

I don’t know personally. I don’t make any particular decisions about the project at the moment or have insight as to where it’s going. Having said that I also don’t know of any reasons why a PR would be rejected to add this feature (that’s not to say others might not have opposing views).

if you know some kind of simple/dirty patch to completely turn off empty parameters

Unfortunately I don’t.


Regarding a potential solution, I think that @propi’s suggestion of having checkboxes for each parameter makes the most sense. I’m wondering though whether there should be the same functionality for toggling empty headers & query parameters as well as request bodies. Would either of you have thoughts there? I’m inclined to say the same should apply.

I did start an attempt at this before and it is a little bit fiddly so might take a bit of time to get a good solution, but I may give it another go this weekend.

One last thing, regarding #5314, I wonder if that issue should be closed in favour of keeping the discussion here?

@archivelm
Copy link

Hi @leggsimon, not a problem at all. I hope your holiday was good. :)

Regarding a potential solution, I think that @propi’s suggestion of having checkboxes for each parameter makes the most sense. I’m wondering though whether there should be the same functionality for toggling empty headers & query parameters as well as request bodies. Would either of you have thoughts there? I’m inclined to say the same should apply.

Yup, same should apply in my opinion as well.

One last thing, regarding #5314, I wonder if that issue should be closed in favour of keeping the discussion here?

Yup, we can close this one and focus on finding and delivering a solution within this thread.

Thanks, I will be free to check in on a status of this issue from time to time. Meanwhile, let me know if there is anything that I can do or help from my side.

@leggsimon
Copy link
Contributor

So I had a little look at this over the weekend and although I’m not completely familiar with the project it seems like quite a complicated implementation. I’m not sure if I’m going to have much time soon to get to a real fix (although I’ve written some failing e2e tests to cover the implementation which I’ll try and push up). I’m just going to write what I’ve found so far in case some else has a bit more time.

Parameters allow for an allowEmptyValue property which is in the openapi spec, when this value is true swagger-ui renders a checkbox (ParameterIncludeEmpty component). But this is a bit of a red herring.

Since Request Bodies don’t support this property in the spec that component is never used in the flow of rendering the Request Body “form”. The Request Body is rendered initially here and the actual form fields are described in json-schema-components.jsx (eg rendering the text input is the JsonSchema_string component)
As far as I can tell the onChange prop that is passed to the RequestBody component listens for changes to the rendered fields and updates a Redux store via the setRequestBodyValue action.

In order to omit any empty values via a checkbox I think one of 2 things needs to happen.

  1. Add another Redux action which can remove a value from the store when we say “don’t send this empty value”. That way when execute is triggered it continues to read from the store and send the body as it is described in the store.
  2. Change the entire way the Request is made. I almost think that, rather than onChange of each form field updating a store and making a request by grabbing what’s in the store, the whole thing should really be a form and when Submitted (Executed) we then go and grab all the values in the form skipping any that are empty and we have indicated not to send it.

My preferred approach would be 2 personally since I think it makes it a little more semantically correct, however I think it is the most amount of work and would possibly be a breaking change depending on how this project deems changes to be breaking.

@archivelm
Copy link

True, I agree, 2nd approach would be more semantically correct.

allowEmptyValue and current checkboxes implementation is not useful because it is not covering requestBody properly.

@leggsimon thank you once more for looking into this and documenting your findings.

@liv1n9
Copy link

liv1n9 commented Mar 16, 2020

Has this issue been handling?

@archivelm
Copy link

liv1n9 nope, not that I know... whole year, not a step forward :)

@abcang
Copy link
Contributor

abcang commented Mar 17, 2020

I also wanted this feature, so I implemented it and sent a Pull Request.
#5830

@hkosova
Copy link
Contributor

hkosova commented Jul 7, 2020

Reopening because PR #5830 was reverted.

@hkosova hkosova reopened this Jul 7, 2020
@hkosova
Copy link
Contributor

hkosova commented Jul 17, 2020

Added back in #6227.

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 a pull request may close this issue.

6 participants