factory_boy icon indicating copy to clipboard operation
factory_boy copied to clipboard

Help me understand: factory.errors.CyclicDefinitionError: Cyclic lazy attribute definition for

Open mark0978 opened this issue 7 years ago • 5 comments

In the code below, I can create a NotificationEvent, but I cannot create a Notification, if I also try to create the NotificationEvent.

I've tried several different things, but I'm finally stuck with this error: factory.errors.CyclicDefinitionError: Cyclic lazy attribute definition for user_account; cycle found in ['account', 'user_account'].

Sadly there is not much in the way of docs to help solve what is going on here. I'm going to try using Params and see if that helps, but I think it does mostly the same as what I have below.

Is it possible to get some more documentation around this error and around how you create Objects that have some nesting, but can be used without the nesting?

# encoding: utf-8

import factory
from .uniregistry_model_factory_base import UniregistryModelFactoryBase
from uniregistrar.models.db import DBSession
from uniregistrar.factories import UserAccountFactory

from notification.models import (Notification, NotificationEvent, NotificationSubcategory,
                                 NotificationSeverity)

class NotificationEventFactory(UniregistryModelFactoryBase):
    class Meta:
        model = NotificationEvent
        sqlalchemy_session = DBSession
        exclude = ("user_account", )

    user_account = factory.SubFactory(UserAccountFactory)
    subcategory = "domain-sold"
    severity = "info"
    account = factory.SelfAttribute("user_account.account")
    message = factory.Sequence(lambda n: "Message %d" % n)
    object = factory.Sequence(lambda n: "Object %d" % n)
    url = factory.Sequence(lambda n: "https://uni.com/url?p=%d" % n)

    @classmethod
    def attributes(cls, create=False, extra=None):
        if 'account' in extra:
            raise ValueError("Pass a user_account, not individual account values")

        return super(NotificationEventFactory, cls).attributes(create, extra)

    @classmethod
    def _create(cls, model_class, *args, **kwargs):
        return super(NotificationEventFactory, cls)._create(model_class, *args, **kwargs)


class NotificationFactory(UniregistryModelFactoryBase):
    class Meta:
        model = Notification
        sqlalchemy_session = DBSession
        exclude = ("user_account", )

    user_account = factory.SubFactory(UserAccountFactory)
    user = factory.SelfAttribute("user_account.user")
    event = factory.SubFactory(NotificationEventFactory,
                               user_account=factory.SelfAttribute("user_account"))
    # website_show = defaults to True in the DB
    # viewed_date = timestamp, defaults to NULL in the DB
    # notified_date = timestamp defaults to NULL in the DB

    @classmethod
    def attributes(cls, create=False, extra=None):
        if 'user' in extra:
            raise ValueError("Pass a user_account, not individual user values")

        return super(NotificationFactory, cls).attributes(create, extra)

    @classmethod
    def _create(cls, model_class, *args, **kwargs):
        return super(NotificationFactory, cls)._create(model_class, *args, **kwargs)

mark0978 avatar Nov 10 '18 01:11 mark0978

While I still don't understand the error message, my problem was fixed by changing

    event = factory.SubFactory(NotificationEventFactory,
                               user_account=factory.SelfAttribute("user_account"))

to

    event = factory.SubFactory(NotificationEventFactory,
                               user_account=factory.SelfAttribute("..user_account"))

I've got an idea as to why, but I can't justify it.

mark0978 avatar Nov 11 '18 04:11 mark0978

Hi! Indeed, we should write a « solving common errors » section in the docs — any help would be more than welcome!

Regarding your issue, let's walk through the messages.

The error

You get an error saying "I find a cyclic definition: the value of user_account depends on the value of account, which itself relies on user_account".

Under the hood, factory_boy starts by collecting all declarations from the factory it's going to instantiate, and walks through all those attributes to "resolve" them (i.e convert a declaration into an actual value). Since some fields might depend on the value of other fields, we keep a list of « attributes currently being computed »: if A depends on B, and B on C, and C on A, we'll raise when we discover that C is trying to access the value of A. => That's the issue you see here.

The cause

So, where do we have both account depending on user_account, and user_account on user_account?

When you call your NotificationFactory, it tries to instantiate a NotificationEventFactory with an extra declaration, user_account=factory.SelfAttribute("user_account"). This extra declaration is added to the list of declarations of the NotificationEventFactory during that instantiation.

This is equivalent to the following (shortened) factory:

class NotificationEventFactory(factory.Factory):
    class Meta:
        model = NotificationEvent

    account = factory.SelfAttribute('user_account.account')
    user_account = factory.SelfAttribute('user_account')

Note how:

  • the user_account=factory.SelfAttribute() passed in the SubFactory shadows the initial user_account = factory.SubFactory() from the initial class declaration;
  • Your declaration says that "user_account takes the value of user_account". => Your issue is in that second point

The solution

From your description, when building a Notification, you want the NotificationEvent to use the same UserAccount as the Notification.

The proper declaration is thus:

class NotificationFactory(factory.Factory):
    class Meta:
        model = Notification

    user_account = factory.SubFactory(UserAccountFactory)
    user = factory.SelfAttribute('user_account.user')  # Pick the 'user' from Notification.user_account
    event = factory.SubFactory(
        NotificationEvent,
        # Retrieve the user_account from the *calling* Notification
        user_account=factory.SelfAttribute('..user_account'),
    )

Basically, any extra declarations added in a SubFactory are evaluated from the point of view of the SubFactory; we need to get back to the factory calling our SubFactory, thus the ..user_account

rbarrois avatar Nov 12 '18 10:11 rbarrois

That last part was what I had figured out and you did indeed confirm what I was suspecting. The lazy part of that declaration is so lazy it doesn't get evaluated when the SubFactory "call" is made, but when it is actually used within the SubFactory and that is why you need the .. at the beginning of the SelfAttribute.

On Mon, Nov 12, 2018 at 5:00 AM Raphaël Barrois [email protected] wrote:

Hi! Indeed, we should write a « solving common errors » section in the docs — any help would be more than welcome!

Regarding your issue, let's walk through the messages. The error

You get an error saying "I find a cyclic definition: the value of user_account depends on the value of account, which itself relies on user_account".

Under the hood, factory_boy starts by collecting all declarations from the factory it's going to instantiate, and walks through all those attributes to "resolve" them (i.e convert a declaration into an actual value). Since some fields might depend on the value of other fields, we keep a list of « attributes currently being computed »: if A depends on B, and B on C, and C on A, we'll raise when we discover that C is trying to access the value of A. => That's the issue you see here. The cause

So, where do we have both account depending on user_account, and user_account on user_account?

When you call your NotificationFactory, it tries to instantiate a NotificationEventFactory with an extra declaration, user_account=factory.SelfAttribute("user_account"). This extra declaration is added to the list of declarations of the NotificationEventFactory during that instantiation.

This is equivalent to the following (shortened) factory:

class NotificationEventFactory(factory.Factory): class Meta: model = NotificationEvent

account = factory.SelfAttribute('user_account.account')
user_account = factory.SelfAttribute('user_account')

Note how:

  • the user_account=factory.SelfAttribute() passed in the SubFactory shadows the initial user_account = factory.SubFactory() from the initial class declaration;
  • Your declaration says that "user_account takes the value of user_account". => Your issue is in that second point

The solution

From your description, when building a Notification, you want the NotificationEvent to use the same UserAccount as the Notification.

The proper declaration is thus:

class NotificationFactory(factory.Factory): class Meta: model = Notification

user_account = factory.SubFactory(UserAccountFactory)
user = factory.SelfAttribute('user_account.user')  # Pick the 'user' from Notification.user_account
event = factory.SubFactory(
    NotificationEvent,
    # Retrieve the user_account from the *calling* Notification
    user_account=factory.SelfAttribute('..user_account'),
)

Basically, any extra declarations added in a SubFactory are evaluated from the point of view of the SubFactory; we need to get back to the factory calling our SubFactory, thus the ..user_account

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/FactoryBoy/factory_boy/issues/540#issuecomment-437822153, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGepoEQw-gOGRZOOcll_GDeI6hUP3Tqks5uuUa1gaJpZM4YXwFm .

mark0978 avatar Nov 12 '18 10:11 mark0978

Basically, any extra declarations added in a SubFactory are evaluated from the point of view of the SubFactory; we need to get back to the factory calling our SubFactory, thus the ..user_account

This was key to understanding this issue. Thanks!

Skowt avatar Jun 24 '20 19:06 Skowt

Nice explanation @rbarrois , much appreciated.

00dav00 avatar Apr 29 '21 21:04 00dav00