enforce event to topic map if the endpoint specifies #1242
Conversation
PhilBastian
requested review from
danielmarbach,
jpalac,
warwickschroeder and
abparticular
There was a problem hiding this comment.
I think the option is misplaced. See inline comment
// cc @SzymonPobiega
| /// <summary> | ||
| /// Determines if an exception should be thrown when attempting to publish an event not mapped in PublishedEventToTopicsMap | ||
| /// </summary> | ||
| public bool ThrowIfUnmappedEventTypes |
There was a problem hiding this comment.
I don't think this property belongs here. This is a behavior that is only relevant to opt in for the TopicPerEventTopology. The migration topology already throws when types are unmapped. If you put it here, one could argue the migration topology would have to respect this flag too, which I think should never be the case by design.
There was a problem hiding this comment.
discussed in call: only have it on options and explicitly force migrationoptions version to true
| /// <summary> | ||
| /// Determines if an exception should be thrown when attempting to publish an event not mapped in PublishedEventToTopicsMap | ||
| /// </summary> | ||
| public bool ThrowIfUnmappedEventTypes { get; set; } = false; |
There was a problem hiding this comment.
Same comment as above. I think this is the time when we need to introduce the TopicPerEventTopologyOptions again (we killed the type because it was empty at the time and said we could always reintroduce it later).
There was a problem hiding this comment.
Alternative name suggestions: ThrowForUnmappedEventTypes
There was a problem hiding this comment.
discussed in call: having a new subclass of TopologyOptions which now becomes the 'true' representation of options for TopicPerEvent topology introduces potential breaking changes and difficulty in obsoleting which aren't worth this change. Instead we are forcing this property to true for MigrationTopologyOptions since that's the expected behaviour there
Implements #1201