Allow to dynamically enrich events metadata by using with_metadata
#327
Conversation
This file contains 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
paneq
reviewed
Apr 30, 2018
ruby_event_store/CHANGELOG.md
Outdated
| @@ -1,5 +1,10 @@ | |||
| Further changes can be tracked at [releases page](https://github.com/RailsEventStore/rails_event_store/releases). | |||
|
|
|||
| ### 0.15.0 (not released yet) | |||
There was a problem hiding this comment.
We've moved changelog to https://github.com/RailsEventStore/rails_event_store/releases as the 1st line says :)
paneq
reviewed
Apr 30, 2018
| if metadata_proc | ||
| warn "`RubyEventStore::Client#metadata_proc` has been deprecated. Use `RubyEventStore::Client#with_metadata` instead." |
There was a problem hiding this comment.
I am not sure we want to deprecate it... Let me think/check.
There was a problem hiding this comment.
hmm too compilicated for quick review:
paneq
reviewed
Apr 30, 2018
| @@ -171,6 +171,16 @@ def within(&block) | |||
| Within.new(block, event_broker) | |||
| end | |||
|
|
|||
| def with_metadata(metadata, &block) | |||
| previous_metadata = self.metadata | |||
| begin | |||
There was a problem hiding this comment.
i think we could use just ensure without begin
There was a problem hiding this comment.
yes, you're right
jakubkosinski
force-pushed
the
with-metadata
branch
from
d78f61c to
2829521
Compare
Instead of using metadata_proc when initializing RubyEventStore::Client one can now use with_context instance method to dynamically set the event metadata for all events published inside a block, e.g. ``` event_store.with_metadata(request_ip: '127.0.0.1', correlation_id: '1234567890') do event_store.publish(event) end ``` `with_metadata` calls can be nested but the passed metadata will not be merged with one from the upper level. This allows to clear the metadata set in the upper level by calling `with_metadata(nil)` - in this case only default timestamp will be added to published events metadata
Instead of setting thread local variable, use with_metadata method from the event_store instance taken from the app configuration. If `config.event_store` is not set, no action is performed. Custom request metadata proc is still taken from `config.x.rails_event_store.request_metadata` If that is not present, the default proc is called
[ci skip]
It looks like the @app in the middleware is not an instance of `Rails::Application` so we cannot just read config from there. But we can use the Rails global and read configuration from there
Store metadata in thread local variable that has unique name for each instance so that you can nest `RubyEventStore::Client#with_metadata` calls of different client instances without overwriting the metadata
Simplify middleware so that it only calls the app in Client#with_request_metadata block if config.event_store is set You can now set the custom request_metadata proc in `RailsEventStore::Client`, the default proc returns the remote IP & request ID from ActionDispatch::Request created from the env
[ci skip]
When with_metadata calls are nested, merge the metadata instead of overwriting it
jakubkosinski
force-pushed
the
with-metadata
branch
from
9ee2174 to
30866ab
Compare
| request = ::Rack::MockRequest.new(::Rack::Lint.new(Middleware.new( | ||
| ->(env) { [200, {}, ['Hello World']] }, | ||
| ->(env) { { kaka: 'dudu' } } ))) | ||
| specify 'calls app within with_metadata block when app has configured the event store instance' do |
There was a problem hiding this comment.
Isn't that a bit too much of testing implementation details, something which one would call klasotesty?
| require 'rails_event_store/middleware' | ||
| require 'rack/lint' | ||
|
|
||
| module RailsEventStore | ||
| RSpec.describe Middleware do | ||
| specify 'lint' do | ||
| request = ::Rack::MockRequest.new(::Rack::Lint.new(Middleware.new( |
There was a problem hiding this comment.
Why have you decided to removeRack::Lint that verifies Middleware in accordance to Rack specification?
| @@ -126,17 +133,142 @@ module RubyEventStore | |||
| end | |||
|
|
|||
| specify 'published event metadata will be enriched by proc execution' do | |||
| client = RubyEventStore::Client.new(repository: InMemoryRepository.new, metadata_proc: ->{ {request_id: '127.0.0.1'} }) | |||
| client = silence_warnings { RubyEventStore::Client.new(repository: InMemoryRepository.new, metadata_proc: ->{ {request_id: '127.0.0.1'} }) } | |||
There was a problem hiding this comment.
Is this silence_warnings necessary here with output(deprecation_warning).to_stderr above?
|
Looks good, the questions remain open and can be addressed async -- as usual. |
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.
#325
Instead of using metadata_proc when initializing RubyEventStore::Client one can now
use with_metadata instance method to dynamically set the event metadata for all events
published inside a block, e.g.
with_metadatacalls can be nested but the passed metadata will not be merged with one fromthe upper level. This allows to clear the metadata set in the upper level by calling
with_metadata(nil)