python-dependency-injector icon indicating copy to clipboard operation
python-dependency-injector copied to clipboard

Erroneous initialization of Resources

Open atten opened this issue 4 years ago • 9 comments

After upgrading from 4.10.1 to 4.29.2, I noticed that some Resources which remained unitialized in prior version unexpectedly started to initialize during init_resorces() call. The Selector provider initializes all nested resources disregarding selector key:

from dependency_injector import containers, providers


class PostgresAdapter:

    def __init__(self, host, port):
        print('Postgres initialized:', host, port)


class SQLiteAdapter:

    def __init__(self, db_file):
        print('Sqlite initialized:', db_file)


def setup_db_adapter(klass, **kwargs):
    yield klass(**kwargs)
    print('close')


class Container(containers.DeclarativeContainer):

    config = providers.Configuration()

    database = providers.Selector(
        config.db_type,
        postgres=providers.Resource(
            setup_db_adapter,
            klass=PostgresAdapter,
            host=config.db_host,
            port=config.db_port,
        ),
        sqlite=providers.Resource(
            setup_db_adapter,
            klass=SQLiteAdapter,
            db_file=config.db_file,
        ),
    )


if __name__ == '__main__':
    container = Container(
        config={
            'db_type': 'postgres',
            'db_host': 'localhost',
            'db_port': 5432,
        }
    )

    container.init_resources()
    container.shutdown_resources()

Output:

Postgres initialized: localhost 5432
Sqlite initialized: None
close
close

Expected output:

Postgres initialized: localhost 5432
close

Both resources are initialized even if db_type is unspecified, which is nonsence:

    container = Container(config={})

Output:

Postgres initialized: None None
Sqlite initialized: None
close
close

atten avatar Mar 11 '21 11:03 atten

Hello @atten ,

Method container.init_resources() initializes nested resources since version 4.14.0. Here is an entry from the changelog:

Fix an issue with container.init_resource() and container.shutdown_resource() ignoring nested resources that are not present on the root level. See issue: #380.

I'm sorry that this change affected you negatively. I didn't realize the Selector + Resource use case.

rmk135 avatar Mar 11 '21 13:03 rmk135

If you're looking for a workaround, you could stop calling container.init_resources(). It will make the initialization work lazily.

rmk135 avatar Mar 11 '21 13:03 rmk135

@atten , I think that container.init_resources() behaviour should keep initialize nested resources by default. This covers a use case when there is nested containers setup.

I could add a parameter to initialize only flat resources container.init_resources(nested=False). How does it look to you?

rmk135 avatar Mar 11 '21 17:03 rmk135

Or if lazy option with having no container.init_resources() call would work, I could leave it as it's now.

rmk135 avatar Mar 11 '21 17:03 rmk135

As a user of your package, I need to initialize resources which are enabled by Selector and keep disabled ones untouched (as it worked prior to 4.14.0. What you suggest with nested=False doesn't solve this problem as far as I can see (on a top-level, I dont't need to know whether resources are nested or not).

The example in the first message demonstrates a program with 2 databases (production-ready and testing) with a selector. Do you think that attempt of initializing both of them disregarding of specific config/environment is a good idea?

atten avatar Mar 11 '21 17:03 atten

The example in the first message demonstrates a program with 2 databases (production-ready and testing) with a selector. Do you think that attempt of initializing both of them disregarding of specific config/environment is a good idea?

No. It's not a good idea.


Selector and resource providers do not work the same way you described. Method container.init_resources() should initialize all resources in the container. Prior to version 4.10 this method didn't initialize nested resources. So if you redesign your example a bit you will get in the same trouble like with latest version:

class Container(containers.DeclarativeContainer):

    config = providers.Configuration()

    postgres_db = providers.Resource(
        setup_db_adapter,
        klass=PostgresAdapter,
        host=config.db_host,
        port=config.db_port,
    )

    sqlite_db = providers.Resource(
        setup_db_adapter,
        klass=SQLiteAdapter,
        db_file=config.db_file,
    )

    database = providers.Selector(
        config.db_type,
        postgres=postgres_db,
        sqlite=sqlite_db,
    )


if __name__ == '__main__':
    container = Container(
        config={
            'db_type': 'postgres',
            'db_host': 'localhost',
            'db_port': 5432,
        }
    )

    container.init_resources()  # 4.10.1: All resources initialized, not desired behaviour
    container.shutdown_resources()

Output:

Postgres initialized: localhost 5432
Sqlite initialized: None
close
close

Is it a bug or feature? Experience shows that it's a tricky question. Despite somebody finds it to be a bug, I consider your use case totally valid.

From the point of the library, method container.init_resources() should initialize all container resources. That's what happens now. I think that it's a fair statement.


Finally, speaking about your use case, I would recommend to redesign it that way:

from dependency_injector import containers, providers


class PostgresAdapter:

    def __init__(self, host, port):
        print('Postgres initialized:', host, port)


class SQLiteAdapter:

    def __init__(self, db_file):
        print('Sqlite initialized:', db_file)


def setup_db_adapter(adapter):
    yield adapter
    print('close')


class Container(containers.DeclarativeContainer):

    config = providers.Configuration()

    database = providers.Resource(
        setup_db_adapter,
        adapter=providers.Selector(
            config.db_type,
            postgres=providers.Factory(
                PostgresAdapter,
                host=config.db_host,
                port=config.db_port,
            ),
            sqlite=providers.Factory(
                SQLiteAdapter,
                db_file=config.db_file,
            ),
        ),
    )


if __name__ == '__main__':
    container = Container(
        config={
            'db_type': 'postgres',
            'db_host': 'localhost',
            'db_port': 5432,
        },
    )

    container.init_resources()
    container.shutdown_resources()

That example works like you expect. The redesign idea is to have single resource. I use selector as a resource dependency. The selector creates proper adapter and passes it to further initialization to the setup generator.

Does it make better sense to you?

rmk135 avatar Mar 11 '21 18:03 rmk135

Yes it does, thanks!

Your redesign proposal works as expected. However, the idea that init_resources() initializes ALL of them (including disabled by Selector) still seems to be too straightforward. Are resources in overridden containers initialized too?

atten avatar Mar 12 '21 06:03 atten

Are resources in overridden containers initialized too?

Yep.

However, the idea that init_resources() initializes ALL of them (including disabled by Selector) still seems to be too straightforward.

I understand. In fact selector does not "disable" a resource. It just selects the proper resource in runtime. At least this is how current implementation works.

As a short term plan, I can add example above to the resources docs. Long term, need to think of the design. Idea of disabling selector looks interesting. Fairly saying it even could be a new provider ResourceSelector.

rmk135 avatar Mar 12 '21 14:03 rmk135

Hm this gets a bit unergonomic when I for example want to switch between a resource and a factory. To visualize the problem:

class SecretManager(Protocol):
    async def get(self, name: str) -> Secret[str]:
        ...


class InMemorySecretManager(SecretManager):
    """In-memory mapping of secrets and their values"""
    def __init__(self, secrets: dict[str, str]) -> None:
        self.secrets = MappingProxyType(secrets)


class SSMSecretManager(SecretManager):
    """Backed by AWS Simple System manager"""
    def __init__(self, client: AioBaseClient) -> None:
        self.client = client


async def create_ssm_secret_manager(
    region_name: str,
    aws_access_key_id: str,
    aws_secret_access_key: str,
) -> SecretManager:
    session = aiobotocore.session.get_session()
    client_creator = session.create_client(
        "ssm",
        region_name=region_name,
        aws_access_key_id=aws_access_key_id,
        aws_secret_access_key=aws_secret_access_key,
    )
    async with client_creator as client:
        yield SSMSecretManager(client)


class Container(containers.DeclarativeContainer):
    settings = providers.Configuration()
    secret_manager: providers.Provider[SecretManager] = providers.Selector(
        settings.secret_manager.type,
        ssm=providers.Resource(
            create_ssm_secret_manager,
            region_name=settings.secret_manager.region_name.required(),
            aws_access_key_id=settings.secret_manager.aws_access_key_id.required(),
            aws_secret_access_key=settings.secret_manager.aws_secret_access_key.required(),
        ),
        memory=providers.Factory(
            InMemorySecretManager,
            secrets=settings.secret_manager.secrets.required(),
        ),
    )

Now when using this with the memory-backed secret manager, the container will complain about missing config for the SSM-backed manager.

If you're looking for a workaround, you could stop calling container.init_resources(). It will make the initialization work lazily.

I feel like a possible solution would be to make init_resources() respect the selectors and only initialize the selected resources. Then in case of an override, new resources would be initialized lazily.

kjagiello avatar Apr 05 '22 09:04 kjagiello