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

Adding collection schema to methods that use openApiPaginator #563

Merged
merged 6 commits into from
Feb 7, 2025

Conversation

MasaKni
Copy link
Contributor

@MasaKni MasaKni commented Dec 18, 2024

Resolves #562

I modified the code so that actions that have the openApiPaginator will get the collection response, and i also added the schema to the action because it was not being set at all.

moved:
I also added the possibility to sort/order by an array which requires an array object in the swagger

@MasaKni MasaKni mentioned this pull request Dec 18, 2024
@cnizzardini
Copy link
Owner

Hey there thanks for submitting a pull request. Do you mind adding a description? I''ll review this when I have time over the holidays. Thanks @MasaKni!

@MasaKni MasaKni changed the title adding collection schema to methods that use openApiPaginator Adding collection schema to methods that use openApiPaginator Dec 23, 2024
@MasaKni
Copy link
Contributor Author

MasaKni commented Jan 7, 2025

@cnizzardini another thing.
in OperationResponse.php file assignFromAttributes function why do you add an empty content if all the response checks are not met? doesn't this mean that the request does not have a body and we don't want to add content so why adding it?

@@ -14,6 +14,21 @@ x-swagger-bake:
required: false
schema:
type: integer
paginatorOrder:
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this needed? How come paginatorSort will not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

paginatorSort only works for one field sorting, if we want to sort by let's say name and date we can't do that with it we would use the order.

Copy link
Owner

@cnizzardini cnizzardini Jan 12, 2025

Choose a reason for hiding this comment

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

I've pulled your branch in using the demo: https://github.com/cnizzardini/cakephp-swagger-bake-demo

How come I get output that looks like this on /actors/{id}/films:

image

The only change I made here (https://github.com/cnizzardini/cakephp-swagger-bake-demo/blob/master/src/Controller/ActorsController.php#L155) was adding in #[OpenApiPaginator]

Copy link
Owner

Choose a reason for hiding this comment

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

@MasaKni bump ^

Copy link
Contributor Author

@MasaKni MasaKni Jan 14, 2025

Choose a reason for hiding this comment

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

yes well we can agree on another structure for order, but i saw that adding it in this format will be helpful,
we can add ordering as object and each we add we specify the field and dir
if you try it then it will add objects of the
{ "field": "name", "dir": "asc" }

and we would wanna keep the sort because we wanna allow sorting by one field not a combination

Copy link
Owner

Choose a reason for hiding this comment

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

If we can't resolve this in the current PR can we remove this for now and come back to it? I don't want to release something that doesn't work with SwaggerUI @MasaKni

Copy link
Owner

Choose a reason for hiding this comment

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

@MasaKni is this resolved?

Copy link
Owner

Choose a reason for hiding this comment

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

@MasaKni I am going to close this PR if you don't respond this.

@cnizzardini
Copy link
Owner

cnizzardini commented Jan 9, 2025

@cnizzardini another thing. in OperationResponse.php file assignFromAttributes function why do you add an empty content if all the response checks are not met? doesn't this mean that the request does not have a body and we don't want to add content so why adding it?

Honestly, I am not sure. I am not as active in this library as I once was. Is it causing an issue or just seems off?

@MasaKni
Copy link
Contributor Author

MasaKni commented Jan 10, 2025

@cnizzardini another thing. in OperationResponse.php file assignFromAttributes function why do you add an empty content if all the response checks are not met? doesn't this mean that the request does not have a body and we don't want to add content so why adding it?

Honestly, I am not sure. I am not as active in this library as I once was. Is it causing an issue or just seems off?

yes it is adding the default content string to responses that i define as with no body

@cnizzardini
Copy link
Owner

@cnizzardini another thing. in OperationResponse.php file assignFromAttributes function why do you add an empty content if all the response checks are not met? doesn't this mean that the request does not have a body and we don't want to add content so why adding it?

Honestly, I am not sure. I am not as active in this library as I once was. Is it causing an issue or just seems off?

yes it is adding the default content string to responses that i define as with no body

Can you post an issue for this with steps to reproduce and I can take a look at it?

@cnizzardini
Copy link
Owner

cnizzardini commented Jan 12, 2025

I'm a bit confused by this PR. I am looking at a private codebase I maintain (running cakephp 5.1 and swaggerbake 3.0.5 on php 8.3) and #[OpenApiPaginator] already works as expected on non index() controller actions.

Given the following setup:

    #[OpenApiPaginator]
    #[OpenApiSecurity(name: 'bearerAuth')]
    public function bar()

I get this output:

  /api/foo/bar:
    get:
      operationId: foo:bar:get
      tags:
        - Foo
      parameters:
        - $ref: '#/x-swagger-bake/components/parameters/paginatorPage'
        - $ref: '#/x-swagger-bake/components/parameters/paginatorLimit'
        - $ref: '#/x-swagger-bake/components/parameters/paginatorSort'
        - $ref: '#/x-swagger-bake/components/parameters/paginatorDirection'

@cnizzardini
Copy link
Owner

cnizzardini commented Jan 12, 2025

@MasaKni Given what I noted above, I am going to need steps from you to reproduce a state with the problem you are attempting to solve for so I can test this and better understand what is being solved before I merge this PR in.

My apologies, I see now what you a specifically solving for is the database schema portion not showing as the OpenAPI example schema, not the query parameters.

@MasaKni
Copy link
Contributor Author

MasaKni commented Jan 13, 2025

@MasaKni Given what I noted above, I am going to need steps from you to reproduce a state with the problem you are attempting to solve for so I can test this and better understand what is being solved before I merge this PR in.

My apologies, I see now what you a specifically solving for is the database schema portion not showing as the OpenAPI example schema, not the query parameters.

yes indeed, the parameters are added correctly but the schema is set for the default response which is
"string"

@cnizzardini
Copy link
Owner

It is courtesy to note when you have resolved comments.

@cnizzardini cnizzardini merged commit d8c2792 into cnizzardini:master Feb 7, 2025
4 checks passed
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 this pull request may close these issues.

OpenApiPaginator
2 participants