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

Feature: Implements on_app_init hook #499

Merged
merged 11 commits into from
Sep 23, 2022
Merged

Feature: Implements on_app_init hook #499

merged 11 commits into from
Sep 23, 2022

Conversation

peterschutt
Copy link
Contributor

@peterschutt peterschutt commented Sep 20, 2022

on_app_init callbacks

Issue with the current PluginProtocol.on_app_init() system.

  • The Plugin.on_app_init() callback occurs before route handler registration, so plugins do not
    have the opportunity to receive the handlers passed to the application before they are applied to
    the app.
  • Certain application attributes have pre-processing applied to them on assignment to self.
    Therefore, if a plugin wants to manage or modify these attributes, the plugins need to perform this
    step of pre-processing, or at least understand it, before they can operate on these configuraion
    attributes. E.g., as_async_callable_list().
  • Difficult to be confident in longevity of implementations. Simple things like order of
    configuration/attribute changes, or changes to wrapper objects in later releases may impact the way
    plugins need to work to operate on certain configuration values.
  • We run the risk of a plugin that abuses internal apis in PluginProtocol.on_app_init() becoming popular and making a rod for our backs in terms of internal refactoring.

Proposed solution

  • We construct a pydantic model of the arguments that the application object receives, called
    AppConfig. We already use the validate_arguments() decorator, that constructs a signature
    model to validate arguments passed to the app constructor, so this would just be replacing that
    validation with an explicit model.
  • The arguments to the starlite constructor are used to instantiate this AppConfig object.
  • After the configuration object has been instantiated with the parameters passed to Starlite,
    any handlers registered for on_app_config callback receive the AppConfig instance and must
    return it.
  • Config callbacks are passed the AppConfig object in the order that they are passed to
    the Starlite constructor and must define their own behavior and recommendations for where the
    plugin should be inserted into the callback list.
  • Benefit of this is that plugins can be built around the certainty of semantic versioning guarantees of
    the Starlite interface.

Proposed implementation

The implementation is totally backward compatible.

Registered on_app_config hook handlers must receive and return an instance of AppConfig.

The AppConfig object is first instantiated from parameters passed to Starlite.__init__(), and then passed to each of the registered hook handlers.

The resultant AppConfig instance is used to instantiate the application.

Removes the use of pydantic.validate_arguments() on Starlite.__init__() as the instantiation of the AppConfig object does the same thing.

There is a test to ensure that the parameters of the Starlite constructor are matched by properties on AppConfig.

There is a test that ensures all properties on the AppConfig object are accessed within the Starlite constructor.

PR Checklist

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you got 100% test coverage on new code?
  • Have you updated the prose documentation?
  • Have you updated the reference documentation?

@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2022

This pull request introduces 1 alert when merging ed8c247 into c9387f8 - view on LGTM.com

new alerts:

  • 1 for Unused import

starlite/app.py Outdated Show resolved Hide resolved
starlite/app.py Outdated Show resolved Hide resolved
Registered hooks must receive and return an instance of `AppConfig` object.

The `AppConfig` object is first instantiated from parameters passed to `Starlite.__init__()`, and then passed to each of the registered hook handlers.

The resultant `AppConfig` instance is used to instantiate the application.

Removes the use of `pydantic.validate_arguments()` on `Starlite.__init__()` as the instantiation of the `AppConfig` object does the same thing.

Test to ensure that the parameters of the `Starlite` constructor are matched by properties on `AppConfig`.

Test that ensures all properties on the `AppConfig` object are accessed within the `Starlite` constructor.
starlite/app.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Goldziher Goldziher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Please update the docs before merging, and also add attribute comments in the AppConfig class.

@peterschutt peterschutt changed the title RFC: Implements on_app_config hooks. Feature: Implements on_app_init hook Sep 22, 2022
# Conflicts:
#	starlite/app.py
Any parameter that we did something like `thing or []` with inside `Starlite.__init__()` and `Router.__init__()`, I've moved the `thing or []` bit to the `AppConfig` instantiation point.

This makes it nicer to use the `AppConfig` object externally, otherwise need to to the `if None` dance for every collection property on access.
@peterschutt peterschutt marked this pull request as ready for review September 23, 2022 01:58
starlite/app.py Outdated Show resolved Hide resolved
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@sonarcloud
Copy link

sonarcloud bot commented Sep 23, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@peterschutt peterschutt merged commit 4c40053 into main Sep 23, 2022
@peterschutt peterschutt deleted the on-app-config-hook branch September 23, 2022 12:12
Alex-CodeLab pushed a commit to Alex-CodeLab/starlite that referenced this pull request Sep 30, 2022
Implements `on_app_config` hooks.

Registered hooks must receive and return an instance of `AppConfig` object.

The `AppConfig` object is first instantiated from parameters passed to `Starlite.__init__()`, and then passed to each of the registered hook handlers.

The resultant `AppConfig` instance is used to instantiate the application.

Removes the use of `pydantic.validate_arguments()` on `Starlite.__init__()` as the instantiation of the `AppConfig` object does the same thing.

Test to ensure that the parameters of the `Starlite` constructor are matched by properties on `AppConfig`.

Test that ensures all properties on the `AppConfig` object are accessed within the `Starlite` constructor.
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.

None yet

3 participants