Mishandling of strict mode Mapping and Dict with non-string keys

[slippycheeze]

• typical version: 2.0.12 (and I verified the bug exists in current master.)
• Python version: CPython 3.7.7 (via homebrew)
• Operating System: macOS 10.15.4

Description

I tried using typic.al and typic.klass with the type annotation Mapping[Path, Path].

This failed with the error TypeError: sequence item 1: expected str instance, PosixPath found -- full backtrace at the very end, not that it matters, along with a little other debug output.

The goal was to have strict runtime validation; the decoration reflects the actual type passed. If it matters, I wanted runtime validation to catch type punning errors like passing a str where a List[str] was expected. The code in question mostly runs external applications, and operates on very small datasets, so the performance cost was not a concern.

Minimal reproduction

#!/usr/bin/env python3
from typing import Mapping
import typic

@typic.al(strict=True)
def bad(val: Mapping[int, str]) -> None:
  pass
bad({1: 1})

What is broken

The validation code tries to use the raw key in a call to str.join, which fails. Comes from the code in MappingConstraints._set_item_validator_keys_values_line() in typic/constraints/mapping.py

The code generated for describing the invalid mapping value is:

        field = f"'.'.join(({self.VALTNAME}, {self.X}))"

The correct version is probably:

# too horrible for words, really...
field = f"f'{{{self.VALTNAME}}}[{{repr({self.X})}}]'"
# nice version that someone could actually read...
field = ''.join(("f'{", self.VALTNAME, "}[{repr(", self.X, ")}]'"))

That fixes the bug in two ways, first by not using an almost-Any as an argument to str.join where a str is expected, but second by using repr to get a python-ish version of the value, not a string, which is kind of confusing.

Regardless, you could also fix it in the current model with:

        field = f"'.'.join(({self.VALTNAME}, repr({self.X})))"

FWIW, I'd very strongly argue that, at least for Mapping and Dict, using the . dot separator is wrong. That really belongs to class attributes, and that isn't the right context here. Given:

@dataclass
class MyDataclass:
  MyDictField: Dict[str, str]

MyDataclass(MyDictField={'hello': 12}

The validation output should probably be something like

MyDataclass.MyDictField['hello'] was int, expected str

Rather than:

MyDataclass.MyDictField.hello was int, expected str

Longer demo, showing various failure types

] python3 tmp/demo.py
bad: {1: PosixPath('/Users/slippycheeze')}
sequence item 1: expected str instance, int found
/Users/slippycheeze
ugly: {PosixPath('/Users/slippycheeze'): PosixPath('/Users/slippycheeze')}
Given value <PosixPath('/Users/slippycheeze')> fails constraints: (type='int', nullable=False, coerce=False)
/Users/slippycheeze
worse: {(1, 2): True}
sequence item 1: expected str instance, tuple found
True

Code for the long failure demo

#!/usr/bin/env python3
from typing import Any, Dict, Mapping, Tuple
from pathlib import Path
import typic

@typic.al(strict=True)
def bad(val: Mapping[Path, Path]) -> None:
  pass

@typic.al(strict=True)
def ugly(val: Dict[int, Path]) -> None:
  pass

@typic.al(strict=True)
def worse(val: Dict[Tuple, Any]) -> None:
  pass

# In each case the demo is that the key *is* valid, and is not simply an
# identity comparison.  Not that this is necessary, but y'know, thorough.
try:
  data = {1: Path.home()}
  print("bad:", repr(data))
  bad(data)
except Exception as e:
  print(e)
  print(repr(data[1]))

try:
  data = {Path.home(): Path.home()}
  print("ugly:", repr(data))
  ugly(data)
except Exception as e:
  print(e)
  print((data[Path.home()]))

try:
  data = {(1,2,): True}
  print("worse:", repr(data))
  worse(data)
except Exception as e:
  print(e)
  print(repr(data[(1,2,)]))

Long Backtrace of real world failure, pdb bits

[...elided the uninteresting parts]
  File "/Users/slippycheeze/share/fonts/conform.py", line 83, in task_fonts
    CopyFiles(copies)
  File "<string>", line 3, in __init__
  File "/Users/slippycheeze/homebrew/lib/python3.7/site-packages/typic/api.py", line 220, in __setattr_typed__
    self, name, __trans[name](item) if name in protos else item,
  File "/Users/slippycheeze/homebrew/lib/python3.7/site-packages/typic/constraints/common.py", line 103, in validate
    valid, value = self.validator(value)
  File "<typical generated validator_4371568336>", line 9, in validator_4371568336
    valid, val = validator_4371568336_item_validator(val, addtl)
  File "<typical generated validator_4371568336_item_validator>", line 7, in validator_4371568336_item_validator
    retx, rety = validator_4371568336_item_validator_keys_validator(x), validator_4371568336_item_validator_vals_validator(y, field='.'.join((valtname, x)))

] python3 tmp/demo.py
> <typical generated validator_4460259152_item_validator>(7)validator_4460259152_item_validator()
-> retx, rety = validator_4460259152_item_validator_keys_validator(x), validator_4460259152_item_validator_vals_validator(y, field='.'.join((valtname, x)))
(Pdb) p validator_4460259152_item_validator_vals_validator(y)
PosixPath('/Users/slippycheeze/b')
(Pdb) p '.'.join((valtname, x)
*** SyntaxError: unexpected EOF while parsing
(Pdb) p '.'.join((valtname, x))
*** TypeError: sequence item 1: expected str instance, PosixPath found
(Pdb) p valtname
'Mapping'
(Pdb) p x
PosixPath('/Users/slippycheeze/a')
(Pdb) p validator_4460259152_item_validator_keys_validator(x)
PosixPath('/Users/slippycheeze/a')
(Pdb) p validator_4460259152_item_validator_keys_validator
<bound method __AbstractConstraints.validate of (type='Path', nullable=False)>
(Pdb) p '.'.join((valtname, str(x)))
'Mapping./Users/slippycheeze/a'

[slippycheeze]

PS: I did validate my suggested fix worked for my specific issue, but I'm not familiar enough with the code (and don't have the time this second to learn that, and what backward compatibility is needed), so didn't send a patch. Hope this is sufficient anyhow.

[seandstewart]

Hey @slippycheeze -

Thank you for the reproduction. This is more than enough for a fix! I’ll make sure to have one out today.