Skip to content

[php-symfony] fix handling of endpoints with "text/plain" or "image/png" response type #21261

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

Merged
merged 3 commits into from
May 14, 2025

Conversation

gturri
Copy link
Contributor

@gturri gturri commented May 10, 2025

(This is the update of #21258 that I pushed too soon the other day. This time I took more time to thoroughly think it before opening this PR)

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in WSL)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

This fixes #21256 (and this subsequently fixes #13334, which is a subset of the other one).

This PR implements the 3 fixes that are required to be able to handle all the endpoints described in this OpenApi specification:

openapi: 3.0.3
info:
  title: repro issue openapi-generator
  version: 0.0.1
paths:
  /get-bool:
    get:
      summary: get a boolean
      responses:
        "200":
          description: OK
          content:
            text/plain:
              schema:
                type: boolean
  /get-image:
    get:
      summary: get an image
      responses:
        "200":
          description: OK
          content:
            image/png:
              schema:
                type: string
                format: binary
  /get-string:
    get:
      summary: get a string
      responses:
        "200":
          description: OK
          content:
            text/plain:
              schema:
                type: string
  /get-int:
    get:
      summary: get an int
      responses:
        "200":
          description: OK
          content:
            text/plain:
              schema:
                type: integer
  /get-number:
    get:
      summary: get a number
      responses:
        "200":
          description: OK
          content:
            text/plain:
              schema:
                type: number

Namely this:

  • ensures the generated method Controller.getOutputFormat returns a valid format when the user accepts */* (or when they don't specify any Accept header)
  • does not try to call ->serialize($data, ...)`` when the 2nd argument is null` (it would crash (error 500)). Instead it uses a straightforward way to handle the serialization in those simple cases
  • define relevant return types when the specification declares response with context text/plain

(nb: to make it easier to review I did a separate commit for each of those 3 fixes)

Ping technical committed, ie: @jebentier @dkarlovi @mandrean @jfastnacht @ybelenko @renepardon (and thanks for your work!)

gturri added 2 commits May 10, 2025 21:52
When a query has header "Accept" set to "*/*" it means it accepts
everything. It is hence weird to return a 406.
This patch ensures it does not occur: when the query accepts everything
then we take any produced type.

This fixes OpenAPITools#13334. This also partly makes the open PR OpenAPITools#15560 obsolete
(or at least, it provides a workaround)
$this->convertFormat may return "null". When it's the case we end up
calling

    ...->serialize($data, null);

but this crashes at runtime because that serialize method declares that
the 2nd parameter is of type "string" (so null is not accepted).

With this patch we avoid having an error 500. Instead we return something
that makes perfect sense when the OpenApi specification declares a content
of type "text/plain" and that the returned value is for instance a string,
an int, or a boolean.
This fixes the generated returned type of controller methods for
endpoint with a response declared like

    content:
      text/plain:
        schema:
          type: <boolean|string|integer|number>

or for

    content:
      image/png:
        schema:
          type: string
          format: binary

Without this commit the generated method *had to* return a value that
matched "array|object|null", which does not work in this case.
This commit makes it possible to return the proper type.
@wing328
Copy link
Member

wing328 commented May 14, 2025

thanks again for the pr

is it correct to say that you've been using these fixes/enhancements in your local environment/production for a while?

@gturri
Copy link
Contributor Author

gturri commented May 14, 2025

Yes, I'm using these fixes for my local development.
Not for a while and not in prod because I do not want to rely on a local fork that I would have to maintain. But yes in my local development I'm having issues with the latest version of Openapi Generator, and it works fine with those fixes.

@wing328 wing328 merged commit 4089438 into OpenAPITools:master May 14, 2025
27 checks passed
@wing328
Copy link
Member

wing328 commented May 14, 2025

ok. let's give it a try

@gturri
Copy link
Contributor Author

gturri commented May 14, 2025

thanks!

@gturri gturri deleted the dev2 branch May 15, 2025 20:06
gturri added a commit to gturri/openapi-generator that referenced this pull request May 15, 2025
PR OpenAPITools#21261 added support for endpoint with response of type text/plain
or even image/png.

This commit adds such endpoint so that:
- the way those are supported is clearer (as it is now directly visible
  in the generated sample files)
- if a future commit impacts this part of the generation it will be easier
  to assess that impact
wing328 pushed a commit that referenced this pull request May 16, 2025
PR #21261 added support for endpoint with response of type text/plain
or even image/png.

This commit adds such endpoint so that:
- the way those are supported is clearer (as it is now directly visible
  in the generated sample files)
- if a future commit impacts this part of the generation it will be easier
  to assess that impact
@wing328 wing328 added this to the 7.14.0 milestone Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants