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

Do not auto discover generic event handlers #17465

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

PMExtra
Copy link
Contributor

@PMExtra PMExtra commented Aug 24, 2023

Description

Adding a GenericTypeDefinition (a generic type without arguments) to the AbpLocalEventBusOptions.Handlers or AbpDistributedEventBusOptions.Handlers makes no sense.

It cannot be determined which event should be subscribed.

And it makes KafkaDistributedEventBus broken:

Volo.Abp.AbpInitializationException: An error occurred during the initialize Volo.Abp.Modularity.OnApplicationInitializationModuleLifecycleContributor phase of the module Volo.Abp.EventBus.Kafka.AbpEventBusKafkaModule, Volo.Abp.EventBus.Kafka, Version=7.2.1.0, Culture=neutral, PublicKeyToken=null: Value cannot be null. (Parameter 'key'). See the inner exception for details.
 ---> System.ArgumentNullException: Value cannot be null. (Parameter 'key')
   at System.ThrowHelper.ThrowArgumentNullException(String name)
   at System.Collections.Concurrent.ConcurrentDictionary`2.set_Item(TKey key, TValue value)
   at Volo.Abp.EventBus.Kafka.KafkaDistributedEventBus.<GetOrCreateHandlerFactories>b__38_0(Type type)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Volo.Abp.EventBus.Kafka.KafkaDistributedEventBus.GetOrCreateHandlerFactories(Type eventType)
   at Volo.Abp.EventBus.Kafka.KafkaDistributedEventBus.Subscribe(Type eventType, IEventHandlerFactory factory)
   at Volo.Abp.EventBus.EventBusBase.SubscribeHandlers(ITypeList`1 handlers)
   at Volo.Abp.EventBus.Kafka.KafkaDistributedEventBus.Initialize()
   at Volo.Abp.EventBus.Kafka.AbpEventBusKafkaModule.OnApplicationInitialization(ApplicationInitializationContext context)
   at Volo.Abp.Modularity.AbpModule.OnApplicationInitializationAsync(ApplicationInitializationContext context)
   at Volo.Abp.Modularity.OnApplicationInitializationModuleLifecycleContributor.InitializeAsync(ApplicationInitializationContext context, IAbpModule module)
   at Volo.Abp.Modularity.ModuleManager.InitializeModulesAsync(ApplicationInitializationContext context)

Checklist

  • I fully tested it as developer / designer and created unit / integration tests
  • I documented it (or no need to document or I will create a separate documentation issue)

How to test it?

Take the MyGenericDistributedEventHandler class to your tests with Kafka Event Bus, then register it to your service collection like EventBusTestModule.

It would be broken without this patch.

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.29%. Comparing base (72d69d3) to head (e1e0342).
Report is 2959 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #17465      +/-   ##
==========================================
- Coverage   53.34%   53.29%   -0.05%     
==========================================
  Files        3024     3024              
  Lines       94357    94362       +5     
==========================================
- Hits        50334    50292      -42     
- Misses      44023    44070      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maliming maliming self-requested a review August 24, 2023 08:29
@maliming
Copy link
Member

Is there any actual reason to use such a class?

public class MyGenericDistributedEventHandler<TEvent> : IDistributedEventHandler<TEvent>

@PMExtra
Copy link
Contributor Author

PMExtra commented Aug 24, 2023

Yes, we have an options to set which events should be handled by this handler.

Copy link

stale bot commented Dec 15, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive label Dec 15, 2023
@PMExtra
Copy link
Contributor Author

PMExtra commented Dec 15, 2023

This issue is still unresolved. Waiting for the staff's response.

@stale stale bot removed the inactive label Dec 15, 2023
Copy link

stale bot commented Mar 17, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive label Mar 17, 2024
@PMExtra
Copy link
Contributor Author

PMExtra commented Mar 18, 2024

@maliming

@stale stale bot removed the inactive label Mar 18, 2024
@maliming
Copy link
Member

@PMExtra
Copy link
Contributor Author

PMExtra commented Mar 19, 2024

@maliming 非要在单元测试里复现的话,那需要ABP先提供一个KafkaEventBus的单元测试

@maliming maliming requested review from realLiangshiwei and removed request for maliming March 19, 2024 05:39
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