Skip to content

[Issue24] Prevent Schema Mutation #26

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 28, 2021
Merged

[Issue24] Prevent Schema Mutation #26

merged 5 commits into from
Dec 28, 2021

Conversation

Larox
Copy link
Contributor

@Larox Larox commented Dec 3, 2021

This PR includes the changes needed to solve issue #24 .

Includes some fixes for tests and unit test for testing the fix included.

@Larox
Copy link
Contributor Author

Larox commented Dec 3, 2021

Hi @p1c2u, I hope you are doing well. Please, let me know your code review comments.

cc/ @sebastianmika

@Larox
Copy link
Contributor Author

Larox commented Dec 17, 2021

Hey @p1c2u, I'm following up on this PR code review. Please, let me know your thoughts.

cc/ @sebastianmika

@codecov
Copy link

codecov bot commented Dec 28, 2021 •
edited
Loading

Codecov Report

Merging #26 (7ed2ea8) into master (ab7222a) will increase coverage by 11.30%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #26       +/-   ##
===========================================
+ Coverage   59.89%   71.19%   +11.30%     
===========================================
  Files           6        6               
  Lines         182      184        +2     
===========================================
+ Hits          109      131       +22     
+ Misses         73       53       -20
Impacted Files Coverage Δ
openapi_schema_validator/validators.py 100.00% <100.00%> (ø)
openapi_schema_validator/_validators.py 61.66% <0.00%> (+27.76%) ⬆️
openapi_schema_validator/shortcuts.py 87.50% <0.00%> (+50.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab7222a...7ed2ea8. Read the comment docs.

p1c2u
p1c2u reviewed Dec 28, 2021
View reviewed changes
Copy link
Collaborator

@p1c2u p1c2u left a comment

Choose a reason for hiding this comment

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

We don't need __init__ files inside tests (pytest will treat it as a packages). Rest LGT

@p1c2u
Copy link
Collaborator

p1c2u commented Dec 28, 2021

@Larox thank you for the contribution

@p1c2u p1c2u merged commit 2468a62 into python-openapi:master Dec 28, 2021
jparise
jparise reviewed Dec 28, 2021
View reviewed changes
openapi_schema_validator/validators.py
@@ -63,7 +65,8 @@ def __init__(self, *args, **kwargs):

def iter_errors(self, instance, _schema=None):
if _schema is None:
_schema = self.schema
# creates a copy by value from schema to prevent mutation
_schema = deepcopy(self.schema)
Copy link
Contributor

@jparise jparise Dec 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this could be a shallow copy (_schema = self.schema.copy()), at least for the specific issue that's being addressed by this fix (because we only mutate by adding a new key). That would be a little more memory efficient than a full (deep) object copy.

@ngnpope ngnpope mentioned this pull request Feb 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jparise jparise jparise left review comments

@p1c2u p1c2u p1c2u left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Larox @p1c2u @jparise