Skip to content

fix(ioc): drop type-lie in eager-DI provider construction#9

Merged
lesnik512 merged 1 commit into
mainfrom
bugfix/ioc-eager-di-none-field
Jun 8, 2026
Merged

fix(ioc): drop type-lie in eager-DI provider construction#9
lesnik512 merged 1 commit into
mainfrom
bugfix/ioc-eager-di-none-field

Conversation

@lesnik512

Copy link
Copy Markdown
Member

Summary

modern-di's Factory.resolve() eagerly resolves all provider_kwargs. The 0.3.x current_provider factory declared both gitlab_provider and github_provider as kwargs, so both got constructed on every resolution. When provider=github, _build_gitlab_provider(settings, gitlab_client) was called with settings.project_id=NoneGitLabProvider(project_id=None) — a field typed int holding None, suppressed with # ty: ignore. Harmless today (no method touches self.project_id at construction), but trivially broken if anyone adds a __post_init__ validator or eager field access.

This PR collapses the per-provider factories into a single _build_current_provider that takes both clients and constructs only the active provider, with asserts that narrow the Optional fields and document the Settings._resolve_provider validator invariant.

  • Delete _build_gitlab_provider, _build_github_provider, _close_gitlab_provider, _close_github_provider
  • Delete ProvidersGroup.gitlab_provider, ProvidersGroup.github_provider
  • Consolidate finalizers into one _close_client attached to each client factory's cache_settings (lifecycle target moved from provider to client)
  • _build_current_provider now takes (settings, gitlab_client, github_client); the unused client still resolves (eager Factory) but httpx2 connection pools are lazy

420 tests pass, 100% coverage, lint clean — existing test_ioc.py tests resolve current_provider and check the type, which still works under the new shape.

Test plan

  • just test — 420 pass (same total, no test removed/added)
  • just lint-ci — clean
  • Existing test_container_resolves_github_provider_when_settings_provider_is_github + ..._gitlab_..._is_gitlab confirm both branches dispatch correctly
  • Merge — bugfix/ prefix triggers dogfood auto-tag (0.3.2 → 0.3.3)
  • Manually cut 0.3.3 GitHub release to publish to PyPI (optional — internal-only refactor, no user-facing change)

🤖 Generated with Claude Code

… type-lie

Before: ProvidersGroup had gitlab_provider + github_provider factories
that were both eagerly resolved by current_provider's DI (modern-di's
Factory.resolve() eagerly resolves all provider_kwargs, confirmed in
factory.py:147). When provider=github, _build_gitlab_provider was
called with settings.project_id=None and constructed
GitLabProvider(project_id=None) — a `int`-typed field holding None
(suppressed with `# ty: ignore`). Harmless today because no method
references self.project_id at construction, but trivially broken if
someone adds a __post_init__ validator or eager field access.

After: drop the gitlab_provider/github_provider per-provider factories.
_build_current_provider takes both clients directly and constructs
only the active provider, narrowing settings.repo/project_id with
asserts that document the validator's invariant. The unused client
still resolves (eager Factory) but httpx2 connection pools are lazy
so no real cost.

Also consolidates _close_gitlab_provider + _close_github_provider
into one _close_client finalizer attached to each client factory's
cache_settings — the lifecycle target moved from the provider to
the client it owns.

Surfaced by the final code-review of the GitHub provider branch
(PR #4); deferred at landing because the dogfood (PR #5/#6/#8)
proved the type-lie path is currently unreachable. This PR closes
the gap before someone makes it reachable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lesnik512 lesnik512 self-assigned this Jun 8, 2026
@lesnik512 lesnik512 merged commit c5e9676 into main Jun 8, 2026
5 checks passed
@lesnik512 lesnik512 deleted the bugfix/ioc-eager-di-none-field branch June 8, 2026 18:36
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.

1 participant