apideck-libraries / portman

Port OpenAPI Specs to Postman Collections, inject test suite and run via Newman 👨🏽‍🚀
http://getportman.com/
Apache License 2.0
632 stars 60 forks source link

Bad request due to invalid request body generation #601

Closed FreyAtGit closed 1 month ago

FreyAtGit commented 2 months ago

Hi,

we have just started with Portman, but we are facing an issue that looks like a bug but might also be a misconfiguration on our side.

image

image

image

thim81 commented 2 months ago

hi @FreyAtGit

To verify, you have no special Portman configuration or conversion settings defined?

FreyAtGit commented 2 months ago

@thim81 I have one to get the bearer token first and trying to setup some data on the "problem" endpoint. But no postman-config file. image image

thim81 commented 2 months ago

@FreyAtGit I'm able to reproduce the issue (see screenshot).

image

First indication points towards the "x-www-form-urlencoded" conversion, but I will have to debug it more in detail to find the cause.

Postman only converts to 1 content-type during the conversion, Question: do you expect "x-www-form-urlencoded" in the Postman collection or "application/json"?

image

When we import it manually in Postman, it behaves similar by taking the "x-www-form-urlencoded"

image image
thim81 commented 2 months ago

FYI: If commenting out "x-www-form-urlencoded" the conversion behaves as more as expected:

image image
FreyAtGit commented 2 months ago

ah interesting, thanks. I will try it :) So, I would say "application/json" is the main one and I am unsure whether the url-encoded is really needed, but it is a released API that we cannot easily deprecate. But, would it help to just re-order the different types, so that "application/json" is at the end?

thim81 commented 2 months ago

hi @FreyAtGit

Let me check, if re-ordering would help or if we can improve the logic to start with "application/json". Another option would be to use any of the build-in filtering capabilities before conversion, to filter out the "requestBody.content".

Give me a bit of time to do some experimenting, I'll report back with my findings.

Question: The OpenAPI spec from Siemens is owned by you or your team or you are a consumer of it? And the goal is to use it for testing yourself or for distribution to integrators or internal teams?

FreyAtGit commented 2 months ago

We own it and the goal is to use Portman for testing purposes, starting with contract-testing.

thim81 commented 2 months ago

@FreyAtGit Short update:

We use the official openapi-to-postman package from Postman to convert openapi-to-postman. It is in that package, where the logic is fixed to take the 'x-www-form-urlencoded', before the 'raw' content-type.

We created an issue and PR + PR, with 2 options to have more control on the selection of the request body during the conversion.

Let's see what the Postman team responds.

thim81 commented 2 months ago

While we wait for feedback, you could try to filter out the application/x-www-form-urlencoded,

image

but keep in mind that this will also remove the form from the "token" endpoint. If you don't need it for your contract testing, you could already progress.

image

Have a look at "Diff" of your OpenAPI and the filtered OpenAPI result, in the playground

thim81 commented 2 months ago

@FreyAtGit Another update: The Postman team already responded and indicated their preference to pick the content-type based on the order in the content-type list.

We will continue with that direction and hope that the PR will land soon in the openapi-to-postman package.

FreyAtGit commented 2 months ago

Thanks for your help :)

thim81 commented 2 months ago

@FreyAtGit Side-question to get some input from our community: what would you expect to be the default behavior, in case there are multiple content-types in the request body:

Option A: Take the first content-type from the request body as ordered in the OpenAPI spec, meaning which ever is found first will be taken. Option B: Take the application/json always (if it is present)

FreyAtGit commented 2 months ago

I would go with Option A as it gives you the option to choose the one you want to use or even better, make a setting to define which one to choose (maybe a global setting would be enough)

FreyAtGit commented 2 months ago

Ah no, you mean if there is a setting, then what would be the expected default behavior? Then, hmm..., both options seem a valid decision. But, you have an always clearly defined default with Option A (compared to Option B).

thim81 commented 2 months ago

Thanks for your input. I had the same preference for Option A and will make that the default for Portman, which can always be overwritten via a config setting.

thim81 commented 2 months ago

hi @FreyAtGit

Another update the PR got accepted and merged by the Postman team. I'll wait a few days, to see if a new version of openapi-to-postman with the changes, will be released.

If it takes more time, we will include a patch in Portman.

FreyAtGit commented 2 months ago

Sounds great, thank you very much :)

thim81 commented 2 months ago

hi @FreyAtGit

FYI Portman v1.28.0 is just released, which will use the first-listed request body.

The PR openapi-to-postman was accepted and was released with v4.23.x of openapi-to-postman.

FreyAtGit commented 2 months ago

Thanks a lot for the fast fix 👍😀

FreyAtGit commented 2 months ago

@thim81 I have just updated Portman and openapi2postman and tried it, but I still got the same (error) result :( The first content is "application/json" and it still generates the "application/x-www-form-urlencoded". Maybe, I am doing something wrong or missed something ...

thim81 commented 2 months ago

@FreyAtGit Is "application/json" listed 1st in the request body content types of your OpenAPI spec?

An alternative option is to pass along a postman config --postmanConfigFile, with "preferredRequestBodyType": "raw"

Example

{
  "folderStrategy": "Tags",
  "requestParametersResolution": "Example",
  "exampleParametersResolution": "Example",
  "keepImplicitHeaders": true,
  "enableOptionalParameters": false,
  "preferredRequestBodyType": "raw"
}
FreyAtGit commented 2 months ago

Ah, this is not per endpoint, this is per whole spec? But, I also added the "application/json" as very first now, but also seems to have no effect .

Edit: also the setting is not working for me

thim81 commented 2 months ago

@FreyAtGit Let me run some manual validation, since we have tests in place to verify the results.

thim81 commented 2 months ago

hi @FreyAtGit

I'm able to reproduce it. When I'm using a minimal version of your OpenAPI spec, it converts as expected:

image

When using the full spec:

image

Let me further investigate.

thim81 commented 2 months ago

@FreyAtGit

Another short update: Portman is using convert (V1), while the openapi-to-postman introduced convertV2 (a while ago) made it the default for the CLI, but not for the direct usage as a openapi-to-postman module. We assumed that under the hood V2 had become the default, which had the recent improvement we did.

Long story short: we found the reason, switching to convertV2 would solve the cause BUT it looks that convertV2 has a a strange behaviour with ENUM values, which we want to sort out to prevent regression.

thim81 commented 2 months ago

@FreyAtGit

Update: The PR introduces convertV2 from openapi-to-postman, which is needed for respecting the content-type order for request body. In the PR I included the results after using Portman.

thim81 commented 1 month ago

hi @FreyAtGit

We implemented convertV2 and it is included in the Portman v1.29.0. Closing this issue, but let me know if it does not work as expected.

FreyAtGit commented 1 month ago

@thim81 It is working now 😀. Thank you very much!

thim81 commented 1 month ago

@FreyAtGit Thank you for the confirmation and your patience. Wishing you success as you build your use-cases with Portman!