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

fix: singleton reinstantiated if registered at last & fix: initialized class attribute being overridden #44

Merged
merged 3 commits into from
Nov 11, 2023

Conversation

lucas-labs
Copy link
Contributor

@lucas-labs lucas-labs commented Nov 11, 2023

Hi there, I made these two corrections that you may be interested in.

  • singleton being reinstantiated if registered after its dependent
    Right now, if we register a singleton dependency after registering it dependant, it will instanciate the singleton multiple
    times.
    With this change, I'm basically making the following test pass (it wouldn't pass with current release):

    class Bar:
        foo: Foo
    
    class Qux:
        foo: Foo
    
    container = Container()
    container.register(Bar)
    container.register(Qux)
    
    # we register the dependency AFTER already registered its dependents
    container._add_exact_singleton(Foo)
    
    bar = container.resolve(Bar)
    qux = container.resolve(Qux)
    foo = container.resolve(Foo)
    
    # check that singletons are always the same instance
    # the following line goes 💥 right now, and it has been fixed by this PR
    assert bar.foo is qux.foo is foo 
  • initialized class attribute being overridden
    If we declare a class attribute and initialize it inside the class, rodi should not override it or even try to resolve it (IMO).
    This is already being taken care of when the class attribute is annotated with ClassVar, but I think that the same logic
    should apply if the attribute already has a value at resolving time, in other words, if the user already resolved it for us.

    foo_instance = Foo()
    
    class A:
      # we explicitly initialize foo already
      foo: Foo = foo_instance
    
    container.register(A)
    container._add_exact_singleton(Foo)
    
    # right now, if Foo were not in the container
    # it would fail right here trying to resolve A
    a = container.resolve(A)
    
    foo = container.resolve(Foo)
    
    assert isinstance(a.foo, Foo)
    
    # right now, if Foo is in the container, the next line would fail
    # because rodi has overridden a.foo for a new instance
    assert foo_instance is a.foo
    assert foo is not a.foo

Warning
I think the first correction would be a nice addition and it shouldn't be a breaking change.
The second fix, however, could break existing code in case someone was initializing a dependency without noticing. It
would be an edge case, but... it could be.

lucas-labs and others added 3 commits November 11, 2023 03:00
changed service provider build logic in order to
avoid singletons to be instantiated n times when
they're registered after its dependant
changed the "ignore attributes" logic so that if
a class variable has already been initialized
externally, rodi doesn't attempt to reinitialize
it (and to also prevent overriding it if the
initialized class variable is also a registered
object).
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (de16370) 100.00% compared to head (b9aafd1) 99.82%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##              main      #44      +/-   ##
===========================================
- Coverage   100.00%   99.82%   -0.18%     
===========================================
  Files            2        2              
  Lines          555      560       +5     
===========================================
+ Hits           555      559       +4     
- Misses           0        1       +1     
Files Coverage Δ
rodi/__init__.py 99.82% <92.30%> (-0.18%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RobertoPrevato
Copy link
Member

Hi @lucas-labs
Thank You for your contributions! I merge and publish a new version to PyPi soon.

@RobertoPrevato RobertoPrevato merged commit 6043b5b into Neoteroi:main Nov 11, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants