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

InterfaceObjectIO: Updating Dict fields can go badly (WrongContainedType is mishandled) #121

Open
jamadden opened this issue Oct 5, 2021 · 0 comments

Comments

@jamadden
Copy link
Member

jamadden commented Oct 5, 2021

Suppose you have interfaces like these (and appropriate implementations):

from zope.schema import Dict, Object, TextLine
class IAddress(Interface):
    street_address_1 = TextLine()
    full_name = TextLine()
    ...

class IAddressable(Interface):
    # An entity can have multiple named addresses, e.g., home and office
    addresses = Dict(key_type=TextLine(), value_type=Object(IAddress))

Attempting to update an implementation of IAddressable from a dictionary like the following will fail with WrongContainedType:

{'addresses': {'mailing': {'Class': 'Address',
   'MimeType': 'application/vnd.nextthought.users.address',
   'full_name': 'J R Z',
   'street_address_1': '123 Address Avenue',
   'city': 'Town',
   'state': 'OK',
   'postal_code': 'ZIP',
   'country': 'United States'}}}

What's happening here is that nti.externalization+nti.schema basically doesn't handle WrongContainedType very well when it happens to a dictionary. For reference, here's InterfaceObjectIO.updateFromExternalObject():

https://github.com/NextThought/nti.externalization/blob/5a445b85fb809a7c27bf8dbe45c29032ece187d8/src/nti/externalization/datastructures.py#L755-L767

And here's the relevant portion of the superclass method it calls:

https://github.com/NextThought/nti.externalization/blob/5a445b85fb809a7c27bf8dbe45c29032ece187d8/src/nti/externalization/datastructures.py#L344-L354

The call to the superclass on line 713 winds up invoking self._ext_setattr("addresses", {"mailing":...}). InterfaceObjectIO overrides _ext_setattr to do this:

https://github.com/NextThought/nti.externalization/blob/5a445b85fb809a7c27bf8dbe45c29032ece187d8/src/nti/externalization/datastructures.py#L649-L650

That ultimately winds up in nti.externalization.internalization.fields:validate_field_value which (right now) simply calls IAddressable['addresses'].validate({"mailing": ...}). This raises WrongContainedType, naturally.

That's OK, we're expecting that, and we attempt to handle it by invoking the adapter machinery. Unfortunately, the code assumes that WrongContainedType is raised only for sequences (list, tuple), so even if the adapter machinery could handle IAddress({"mailing":...}), we'd still wind up with the wrong value (a list) and the field would throw validation errors (it's actually worse than that, we'd attempt to invoke IAddress("mailing") on the keys of the dictionary, so that's no good at all).

Remember where I said "right now" a minute ago? This gets somewhat better if you use nti.schema.field.DictFromObject as the field value instead of a plain zope.schema.Dict. This gets a field that understands how to convert incoming keys and values to the declared types. This still just uses the adapter machinery, so you'd also need to register a trivial adapter for IAddress:

   @implementer(IAddress)
   @adapter(dict)
   def dict_to_address(d):
      new_addr = Address()
      nti.externalization.update_from_external_object(new_addr, d)
      return new_addr

That's not ideal, since it bypasses all the registered factories and the factory finder (which can have security implications; some environments customize it to filter who can create what objects), but it should work.

So there are at least two issues here:

  1. Assuming all WrongContainedType belong to a sequence.

  2. Generally doing a bad job on dictionaries, including not invoking the factory machinery. Perhaps _ext_setattr should attempt some field transformations? This is doubly confusing because InterfaceObjectIO does define a find_factory_for_named_value method that doesn't get used in this code path.

NOTE: This has been seen in client code that, at the top level, uses nti.externalization.update_from_external_object(), but in the object that actually contains the addresses attribute, it calls InterfaceObjectIO directly. However, the same error happens if it calls the public API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant