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

Enable enforcing defaults #1616

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Enable enforcing defaults #1616

merged 1 commit into from
Jan 26, 2023

Conversation

RobbeSneyders
Copy link
Member

This PR is a proposal to enforce defaults in json bodies.

I'm not sure which way to go here

  • I think connexion should offer the functionality to fill in the defaults, as this helps enforce the contract. If a value is not provided, the application gets the default. We should probably offer this using a simple default flag instead of the custom validator now needed.
  • I don't like that validation adapts the body. Validation is a read-only operation and it would be nice to keep it that way.
  • Splitting enforcing defaults from validation would be less efficient since we'd need to traverse the body multiple times.

If we do want to go this way, we should probably implement a similar fix for form data, as we also use json validators there.

@RobbeSneyders RobbeSneyders added this to the Connexion 3.0 milestone Dec 28, 2022
@RobbeSneyders RobbeSneyders changed the base branch from main to feature/update-examples December 28, 2022 21:51
Base automatically changed from feature/update-examples to main December 30, 2022 19:34
@RobbeSneyders RobbeSneyders force-pushed the feature/enforcedefaults branch 2 times, most recently from a477bba to f4f4858 Compare January 18, 2023 22:34
@RobbeSneyders RobbeSneyders changed the base branch from main to feature/middleware-interface January 18, 2023 22:34
@RobbeSneyders
Copy link
Member Author

I updated the PR with a solution that I think is a good middle ground.

The standard JsonRequestBodyValidator does not adapt the body. Instead, I added a DefaultsJsonRequestBodyValidator which does. The user can easily activate enforcing default by passing this Validator in a validator_map.

I would have liked connexion to have an enforce_defaults flag, but it would be hard to make this compatible with the validator_map argument.

@@ -63,7 +63,8 @@ def fn(self) -> t.Callable:
async def __call__(
self, scope: Scope, receive: Receive, send: Send
) -> StarletteResponse:
return await self.fn(scope=scope, receive=receive, send=send)
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a fix to get the AsyncApp working correctly. Not related to enforcing defaults, but necessary to make the example work.

def __init__(self) -> None:
self.apis: t.Dict[str, AsyncApi] = {}
self.operations: t.Dict[str, AsyncOperation] = {}
self.router = Router()
super().__init__(self.router)

def add_api(self, *args, **kwargs):
api = AsyncApi(*args, **kwargs)
self.apis[api.base_path] = api
api = super().add_api(*args, **kwargs)
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a fix to get the AsyncApp working correctly. Not related to enforcing defaults, but necessary to make the example work.

while asyncio.iscoroutine(response):
response = await response
return response
return await decorated_function(request)
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a fix to get the AsyncApp working correctly. Not related to enforcing defaults, but necessary to make the example work.

api = self.api_cls(specification, next_app=self.app, **kwargs)
self.apis[api.base_path] = api
return api
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a fix to get the AsyncApp working correctly. Not related to enforcing defaults, but necessary to make the example work.

@@ -62,10 +62,10 @@ paths:
'201':
description: New pet created
requestBody:
x-body-name: pet
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated fix

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.

I agree that the proposed validator is a good middleground :)

Perhaps also good to put this in the updated documentation, as it might otherwise be hard to find

:return: A tuple (body, max_length) where max_length is the length of the larges message.
"""
more_body = True
max_length = 256000
Copy link
Member

Choose a reason for hiding this comment

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

Where does this number come from? I'm not aware of an ASGI limit

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not constrained by ASGI, but by the server. I ran some tests with Uvicorn and this was the upper limit, although I couldn't find where this limit is defined.

async def read_body(self) -> t.Tuple[str, int]:
"""Read the body from the receive channel.

:return: A tuple (body, max_length) where max_length is the length of the larges message.
Copy link
Member

Choose a reason for hiding this comment

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

typo: largest

Base automatically changed from feature/middleware-interface to main January 26, 2023 13:40
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4015715852

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 4015691791: 0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4015715852

  • 19 of 57 (33.33%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.13%

Changes Missing Coverage Covered Lines Changed/Added Lines %
connexion/decorators/main.py 0 1 0.0%
connexion/apps/asynchronous.py 1 4 25.0%
connexion/validators/json.py 15 49 30.61%
Totals Coverage Status
Change from base Build 4015691791: 0%
Covered Lines: 3114
Relevant Lines: 3455

💛 - Coveralls

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.

None yet

3 participants