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

fix unique schema key issues #416

Closed
wants to merge 41 commits into from

Conversation

@zedrdave
Copy link

commented Mar 31, 2019

Fixed 2 issues with current code:

        try:
            hash(attribute)
        except TypeError:
            attribute = tuple(attribute)
  1. If attribute is immutable (and can be hashed) nothing is actually done (the hash is not assigned).

  2. For schema attributes that are set (such as only, exclude etc), an exception is triggered and a tuple is used, which randomly fails to match on itself (since str(tuple(x)) is non-deterministic when x is a set)…

lafrech and others added some commits Mar 29, 2019

@lafrech lafrech requested a review from Bangertm Apr 1, 2019

@lafrech lafrech added the bug label Apr 1, 2019

@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Thanks for submitting this. I think it is on the right track.

We could discuss what needs to be covered by a try/except and by a if condition, but on the principle, it looks good to me.

Do you think you could add tests?

@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

MODIFIERS = ["only", "exclude", "load_only", "dump_only", "partial"]

Modifiers can be strings or iterables (hashable or not) such as list, tuple, set.

Would the code be simpler and the intent clearer if we just wrote it like this?

        try:
            # Hashable (string, tuple)
            attribute = hash(attribute)
        except TypeError:
            # Unhashable iterable (list, set)
            attribute = hash(frozenset(attribute))

dependabot-bot and others added some commits Apr 2, 2019

Bump pre-commit from 1.15.0 to 1.15.1
Bumps [pre-commit](https://github.com/pre-commit/pre-commit) from 1.15.0 to 1.15.1.
- [Release notes](https://github.com/pre-commit/pre-commit/releases)
- [Changelog](https://github.com/pre-commit/pre-commit/blob/master/CHANGELOG.md)
- [Commits](pre-commit/pre-commit@v1.15.0...v1.15.1)

Signed-off-by: dependabot[bot] <support@dependabot.com>
Merge pull request #414 from marshmallow-code/fix_resolve_parameters_…
…resolve_schema

 Fix reference parameter/response with MarshmallowPlugin
Merge pull request #419 from marshmallow-code/factorize_component_sub…
…sections_mapping

Factorize components subsections mapping and add utils.build_ref
@sloria

This comment has been minimized.

Copy link
Member

commented on d6e798e Apr 3, 2019

Woops. Thanks for fixing.

This comment has been minimized.

Copy link
Member Author

replied Apr 3, 2019

No pb. Thanks for releasing. Wasn't sure about the exact date (timezone ?) but I set it on April 2nd.

@Bangertm

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

Is there an example of a case where classes with the same modifiers are not matching properly?

The original intent of make_schema_key was to create a key that can be used in the ref dictionary. We were originally using just the schema class for the key but when we wanted to take modifiers into account we need a way to include them in that key. My thought process was to create a NamedTuple that included the class and each of the modifiers, but since some of the modifiers are not hashable (sets) we needed to convert them to something that is hashable.

The intent behind this code block is to test to see if the attribute is hashable and if not make it hashable. I wanted to avoid storing the hash in the NamedTuple for debug reasons i.e. if someone wanted to inspect the ref dictionary it would be more convenient to see something that looked like the original modifier than a random number.

Maybe instead of changing unhasable attributes to tuples we just change them to frozensets?

@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Maybe instead of changing unhasable attributes to tuples we just change them to frozensets?

Good point.

        try:
            # Hashable (string, tuple)
            hash(attribute)
        except TypeError:
            # Unhashable iterable (list, set)
            attribute = frozenset(attribute)

lafrech and others added some commits Apr 6, 2019

Merge pull request #424 from marshmallow-code/coerce_status_code_string
Coerce response status codes to strings
Merge pull request #421 from marshmallow-code/strip_empty_subsections…
…_from_docs

Strip empty component sections from doc
Bump sphinx from 2.0.0 to 2.0.1
Bumps [sphinx](https://github.com/sphinx-doc/sphinx) from 2.0.0 to 2.0.1.
- [Release notes](https://github.com/sphinx-doc/sphinx/releases)
- [Changelog](https://github.com/sphinx-doc/sphinx/blob/master/CHANGES)
- [Commits](sphinx-doc/sphinx@v2.0.0...v2.0.1)

Signed-off-by: dependabot[bot] <support@dependabot.com>
@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

I think my last proposal is in line with @Bangertm's intent and fixes the bug.

@zedrdave, would you like to

  • update your PR
  • add a test case
  • add yourself to AUTHORS.rst

?

Szabolcs and others added some commits Apr 11, 2019

Bump pre-commit from 1.15.1 to 1.15.2
Bumps [pre-commit](https://github.com/pre-commit/pre-commit) from 1.15.1 to 1.15.2.
- [Release notes](https://github.com/pre-commit/pre-commit/releases)
- [Changelog](https://github.com/pre-commit/pre-commit/blob/master/CHANGELOG.md)
- [Commits](pre-commit/pre-commit@v1.15.1...v1.15.2)

Signed-off-by: dependabot[bot] <support@dependabot.com>
Merge pull request #431 from blagasz/dev
Fix Marshmallow Schema's Meta exclude field error
Andrew Johnson
Use pytest.mark consistently
There are many uses of pytest.mark and two of them use the imported mark symbol. Most
uses access it via pytest.mark. This changes the two inconsistent references to also
use pytest.mark.
Andrew Johnson
Parameterize the validation property tests
Rather than test each field validation property with a separate test, each
field can have a set of properties that gets checked.
Andrew Johnson
Find type and format for field using type hierarchy
When a custom field is not directly found in the field mapping then we use
the MRO to look at parent classes to find  a suitable class to work out
what type of field this is.
Andrew Johnson
Merge pull request #435 from andrjohn/lists-are-arrays
Uses class hierarchy to find type and format properties
Merge pull request #438 from marshmallow-code/pre-commit-show-diff-on…
…-failure

tox/pre-commit: show-diff-on-failure
@zedrdave

This comment has been minimized.

Copy link
Author

commented Apr 27, 2019

@lafrech Hi, I finally have a bit of spare time. Will try and take care of this today.

Dave added some commits Mar 31, 2019

# Note: this test relies on non-deterministic hashing behaviour:
for i in range(0, 50):
assert make_schema_key(PetSchema(load_only=('1', '2', '3', '4'))) == make_schema_key(PetSchema(load_only=('4', '3', '2', '1')))

This comment has been minimized.

Copy link
@lafrech

lafrech Apr 28, 2019

Member

Those are not sets but tuples, so I don't think this test tests what it means to.

I understand the reason for the loop, but I wouldn't do that. I'd just test with a set and a loop.

assert make_schema_key(PetSchema(load_only=['1', '2', '3', '4'])) == make_schema_key(PetSchema(load_only=['4', '3', '2', '1']))
assert make_schema_key(PetSchema(load_only=set(['1', '2', '3', '4']))) == make_schema_key(PetSchema(load_only=set(['4', '3', '2', '1'])))
@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

Do you think you could rebase on dev (rather than merge dev in your branch)? It would make the diff and the history more readable. At this point, you're better off starting a new branch from dev and cherry-picking your fix. You can push force in this branch or start a new PR if that's more convenient to you.

I that's too much trouble, I'll try to find the time to pick this up.

@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

I did the rework in #439.

@sloria sloria closed this Apr 30, 2019

@zedrdave

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

@lafrech Thanks for doing the rewrite! (sorry for the lack of reactivity… haven't had lots of time to work on side-projects lately)

@zedrdave zedrdave deleted the zedrdave:fix-make-schema-key branch Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.