-
Notifications
You must be signed in to change notification settings - Fork 55
some updates for dev documentation #10065
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
base: release-1.11
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,12 +18,21 @@ Constructor dependencies for new code are required parameters - not `collaborato | |
|
|
||
| The single exception is editing existing code where adding a required parameter would force a large change across many call sites. There, an optional parameter is a transitional compromise to keep the change small - not the target shape for new components. | ||
|
|
||
| Late registration is the same anti-pattern in another shape. `set_collaborator(x)`, `register_handler(fn)`, or assigning `obj.on_change = fn` after construction hides the dependency at construction, lets a caller skip wiring it, lets a second caller silently clobber the first's, and forces a `None` check at every use site. Pass it to `__init__`. When the component feeds zero or more collaborators rather than exactly one, that argument is a required `list[...]`, and callers with nothing to wire pass `[]` explicitly. | ||
|
|
||
| ## Build components near the application entry point | ||
|
|
||
| Construct components as close to the application entry point as possible. Use a builder class or factory function when wiring is non-trivial, and inject each sub-component rather than constructing it inside a parent component's `__init__`. | ||
|
|
||
| Prefect `@flow` functions are application entry points: resolve singleton getters (`get_database()`, `get_workflow()`, …) at the top of the flow only — never inside helpers or component internals — then build the component and delegate to it. The flow body stays a thin composition root; the business logic lives in the component. | ||
|
|
||
| Anything that comes from outside the component's own domain — loaded settings, an external service client, a telemetry sink — is resolved at that entry point, never inside the component: | ||
|
|
||
| - **Settings resolve in the factory, not the component.** A component takes plain values (`window_seconds: float`, `max_retries: int`), never a `Settings` object and never a module-global read. The factory is then the only place that knows a value came from configuration, which is also what makes the component directly testable with hand-picked values. | ||
| - **The factory takes its out-of-domain collaborators as parameters too**, rather than choosing them. A factory that both reads settings *and* picks the concrete adapters has only moved the coupling one level out; take them as arguments so the entry point names them and the factory stays reusable with different ones. | ||
| - **Configure at construction, never by assignment afterwards.** Reaching into a built object to finish setting it up leaves a window in which it is misconfigured, makes a fixed value look mutable, and scatters the wiring across two places. Pass it to `__init__`, and expose it through a read-only property if callers need to read it back. | ||
| - **A lazily-built process-global lives in its own registry module**, separate from the component it builds, so importing the component never drags the wiring — and the dependencies behind it — into the import chain. Build on first use rather than at import, so the settings read happens after configuration is loaded and importing the module stays free of side effects. | ||
|
|
||
| ## Single entry point, operating on arguments | ||
|
|
||
| A component should generally expose a single entry point method (occasionally more, when justified by cohesive responsibility). That method only accepts the entities being operated on as arguments — it should not require additional dependencies to be passed in alongside the work payload. | ||
|
|
@@ -57,6 +66,28 @@ A single implementation does not need an interface yet; introduce one when the s | |
| can be either a no-op version (such as in the case of an enterprise-only feature) or a testing version of a component (such as in the case of an | ||
| in-memory version of a component typically backed by the database). | ||
|
|
||
| ## Interfaces to keep an out-of-domain dependency out | ||
|
|
||
| The other reason to declare a `Protocol` is to invert a dependency direction, and there **one implementation is enough**. The situation: a component's logic has no business knowing about some out-of-domain concern — metrics, tracing, analytics, an audit trail, a notification service — but something has to feed that concern from inside the component's flow. Importing the client directly is what you are avoiding: it makes the dependency viral, drags a third-party package into the import chain of pure logic, and means the component can no longer be constructed in a test without it. | ||
|
|
||
| There are two acceptable shapes for the interface itself. Both keep the adapter and the logic from importing each other; pick one per interface and be consistent within it. | ||
|
|
||
| 1. **Implicit — a `Protocol` declared beside the consumer, which the adapter never imports.** Structural typing is what makes this work: the adapter satisfies the protocol by having matching signatures, so nothing in the adapter's module points back at the consumer's. This is the lower-friction option: one new class, no new module, and no coordination with the adapter. | ||
| 2. **Explicit — an interface in a module of its own that both sides import.** Put the `Protocol` (or an ABC, if you want subclassing enforced) in a small, dependency-free interface module; the consumer imports it to type its constructor parameter, and the adapter imports it to declare that it implements it — by subclassing the ABC, or by subclassing the `Protocol` / annotating itself against it. Neither side imports the other, so the dependency still points inward at the interface, but the contract is now named at both ends: the adapter states what it implements, `mypy` checks it at the definition rather than only at the wiring call, and a reader of the adapter can find the interface without knowing which component motivated it. Explicit is better than implicit — prefer this one whenever the interface is worth naming as a contract, which is the case as soon as it has more than one implementer or more than one consumer. | ||
|
|
||
| An ABC only works in shape 2 — a subclass must import whatever module the base lives in, so an ABC declared in the consumer's module drags the dependency backwards. Never do that; if you want an ABC, give it its own module. | ||
|
|
||
| Whichever shape you pick, the remaining two parts do not change: | ||
|
|
||
| - **Name the methods in the depending component's vocabulary**, not the adapter's — `on_depth_changed(*, queued: int, running: int)`, not `set_gauges(...)` — and pass the values as arguments rather than handing over `self`, so the adapter can never read back into the component. The component then depends on a shape it defined, has no idea what is on the other side, and stays free to change its internals. | ||
| - **Put the concrete adapter in a separate, purpose-named module** that is the only place importing the library, and **let only the wiring layer import both** (see "Build components near the application entry point"). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there anything worth saying about explicitly declaring the protocol in the class definition? I mean this: And not: The upside of the first option is the direct link between the implementation and the protocol it satisfies. The downside is that we would need to import the protocol. Another option would be to make sure these protocols live in a package that is safe to import from anywhere.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm always ready to say more about dependency injection. added some more detail to describe either using a Protocol without importing it or using a Protocol/ABC interface defined in a separate module |
||
|
|
||
| The acceptance test is an import-graph one: after this, the library is reachable from the entry point and from the adapter module, and from nowhere in the logic. Verify it by grepping for the package name — if it appears anywhere under the component's own package, the split is incomplete. | ||
|
|
||
| This is the deliberate exception to "a single implementation does not need an interface yet" above. The interface earns its place by fixing which way the dependency points, not by abstracting over variants — and in practice the test doubles become the second and third implementations anyway. | ||
|
|
||
| `backend/infrahub/api/admission/` is a worked example: the decision logic, its protocols, the concrete sinks, and the factory that names them are four separate concerns in four modules. | ||
|
|
||
| ## Dispatching across implementations | ||
|
|
||
| When a component must pick one of several implementations at runtime based on the input, do not branch with `isinstance` (or a `match` on the input's type) inside one class. Give each implementation a predicate on the shared interface (e.g. `supports(request) -> bool`) alongside its entry method, hold the implementations as an injected list in an aggregator component, and let the aggregator delegate to the first that supports the input: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,6 +173,50 @@ branch_data = {"name": "feature-x", "description": None} | |
| branch_data = BranchCreateInput(name="feature-x") | ||
| ``` | ||
|
|
||
| ## Configuration Settings | ||
|
|
||
| `backend/infrahub/config.py` is the boundary where an operator's environment enters the process. Any value that gets past a `Field` is trusted by everything downstream, so constrain it here rather than defending against it in the logic that consumes it. | ||
|
|
||
| ### Bound every numeric field | ||
|
|
||
| Give each numeric field the tightest bounds its *meaning* allows, not just `gt=0`. A multiplier that may only scale a value **up** is `ge=1`; one that may only scale it **down** is `gt=0, le=1`. A count that must leave room for at least one item is `ge=1`, not `ge=0`. | ||
|
|
||
| Every `float` field also needs `allow_inf_nan=False`. Pydantic accepts `inf` and `nan` for floats by default, and both slip past `gt`/`ge`: `inf` produces a threshold that can never be reached, and `nan` makes every comparison against it `False`, so the feature quietly stops working somewhere far from the config file. | ||
|
|
||
| ```python | ||
| retry_backoff_multiplier: float = Field( | ||
| default=2.0, | ||
| ge=1, | ||
| allow_inf_nan=False, | ||
| description="Factor applied to the delay after each failed attempt.", | ||
| ) | ||
| ``` | ||
|
|
||
| ### Enforce cross-field invariants with a model validator | ||
|
|
||
| Per-field bounds cannot express a relationship *between* settings. When one setting is only meaningful relative to another — an ordering, a window that must contain another, a ceiling that must sit above its floor — assert it in a `@model_validator(mode="after")`. A contradictory configuration then fails at startup with an explanation, instead of silently inverting the feature's behavior at runtime. | ||
|
|
||
| ```python | ||
| @model_validator(mode="after") | ||
| def validate_retry_delay_bounds_ordered(self) -> Self: | ||
| """Require the initial retry delay to fall within the configured ceiling. | ||
|
|
||
| Raises: | ||
| ValueError: If the initial delay exceeds the maximum. | ||
|
|
||
| """ | ||
| if self.retry_initial_delay_seconds > self.retry_max_delay_seconds: | ||
| raise ValueError( | ||
| "'retry_initial_delay_seconds' must not exceed 'retry_max_delay_seconds', " | ||
| "otherwise the backoff ceiling is reached before the first retry" | ||
| ) | ||
| return self | ||
| ``` | ||
|
|
||
| Name the validator after the invariant it enforces. Name the offending fields in full in the message so they are greppable, and state both the rule and its consequence — the operator reading it in a crash log has no other context. | ||
|
|
||
| Testing note: don't test that Pydantic enforces `ge`/`le` (see [Testing Standards](./testing.md#what-not-to-test)), but *do* test the model validator and the shipped defaults — the invariant and the defaults are ours. | ||
|
|
||
| ## Docstrings (Google-style) | ||
|
|
||
| All public functions and classes must have Google-style docstrings: | ||
|
|
@@ -233,6 +277,8 @@ When a field or argument accepts only a fixed set of values, don't type it as a | |
| - **`Literal["a", "b"]`** — the lighter option for a small closed set used in a **single file**. Still type-checked, no class to declare. | ||
| - **An enum** — when the set is shared across modules, needs a name, round-trips through the database, or is exposed over GraphQL. Subclass `str` so the value round-trips as text — `StrEnum` on the backend (Python 3.11+); use `class X(str, Enum)` for code shared with `python_testcontainers` (which targets 3.10). | ||
|
|
||
| A value that also leaves the process as an external label — a metric label, a response field, a log key — is shared by definition, so it gets the enum even if only one module reads it today. Note that second role in the enum's docstring, and pass the member itself at the emit site rather than a parallel string literal, so the exported set and the branched-on set cannot drift apart. | ||
|
|
||
| ```python | ||
| # ❌ Bad - any string is accepted; a typo silently bypasses downstream logic | ||
| origin: str | None = None | ||
|
|
@@ -370,6 +416,19 @@ except Exception as exc: | |
|
|
||
| A broad `except Exception` is justified only at a top-level boundary (a task worker loop, a request handler) whose job is to prevent one failure from taking down the process — and even there, log the exception and re-raise or record it, never discard it. | ||
|
|
||
| The one other justified case is a **best-effort side-effect boundary**: notifying an observer or other sink whose failure must not affect the operation that triggered it. There the exception is logged and deliberately not re-raised, and the `try` must wrap the *individual* call so one bad sink cannot skip the ones behind it: | ||
|
|
||
| ```python | ||
| # ✅ Good - a failing sink must not fail the operation that produced the event | ||
| for observer in self._observers: | ||
| try: | ||
| observer.on_depth_changed(queued=queued, running=running) | ||
| except Exception: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should include Instead we should have a specific exception even if it's made up so that the LLM doesn't pick up on this part and start to introduce additional broad exceptions. We also have #10002.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in the case of an "observer" object (or any other "best-effort side-effect" components that should not block the primary logic during failure) we want to catch the bare |
||
| log.warning("queue observer raised; continuing", exc_info=True) | ||
| ``` | ||
|
|
||
| This is narrow: it applies where the emitting code owns no part of the sink's contract and its own operation is correct regardless of the outcome. It is not a licence to wrap a block of real work. | ||
|
|
||
| ## ASGI Middleware | ||
|
|
||
| <!-- Extracted from specs/ifc-2886-priority-api-backpressure on 2026-07-26 --> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any examples of this? In situations where there's a high number of settings i thing a
Settingsobject can be cleaner. An example would be the Config object for the SDK.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
Configis a special case b/c it is a settings object. its only responsibility should be tracking many settings. any components that actually do things should have settings injected when they are constructed.