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

Issues with ScopeFactory leading to unexpected errors, where annotations are ignored #263

Open
wynneplaga opened this issue Mar 20, 2024 · 0 comments

Comments

@wynneplaga
Copy link

(Uber employees may reference the original doc: here)

An issue can occur when using ScopeFactory to connect a parent and child scope to one another.

Dependencies listed in a hand-made Motif dependencies interface could match up with differently annotated access methods in the parent scope, causing, for example, application contexts to be provided where activity contexts were expected. A minimal example repository is available for inspection here, but the critical area is shown below:

@motif.Scope
interface ScopeParent: ScopeCreatableChild.Dependencies {

   fun scopeCreatableChild(): ScopeCreatableChild
  
   @AppContext
   fun context(): Context


   @ActivityContext
   fun activityContext(): Context
  
}


@motif.Scope
interface ScopeCreatableChild: Creatable<ScopeCreatableChild.Dependencies> {

   @ActivityContext
   fun activityContext(): Context


   interface Dependencies {
       // Matches to the activity method if created by a Motif child method,
       // but matches to the application method if created by [ScopeBuilder.create]
       @ActivityContext fun context(): Context
   }

}

fun main() {
   val scopeParent = ScopeFactory.create(ScopeParent::class.java)
   // prints “Parent: Activity”
   println("Parent: ${scopeParent.activityContext().type}")

   // prints “ScopeCreateableChild[Child Method]: Activity”
   println("ScopeCreateableChild[Child Method]: ${scopeParent.scopeCreatableChild().activityContext().type}")

   val scopeManualChild = ScopeFactory.create(ScopeCreatableChild::class.java, scopeParent)
   // prints “ScopeCreateableChild[Child Method]: Application”
   println("ScopeCreateableChild[ScopeFactory]: ${scopeManualChild.activityContext().type}")
}

Two solutions have been put forward internally:

Perspective 1

Author: @wynneplaga
Summary: Use of Creatable with ScopeFactory, except for root scopes, is an anti-pattern and should be forbidden

The Motif README explicitly discusses the use of ScopeFactory within the context of root scopes. This makes sense– Motif has a separate system for tying together parent and child scopes: the child scope method. As a result, using the ScopeFactory in order to recreate the parent-child relationship should be regarded as an anti-pattern, because it breaks Motif’s ability to match up dependencies and sources correctly. It also defeats Motif’s ability to detect duplicate sources when a parent already exposes the needed source. Rather than circumventing child methods by using a different pattern, any shortcomings with the behavior of child scope methods should be addressed by updating Motif itself.

Some standards recommend to use the ScopeFactory as an alternative to child scopes when creating impl feature modules, but the justification offered (“if a feature wants to declare exactly what dependencies should come from its parent”) can be adequately addressed by using Creatable with a traditional child method. This would allow developers to avoid reliance on generated code if they would like, but does not impair Motif’s ability to reason about sources and dependencies.

Suggested solution: A note discouraging the use of ScopeFactory outside of tests should be added, with a note that it is permissible to use for a truly root scope. Existing uses of ScopeFactory out of compliance with this rule should be eliminated. Users should be encouraged to add lint rules that flag uses of ScopeFactory

Perspective 2

Author: @SergeySmykovskyi
Summary: Modify the Motif’s codegen logic to detect and restrict overlap of methods signatures, to keep ScopeFactory usages in reasonable places.

The root cause of the issue is inheritance from the interface, which ignores the @Qualifier or @Named annotation used for DI code generation and uses @Override-equivalent of an already defined function in the Parent scope. That means that not only ScopeFactory usages are affected but also all of the PluginFactorories with Parent interface injection where the Scope inherits the Parent interface.

As was mentioned in the Perspective 1 the Motif documentation explicitly saying about usage of ScopeFactory for RootScope creation but at the same time doesn’t have examples of architecture in multi-modular projects, so “root” scope can be recognized as the root one of specific feature/library module. Restricting usage of the ScopeFactory will require us to keep all of the Scopes as a public API with doesn’t match “strict visibility control” principle and as a result will expose implementation details to external modules.

Suggested solution:

  • (preferred) Detect the interface inheritance that is used as a dependency for Creatable<*> and enforce using the same @Qualifier/@Named annotations as in Parent-matched provider

    • Pros
      • As part of integration's new lib version, all existing issues will be detected and resolved
    • Cons
      • Requires non-simple Motif library logic modifications.
  • Codegen enforcement to keep the method signature the same as generated methods with pattern: (qualifier+generic1+generic2+...+returnType).

    • Pros
      • Less duplicated generated code might slightly improve app size
    • Cons
      • Will require additional work to integrate new library version
      • Non-flexible DI API will require additional engeeniring time to resolve compile-time issues of unmatchable providers
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

No branches or pull requests

1 participant