Skip to content

Make Container hooks more useful for dynamic registration#274

Merged
timriley merged 3 commits intodry-rb:mainfrom
alassek:hooks
Mar 9, 2025
Merged

Make Container hooks more useful for dynamic registration#274
timriley merged 3 commits intodry-rb:mainfrom
alassek:hooks

Conversation

@alassek
Copy link
Copy Markdown
Contributor

@alassek alassek commented Jun 15, 2024

This is a (long-overdue) followup PR to our conversation related to integrating APM into a Dry-System app.

Also relates to hanami/hanami#1360

Use Case: I want to be able to dynamically decorate keys as they are added into the container system in order to perform APM metrics without having to touch every file.

Use Case: I want to audit the usage of particular namespaces in the Container using Dry-Monitor decoration, such as producing an audit log of Authorization checks.

In these use-cases, it's important that I want to react to container registration without having knowledge of precisely what keys are being registered. The decorate feature works well when keys are known in advance, but otherwise you have to enumerate the keys in the container after registration.

Enumeration is a problem, because after_finalize is called after the container is frozen, which is too late to decorate key values.

This PR makes two changes:

  1. Establish an after_register hook that fires whenever a key is registered
  2. Move after_finalize to happen prior to freezing the container.

This allows us to either decorate keys dynamically as they are registered, and to enumerate the container keys before the container is frozen in case we want to make alterations.

Due to the complexity of Container value storage between memoization and callable, I opted not to expose the key value directly; the hook code may choose to call resolve if it needs that.

Allows you to take action when a new key is registered, so that you can
decorate the key without knowing its name ahead of time.

See also hanami/hanami#1360
This hook is much less useful that it might be if you could still make
changes to the container when it ran.
Copy link
Copy Markdown
Member

@timriley timriley left a comment

Choose a reason for hiding this comment

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

These changes look good to me! @flash-gordon — did you want to take a look at all?

Due to the complexity of Container value storage between memoization and callable, I opted not to expose the key value directly; the hook code may choose to call resolve if it needs that.

This is a smart decision, and I agree.

@timriley
Copy link
Copy Markdown
Member

@alassek Do you think you might be able to make a "minimum viable docs" page for container hooks as part of this PR? This is a powerful feature of dry-system that currently has no coverage in docs at all.

@alassek
Copy link
Copy Markdown
Contributor Author

alassek commented Feb 26, 2025

@timriley Good point, I'll add some.

@alassek
Copy link
Copy Markdown
Contributor Author

alassek commented Mar 2, 2025

@timriley I've added a docsite page for container hooks. What do you think?

I was unable to think of good code examples for configure and finalize hooks; if you have any suggestions for what one might want to use them for I could flesh that out.

@flash-gordon
Copy link
Copy Markdown
Member

@timriley I've read the PR and the original discussion, looks good to me

@alassek alassek requested a review from timriley March 7, 2025 01:21
@timriley
Copy link
Copy Markdown
Member

timriley commented Mar 9, 2025

@alassek I'm very happy with those docs! At this stage I don't think we need to block on perfect examples before getting these docs out, so I reckon we can move ahead.

I'll merge this now and have made myself a note to publish a new minor release next week.

@timriley timriley merged commit dd4ad2f into dry-rb:main Mar 9, 2025
7 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Hanami 2.2 Mar 9, 2025
@alassek alassek deleted the hooks branch March 10, 2025 17:25
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.

3 participants