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

Add tests and fix parameter exploding issue #576

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hamir-suspect
Copy link
Contributor

@hamir-suspect hamir-suspect commented Nov 23, 2023

This fixes the parameters exploding issue. Now parameters defined as:

parameter(
            :some,
            :query,
            %Schema{
              type: :object,
              oneOf: [
                %Schema{
                  type: :object,
                  properties: %{foo: %Schema{type: :string}, bar: %Schema{type: :string}},
                  required: [:foo]
                },
                %Schema{
                  type: :object,
                  properties: %{foo: %Schema{type: :string}, baz: %Schema{type: :string}},
                  required: [:baz]
                }
              ]
            },
            "Some query parameter ",
            explode: true,
            style: :form,
            required: true
          )

will be properly casted into the :some parameter.
How this works:
the parameters that we fetch for a location can be flat (?foo=x&bar=y) before casting we search for any parameters that should be exploded, if we find any we gather all the properties it can hold.
We add those to the struct (as if it was a deep struct param ?some[foo]=x&some[bar]=y and remove them from the query params. in the end query params struct before casting looks like %{"some" => %{"foo" => "x", "bar" => "y"}} instead of what we started with: %{"foo" => "x", "bar" => "y"}.

The ideal way would be to fix how the location schema is composed but that seems to be quite tricky with current implementation.

The PR has some leftover code from experimentation, if you think this is a good solution I will clean it up.

@hamir-suspect
Copy link
Contributor Author

This PR resolves #573 , I would welcome any suggestions for improvements.

@mbuhot
Copy link
Collaborator

mbuhot commented Dec 27, 2023

Thanks for the PR @hamir-suspect, I'm a bit concerned about the complexity required to identify all potential parameters that may apply to an exploded form style query parameter object.

Could we simplify the approach by taking all query params, then remove those with names matching non-exploded Parameters? All remaining query params are potentially consumed by exploded Parameters and will be passed along to Cast, leaving it up to the existing polymorphic schema handling to determine which ones are included in the result.

@hamir-suspect
Copy link
Contributor Author

hamir-suspect commented Jan 3, 2024

@mbuhot thanks for the feedback. I've cleaned up the PR a little bit and removed params with names matching to non-exploded schema parameters.

@hamir-suspect hamir-suspect force-pushed the has/fix-parameter-exploding-issue branch 2 times, most recently from a85bef6 to 8683985 Compare January 3, 2024 15:55
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