Skip to content

Conversation

mfmarche
Copy link
Contributor

The actual arg (scope), needs to be updated when the content-length changes.

Fixes #1954

Changes proposed in this pull request:

@RobbeSneyders
Copy link
Member

Thanks @mfmarche!

You are on to something. It indeed seems like the updated header does not have an effect. However, the scope does need to be copied whenever it is modified in middleware, so the changes in the scope are only propagated downstream.

I think the best solution is to update this method to return the modified scope as well.

Also, it would be great if you could add a test for this behavior.

@mfmarche
Copy link
Contributor Author

Thanks @RobbeSneyders . I took your comments and added appropriate test, and now return the content length. The test prior to the scope change gave a 500, which was similiar to what I saw.

@RobbeSneyders
Copy link
Member

Thanks @mfmarche, the change looks good to me. The pipeline is failing on pre-commit though. You can fix this by running pre-commit run --all-files and committing the results.

To have it run automatically in the future, you can run pre-commit install.

The actual arg (scope), needs to be updated when the content-length
changes.

Added test to validate that body changes on setting a default.
Signed-off-by: Mike Marchetti <[email protected]>
@coveralls
Copy link

Coverage Status

coverage: 94.408% (+0.3%) from 94.15%
when pulling ae44b00 on mfmarche:fix_scope_update
into 7c4fcc4 on spec-first:main.

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @mfmarche!

@RobbeSneyders RobbeSneyders merged commit 12fd72c into spec-first:main Nov 4, 2024
8 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.

using validator DefaultsJSONRequestBodyValidator with more content-length breaks json data
3 participants