Skip to content

Bad request due to invalid request body generation #601

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

Closed
FreyAtGit opened this issue Jul 8, 2024 · 29 comments
Closed

Bad request due to invalid request body generation #601

FreyAtGit opened this issue Jul 8, 2024 · 29 comments
Assignees
Labels
investigate Needs more investigation, before next steps can be defined.

Comments

@FreyAtGit
Copy link

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

  • Schema of the request body:

image

  • Generated request body from Portman:

image

  • Issue: The RoleAssignmentsModel is not there.
@thim81
Copy link
Collaborator

thim81 commented Jul 8, 2024

hi @FreyAtGit

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

@FreyAtGit
Copy link
Author

@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 thim81 added the investigate Needs more investigation, before next steps can be defined. label Jul 9, 2024
@thim81 thim81 self-assigned this Jul 9, 2024
@thim81
Copy link
Collaborator

thim81 commented Jul 9, 2024

@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
Copy link
Collaborator

thim81 commented Jul 9, 2024

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

image image

@FreyAtGit
Copy link
Author

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
Copy link
Collaborator

thim81 commented Jul 10, 2024

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
Copy link
Author

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

@thim81
Copy link
Collaborator

thim81 commented Jul 10, 2024

@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
Copy link
Collaborator

thim81 commented Jul 10, 2024

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
Copy link
Collaborator

thim81 commented Jul 11, 2024

@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
Copy link
Author

Thanks for your help :)

@thim81
Copy link
Collaborator

thim81 commented Jul 12, 2024

@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
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

thim81 commented Jul 12, 2024

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
Copy link
Collaborator

thim81 commented Jul 16, 2024

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
Copy link
Author

Sounds great, thank you very much :)

@thim81
Copy link
Collaborator

thim81 commented Jul 22, 2024

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.

@thim81 thim81 closed this as completed Jul 22, 2024
@FreyAtGit
Copy link
Author

Thanks a lot for the fast fix 👍😀

@FreyAtGit
Copy link
Author

FreyAtGit commented Jul 23, 2024

@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
Copy link
Collaborator

thim81 commented Jul 23, 2024

@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"
}

@thim81 thim81 reopened this Jul 23, 2024
@FreyAtGit
Copy link
Author

FreyAtGit commented Jul 23, 2024

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
Copy link
Collaborator

thim81 commented Jul 23, 2024

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

@thim81
Copy link
Collaborator

thim81 commented Jul 24, 2024

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
Copy link
Collaborator

thim81 commented Jul 25, 2024

@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
Copy link
Collaborator

thim81 commented Jul 26, 2024

@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
Copy link
Collaborator

thim81 commented Aug 6, 2024

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.

@thim81 thim81 closed this as completed Aug 6, 2024
@FreyAtGit
Copy link
Author

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

@thim81
Copy link
Collaborator

thim81 commented Aug 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Needs more investigation, before next steps can be defined.
Projects
None yet
Development

No branches or pull requests

2 participants