opsmill / infrahub-sdk-python

Python SDK to interact with Infrahub
https://www.opsmill.com
Apache License 2.0
3 stars 2 forks source link

bug: related object is not fetched when using `prefetch_relationships` #15

Open dgarros opened 3 months ago

dgarros commented 3 months ago

Component

Python SDK

Infrahub version

0.15.1

Current Behavior

In the python SDK, when we are using the flag prefetch_relationships on a query (filters, get or all) it's is still required to call fetch() to access the related object

>>> dev = client.get(kind="InfraDevice", name__value="atl1-edge1", prefetch_relationships=True)
>>> dev.site.peer
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/damien/projects/infrahub_bison/python_sdk/infrahub_sdk/store.py", line 40, in _get
    raise NodeNotFoundError(
infrahub_sdk.exceptions.NodeNotFoundError:
        Unable to find the node in the Store
        None | LocationSite | {'key': ['17e63e1f-98ba-b789-1156-c512bdd7ab92']}

>>> dev.site.fetch()
>>> dev.site.peer
atl1

Expected Behavior

When using prefetch_relationships, it shouldn't be required to run fetch() to access the related node

Steps to Reproduce

Additional Information

No response

wvandeun commented 3 months ago

AFAIK you have to use populate_store=True argument with prefetch_relationships=True. It may not be explicitly called out in the documentation and maybe we should change that behavior?

dev = client.get(kind="InfraDevice", name__value="atl1-edge1", prefetch_relationships=True, populate_store=True)
dev.site.peer
BeArchiTek commented 3 months ago

Exact, prefetch_relationships need populate_store at the moment. We could consider "forcing" it if prefetch_relationships=True

wvandeun commented 3 months ago

We could consider "forcing" it if prefetch_relationships=True

This makes a lot of sense to me.

ogenstad commented 3 months ago

I've also been confused by this in the past. I don't really like the way it currently works but depending on what we mean by "forcing" this sounds wrong to me. If we specify populate_store=False as a user I wouldn't expect this to be overridden and done anyway behind the scenes. With the current function signature I think it would be better if we raised an error if someone tried to set prefetch_relationships=True when populate_store was set to false.

wvandeun commented 3 months ago

IMO we could consider to set the default value for populate_store to True, but the user can still disable this behavior by setting it to False when calling any of these methods?

dgarros commented 3 months ago

IMO we could consider to set the default value for populate_store to True, but the user can still disable this behavior by setting it to False when calling any of these methods?

That works for me