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

Split parsing from validation completely #1934

Merged
merged 1 commit into from
May 28, 2024

Conversation

RobbeSneyders
Copy link
Member

During the refactoring for Connexion 3, we deduplicated all coerce_type calls during the refactoring and moved them into the uri_parser so it is done in a single place. When looking into #1931 however, I noticed we are still using the uri_parser from more places than we should.

This PR centralizes the parsing further into the ConnexionRequest class. During validation, we now instantiate a ConnexionRequest instead of a Starlette Request, and leverage it instead of calling the uri_parser directly.

I split the parsing and validation in the tests so they can be tested in isolation.

@coveralls
Copy link

Coverage Status

coverage: 94.148% (-0.03%) from 94.179%
when pulling 576e67b on feature/split-parsing-validation
into 810a980 on main.

Copy link
Member

@Ruwann Ruwann left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@RobbeSneyders RobbeSneyders merged commit 823fa6c into main May 28, 2024
7 of 8 checks passed
@RobbeSneyders RobbeSneyders deleted the feature/split-parsing-validation branch May 28, 2024 08:53
@CordulaGuder
Copy link

Hi @RobbeSneyders, thanks a lot for looking into my PR and providing this change, which seems to make a lot of change! Trying it out, I found that it only applies to path and query params, and not to header params. Parameter validator gets request.headers, which, on further investigation, is not a property of ConnexionRequest, but only exposes the headers property of the underlying Starlette request object. On this, no type conversion is being done, so I'm now seeing the exact same issue as before.

Trying to fix this by adding a header_params property to ConnexionRequest, I stumbled over more issues. For some reason, I don't know whether that is a bug in Connexion or a misconfiguration on my side, the URIParser used in the ConnexionRequest doesn't have the param_defn from the API spec, so it skips the type conversion. I decided to stop the migration effort here for a while as it's been taking too much time already. This is just to inform you about the things I've noticed.

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.

4 participants