fix: accept an app whose broker is created in an on_startup hook - #57
Merged
Merged
Conversation
setup_di refused an app with no broker at call time, which blocked the FastStream shape where the broker is built inside an on_startup hook and attached with app.add_broker. Since #52 the broker list is read at startup, so the guard was no longer needed for correctness. The guard moves into the startup install hook: it raises when the broker list is still empty when it runs, naming both remedies (pass a broker to FastStream or app.add_broker; register a broker-adding hook before setup_di, since hooks run in registration order). The FromDI message-time error names the other order. ADR-0002 records the change. Closes #56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #56. Option 3 from the issue.
What
setup_diraisedBroker must be defined to setup DIwhen the app had no broker at call time. FastStream supports creating the broker inside anon_startuphook and attaching it withapp.add_broker(itsset_brokerdocstring names this as the intended use), and the guard rejected that app at import time.How
setup_diis gone. Since fix: install the DI middleware on every broker, on startup #52 the middleware is installed by a startup hook that readsapp.brokers, so nothing atsetup_ditime depends on a broker existing.app.brokersis still empty when it runs, it raises aRuntimeErrornaming both remedies: pass a broker toFastStream(...)orapp.add_broker(...), and register a broker-addingon_startuphook beforesetup_di, because hooks run in registration order. This fires before FastStream's ownassert self.brokers.FromDImessage-time error from fix: name setup_di and app startup when FromDI finds no request container #55 gains one sentence for the other order: a broker added by a hook registered aftersetup_diis not covered.setup_dibullet and ADR-0002 updated. The ADR paragraph that deferred this decision now records it, including the one case that still surfaces at message time: a later hook adds a broker while another broker already exists.Tests
Both written first and red on
mainwith the old guard:test_broker_added_by_a_startup_hook_registered_before_setup_di_gets_di:FastStream()with no broker, anon_startuphook that callsadd_broker, thensetup_di; aFromDIsubscriber on that broker resolves underTestApp.test_app_without_broker_at_startup_names_both_remedies:setup_dion a broker-less app succeeds, andapp.start()raises the new error.test_app_without_brokeris replaced by these two, sincesetup_dino longer raises there.just lint-ciclean,just test-ciat 100% coverage.