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

Avoids overwriting Plug.Conn body_params and params with the cast outcome #425

Merged
merged 1 commit into from
May 15, 2022

Conversation

albertored
Copy link
Contributor

See #398 and #376

As aked in this comment the flag for moving cast result to conn.private is now an option to CastAndValidate plug.

Credits to @riccardomanfrin and @lucacorti

@albertored
Copy link
Contributor Author

@mbuhot did you have the chance to review this? Is there something missing to have it merged? (sorry for bothering you)

@mbuhot
Copy link
Collaborator

mbuhot commented Mar 17, 2022

Thanks for the ping @albertored. This PR is very close.
A couple of suggestions:

  1. conn_private_decode won't be immediately understood by newcomers to the library. I'd prefer something like replace_params: true
  2. I think we should always store the casted params in conn.private, and then the replace_params option only determines whether conn.params and conn.body_params are also replaced.
  3. Provide OpenApiSpex.body_params(conn) and OpenApiSpex.params(conn) to retrieve the params from conn.private

@albertored
Copy link
Contributor Author

@mbuhot all your suggestions are now implemented

Copy link
Collaborator

@mbuhot mbuhot left a comment

Choose a reason for hiding this comment

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

@albertored just one final suggestion to keep the values under conn.private.open_api_spex (like we do for PutApiSpec) to prevent potential clashes with other plugs that OpenApiSpex users have in their apps.

lib/open_api_spex.ex Outdated Show resolved Hide resolved
lib/open_api_spex.ex Outdated Show resolved Hide resolved
lib/open_api_spex/cast_parameters.ex Outdated Show resolved Hide resolved
lib/open_api_spex/operation2.ex Outdated Show resolved Hide resolved
test/operation2_test.exs Outdated Show resolved Hide resolved
test/operation2_test.exs Outdated Show resolved Hide resolved
test/operation2_test.exs Outdated Show resolved Hide resolved
test/plug/cast_test.exs Outdated Show resolved Hide resolved
test/plug/cast_test.exs Outdated Show resolved Hide resolved
test/plug/cast_test.exs Outdated Show resolved Hide resolved
test/plug/cast_test.exs Outdated Show resolved Hide resolved
test/plug/cast_test.exs Outdated Show resolved Hide resolved
test/plug/cast_test.exs Outdated Show resolved Hide resolved
test/support/user_no_replace_controller.ex Outdated Show resolved Hide resolved
@albertored
Copy link
Contributor Author

@mbuhot it should be ready now, thank you

@mbuhot mbuhot merged commit 6503474 into open-api-spex:master May 15, 2022
@albertored albertored deleted the cast-to-private branch June 6, 2022 09:02
aerosol added a commit to plausible/analytics that referenced this pull request Sep 28, 2023
aerosol added a commit to plausible/analytics that referenced this pull request Sep 28, 2023
aerosol added a commit to plausible/analytics that referenced this pull request Oct 2, 2023
aerosol added a commit to plausible/analytics that referenced this pull request Oct 2, 2023
* Update depenedencies: OpenAPISpex + cursor based pagination

* Update formatter config

* Add internal server error implementation

* Test errors

* Implement pagination interface

* Implement Plugins API module macros

* Implement Public API base URI

(to be used with path helpers once called from within
forwarded router's scope)

* Implement OpenAPI specs + schemas

* Implement Shared Links context module

* Add pagination and error views

* Add Shared Link view

* Implement Shared Link controller

* Expose SharedLink.t() spec

* Implement separate router for the Plugins API

* Update moduledocs

* Always wrap resource objects with `data`

* Update moduledoc

* Use open-api-spex/open_api_spex#425

due to open-api-spex/open_api_spex#92

* Rely on BASE_URL for swagger-ui server definition

* Fixup goals migration

* Migrate broken goals before deleting dupes

* Remove bypassing test rate limiting for which there's none anyway

* Move the context module under `Plausible.` namespace

* Bring back conn assignment to PluginsAPICase template

* Update test/plausible_web/plugins/api/controllers/shared_links_test.exs

Co-authored-by: Uku Taht <Uku.taht@gmail.com>

* Update renamed aliases

* Seed static token for development purposes

* Delegate Plugins API 500s to a familiar shape

* Simplify with statement

---------

Co-authored-by: Uku Taht <Uku.taht@gmail.com>
github-actions bot pushed a commit to kphrx/pleroma that referenced this pull request Jan 30, 2024
…ided by the :replace_params config option

This allows us to configure Open API Spex to not overwrite the params with the casted versions which violates the Plug.Conn.t() contract

open-api-spex/open_api_spex#92
open-api-spex/open_api_spex#425
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.

2 participants