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

schema.base.Element.validate malfunction #25

Open
ThomasWaldmann opened this issue Oct 30, 2018 · 0 comments
Open

schema.base.Element.validate malfunction #25

ThomasWaldmann opened this issue Oct 30, 2018 · 0 comments
Labels
bug Something isn't working

Comments

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Oct 30, 2018

there is a commented/disabled doctest in validation.containers.SetWithKnownFields now (introduced by c3d08d3 ):

from flatland import Dict, Integer
from flatland.validation import SetWithKnownFields
schema = Dict.of(Integer.named('x'), Integer.named('y')).validated_by(SetWithKnownFields())
schema.policy = None
element = schema()
element.set({'x': 123})
assert element.validate()  # assertion error, issue #25, FIXME!    (1)
element.set({'x': 123, 'z': 789})
assert not element.validate()  # no assertion error, but maybe due to #25 also.    (2)

I did a bit of debugging, but I am not familiar enough with the code to fix it, so here is what I found out:

There's a problem in validate():

valid might get set to False early (when encountering the y == None value) and thus validate() will not be able to return True after that. This is why assertion (1) fails although the SetWithKnownFields validator returns True.

    def validate(self, state=None, recurse=True):
        if not recurse:
            down = self._validate(state, True)
            if down is Unevaluated:
                self.valid = down
            else:
                self.valid = bool(down)

            up = self._validate(state, False)
            # an Unevaluated ascent validator does not override the results
            # of descent validation
            if up is not Unevaluated:
                self.valid = bool(up)
            return self.valid

        valid = True
        elements, seen, queue = [], set(), collections.deque([self])

        # descend breadth first, skipping any branches that return All*
        while queue:
            element = queue.popleft()
            if id(element) in seen:
                continue
            seen.add(id(element))
            elements.append(element)
            validated = element._validate(state, True)

            if validated is Unevaluated:
                element.valid = validated
            else:
                element.valid = bool(validated)
                if valid:
                    valid &= validated  # <<<<<<<<<<<<<<<<<< getting False here due to y == None
            if validated is SkipAll or validated is SkipAllFalse:
                continue
            queue.extend(element.children)

        # back up, visiting only the elements that weren't skipped above
        for element in reversed(elements):
            validated = element._validate(state, False)  # <<< SetWithKnownFields gets called, validated = True

            # an Unevaluated ascent validator does not override the results
            # of descent validation
            if validated is Unevaluated:
                pass
            elif element.valid:
                element.valid = bool(validated)
                if valid:  # <<<<<<< as valid is already False, it stays False
                    valid &= validated
        return bool(valid)
ThomasWaldmann added a commit to ThomasWaldmann/flatland that referenced this issue Oct 30, 2018
docs say:
"A mapping validator that ensures no unexpected fields were set()."

I added a doc test that *only* tests this (avoiding the bug that the
commented test triggered):
- setting all fields: valid
- setting all fields plus a unknown one: not valid

I opened issue discorporate#25 on the issue tracker about the fix that is still
needed.
@ThomasWaldmann ThomasWaldmann added the bug Something isn't working label Oct 30, 2018
ThomasWaldmann added a commit to ThomasWaldmann/flatland that referenced this issue Oct 30, 2018
docs say:
"A mapping validator that ensures no unexpected fields were set()."

I added a doc test that *only* tests this (avoiding the bug that the
commented test triggered):
- setting all fields: valid
- setting all fields plus a unknown one: not valid

I opened issue discorporate#25 on the issue tracker about the fix that is still
needed.
@ThomasWaldmann ThomasWaldmann changed the title schema.base.Element.validate broken schema.base.Element.validate malfunction Oct 31, 2018
ThomasWaldmann added a commit to ThomasWaldmann/flatland that referenced this issue Nov 1, 2018
docs say:
"A mapping validator that ensures no unexpected fields were set()."

I added a doc test that *only* tests this (avoiding the bug that the
commented test triggered):
- setting all fields: valid
- setting all fields plus a unknown one: not valid

I opened issue discorporate#25 on the issue tracker about the fix that is still
needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant