CBIIT / bento-meta

Bento Metamodel
8 stars 2 forks source link

set_with_node doesn't access Node mapspec dict #12

Closed bensonml closed 4 years ago

bensonml commented 4 years ago

https://github.com/CBIIT/bento-meta/blob/90efa1bdd396d8c18caf38f962ffa9ab1e6a11ee/python/bento_meta/entity.py#L178

If the attribute "nanoid" is added to class Node(Entity)

    attspec = {
        "handle": "simple",
        "model": "simple",
        "category": "simple",
        "nanoid": "simple",
        "concept": "object",
        "props": "collection",
    }
    mapspec_ = {
        "label": "node",
        "key": "handle",
        "property": {"handle": "handle", "model": "model", "category": "category", "nanoid": "nanoid"},
        "relationship": {
            "concept": {"rel": ":has_concept>", "end_cls": "Concept"},
            "props": {"rel": ":has_property>", "end_cls": "Property"},
        },
    }

Then a KeyError is raised:

>           patt = type(self).mapspec()["property"][att]
E           KeyError: 'nanoid'

.tox/py3/lib/python3.7/site-packages/bento_meta/entity.py:227: KeyError

the mapspec will instead hold (note no nanoid)

{'key': '_id',
 'label': 'node',
 'property': {'_from': '_from',
              '_id': 'id',
              '_to': '_to',
              'category': 'category',
              'desc': 'desc',
              'handle': 'handle',
              'model': 'model'},

However, this problem does not appear to exist with other entities such as class Property(Entity), or class Edge(Entity)

Proof of workaround is in branch pep8. See https://github.com/CBIIT/bento-meta/blob/pep8/python/bento_meta/entity.py

            # WARNING: the node.mapspec()["property"] is NOT reading
            #          what is defined in objects.py class Node(Entity)
            #          can add 'nanoid' to attspec and mapspec_ but it will
            #          NOT show up here, not yet read?
            # Ergo, code needs to check that the attribute att actually
            # exists in the mapspec()["property"] dictionary, or else it will
            # raise KeyError here
            if att in (type(self).mapspec()["property"]):
                patt = type(self).mapspec()["property"][att]
                if patt in init:
                    setattr(self, att, init[patt])
                else:
                    setattr(self, att, None)

            # NOTE: 'one-off' munge to allow 'nanoid' attspec for Node
            # P.S.: don't let mom see I coded this...
            # TODO fix this!
            if (att == "nanoid" and type(self) == "Node"):
                patt = "nanoid"
                if patt in init:
                    setattr(self, att, init[patt])
                else:
                    setattr(self, att, None)
bensonml commented 4 years ago

going through def set_with_node(self, init) , the self has the base Entity class mapspec dict, but not the Node class mapspec, so when it attempts to look for the 'nanoid' key in the mapspec()['property'] dict, it throws a KeyError (b/c it isn't there, see error note above)

majensen commented 4 years ago

The setwith* are really meant only to be called within __init__-- they enable the constructor to accept a number of types of initial values. (This is why these methods don't have docstrings.) When the subclass calls Entity.__init__, then self has the right type.

I can't really see the test, but my test of nanoid would be that the following doesn't throw.

 n = Node( { "model":"test", "nanoid":"blurg") )
 assert n.nanoid == "blurg"

set_with_node is about initializing an object with a neo4j node as returned from the database driver. so a test of that would be

with drv.session() as session;
  result = session.run('create (n:node {model:"test", handle:"test", nanoid:"blurg"}) return n')
  for rec in result:
    # this succeeds
    n = Node(rec['n'])
    assert n.nanoid == "blurg"
bensonml commented 4 years ago

Here, try on the git branch nanoiderror

git checkout nanoiderror
tox

It fails: tests/test_006mapmodel.py FF

If I add nanoid to Property, it doesn’t fail (on branch nanoidok)

git checkout nanoidok
tox
majensen commented 4 years ago

I pushed the test_000 fix (see 4008093) to the nanoiderror branch, which gave me success in tox run

bensonml commented 4 years ago

awesome! Thank you! yup, It works cleanly for me as well. I deleted branches nanoiderror and nanoidok