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

[question] domain pollution #106

Open
SpareShade opened this issue Jul 19, 2022 · 5 comments
Open

[question] domain pollution #106

SpareShade opened this issue Jul 19, 2022 · 5 comments

Comments

@SpareShade
Copy link

Hello @vardius

Thank you for sharing this library, it is very helpful in many regards!

My question is about your choice of appending the metadata in the aggregate itself. Eg.

func (u *User) trackChange(ctx context.Context, event *domain.Event) error {
	var meta domain.EventMetadata
	if i, hasIdentity := identity.FromContext(ctx); hasIdentity {
		meta.Identity = i
	}
	if m, ok := metadata.FromContext(ctx); ok {
		meta.IPAddress = m.IPAddress
		meta.UserAgent = m.UserAgent
		meta.Referer = m.Referer
	}
	if !meta.IsEmpty() {
		event.WithMetadata(&meta)
	}

	u.changes = append(u.changes, event)
	u.version++

	return nil
}

Seeing the UserAgent or IP exposed in the aggregate feels a bit like pollution of a domain with application/infra data.
I understand that the aggregate essentially proxies this data from the command handler, which usually is acceptable practice, as aggregate is not making use of that data.

I am wondering if you would not mind sharing your thoughts as to why you chose to enrich the event in the aggregate as opposed to eg. in the aggregate repository implementation.

Thanks again for sharing!

@vardius
Copy link
Owner

vardius commented Jul 20, 2022

The reason why I did that is because with the message bus I am using the context is not carried over

@SpareShade
Copy link
Author

Thank you for getting back on this question.

But aren't the events handled first by the repository, which then publishes them to the event bus?
The repository should still have access to the request context, or is this not so.

// Save current user changes to event store and publish each event with an event bus
func (r *userRepository) Save(ctx context.Context, u user.User) error {
	if err := r.eventStore.Store(ctx, u.Changes()); err != nil {
		return apperrors.Wrap(err)
	}

	for _, event := range u.Changes() {
		if err := r.eventBus.Publish(ctx, event); err != nil {
			return apperrors.Wrap(err)
		}
	}

	return nil
}

I don't mean to nitpick. I'm just drawing a lot from your code (very nicely written too), and want to see whether there are potential pitfalls in my implementation/thinking. And of course, it's best to think together :)

@vardius
Copy link
Owner

vardius commented Jul 22, 2022

yes you are right! i suppose i have missed that. i agree it would feel much better in the repository instead of having this in the domain. would you like to contribute and move it yourself? i would happily merge your pr :) and it feels like an easy change

@SpareShade
Copy link
Author

Yes, I can absolutely, glad you agree :).

I am tied up atm, but should have a gap next weekend, it won't be too long in any case :)

@vardius
Copy link
Owner

vardius commented Jul 22, 2022

ok cool, will w8 ;)

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

No branches or pull requests

2 participants