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

[Core][Hosting] Resolve handler services through registered service types #20298

Merged
merged 10 commits into from
Feb 20, 2024

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Feb 1, 2024

Description of Change

Reflection techniques used in MAUI's dynamic resolution of a virtual view's handler are neither AOT friendly nor trimming friendly. It involves iterating through the view's base types (GetServiceBaseTypes) and checking if any registered handler service types match (GetServiceDescriptor).

GetServiceBaseTypes(Type serviceType)
{
	var types = new List<Type> { serviceType };

	Type? baseType = serviceType.BaseType;

	while (baseType != null)
	{
		types.Add(baseType);
		baseType = baseType.BaseType;
	}

	foreach (var interfac in serviceType.GetInterfaces()) // Not AOT friendly, requires trimming annotations
	{
		if (typeof(IView).IsAssignableFrom(interfac))
			types.Add(interfac);
	}

	return types;
}
GetServiceDescriptor(Type serviceType)
{
	if (serviceType == null)
		throw new ArgumentNullException(nameof(serviceType));

	var types = GetServiceBaseTypes(serviceType);

	foreach (var type in types)
	{
		if (_collection.TryGetService(type, out var descriptor) && descriptor != null)
			return descriptor;
	}

	return null;
}

Instead of using reflection to resolve a virtual view's corresponding registered handler, this PR takes inspiration from #20058 and leverages a set of registered handler service types to find a virtual view's corresponding handler.

With the addition of ImageSource service type mappings and Handler service type set, MauiFactory's AOT unfriendly and trimming unsafe logic is no longer needed. This PR opts to simplify MauiFactory, which may be substituted out for the default ServiceProvider in another PR.

This PR does the following:

  • Modifies MauiHandlersFactory service registration to emulate ImageSourceServiceProvider service registration
  • Introduces a RegisteredHandlerServiceTypeSet to keep track of handler service types registered through AddHandler and to resolve a virtual view's corresponding handler service type
  • Cleans up existing tests and adds new tests
  • Simplifies MauiFactory and removes AOT unfriendly + trimming unsafe logic
  • Removes MauiFactory build warnings from BuildWarningsUtilities test

Issues Fixed

Contributes to #19397
Fixes #1298 tested via HostBuilderResolvesToHandlerRegisteredUnderMostDerivedBaseInterfaceType

@samhouts samhouts added the area-core-hosting Extensions / Hosting / AppBuilder / Startup label Feb 2, 2024
@mdh1418 mdh1418 force-pushed the add_handler_service_type_resolver branch from bb78a7a to d31eb32 Compare February 2, 2024 01:30
Copy link
Member

@simonrozsival simonrozsival left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

src/Core/src/Hosting/MauiHandlersCollectionExtensions.cs Outdated Show resolved Hide resolved
src/Core/src/Hosting/MauiHandlersCollectionExtensions.cs Outdated Show resolved Hide resolved
@@ -12,5 +12,7 @@ public ElementHandlerStub() : base(ElementHandler.ElementMapper)
}

protected override object CreatePlatformElement() => new Object();

public override void SetVirtualView(IElement view) => base.SetVirtualView(view);
Copy link
Member

Choose a reason for hiding this comment

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

Is this override necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there is an exception propagation test

[Fact]
public void ExceptionPropagatesFromHandler()
{
var mauiApp = MauiApp.CreateBuilder()
.ConfigureMauiHandlers(handlers => handlers.AddHandler<IViewStub, ViewHandlerWithChildException>())
.ConfigureMauiHandlers(handlers => handlers.AddHandler<ExceptionThrowingStub, ExceptionThrowingViewHandler>())
.Build();
var mauiContext = new MauiContext(mauiApp.Services);
Assert.Throws<PlatformViewStubCreatePlatformViewException>(() =>
new ViewStub().ToPlatform(mauiContext)
);
}
class ViewHandlerWithChildException : ViewHandlerStub
{
public ViewHandlerWithChildException()
{
}
protected override PlatformViewStub CreatePlatformView()
{
return new PlatformViewStub();
}
public override void SetVirtualView(IView view)
{
base.SetVirtualView(view);
new ExceptionThrowingStub().ToPlatform(MauiContext);
}
}
class ExceptionThrowingStub : ViewStub
{
}
class ExceptionThrowingViewHandler : ViewHandlerStub
{
public ExceptionThrowingViewHandler()
{
}
protected override PlatformViewStub CreatePlatformView()
{
throw new PlatformViewStubCreatePlatformViewException();
}
}
class PlatformViewStubCreatePlatformViewException : Exception
{
}

that tests whether a view/element can throw an exception for another view/element.

I originally moved all HandlerTests to HostBuilderHandlerTests because I didn't see why we really needed the separation, both were in the same testing namespace anyways and both used HostBuilding (MauiApp.CreateBuilder().Build)

Then I figured that the test didn't fit in either, as it wasn't testing the HostBuilder's ability to register and retrieve services, rather it would be an ElementTest, so now its there as ElementToHandlerPropagatesThrownException.

The reason ElementToHandlerPropagatesThrownException needs that declared override void SetVirtualView from what I'm guessing because I don't fully understand the intent of the original test, was to give it some easy access method to throw an exception in the nested view/element after grabbing the view/element's handler in the ToHandler extensions method

handler = context.Handlers.GetHandler(viewType);
}
catch (MissingMethodException)
{
handler = viewType.CreateTypeWithInjection(context);
if (handler != null)
handlersWithConstructors.Add(view.GetType());
}
}
if (handler == null)
throw new HandlerNotFoundException(view);
handler.SetMauiContext(context);
view.Handler = handler;
if (handler.VirtualView != view)
handler.SetVirtualView(view);
. So once the view/element (ViewWithExceptionThrowingChildStub) grabbed the nested child view/element which throws the exception (ExceptionThrowingViewStub) we just check that the exception propagates through. Originally it was using CreatePlatformView to throw the exception but I figured if we just wanted to propagate an exception being thrown, using SetVirtualView would achieve the same results. But moving from UnitTests to DeviceTests meant that different stubs were available ViewStubHandler vs ElementStubHandler, but I couldn't override SetVirtualView without adding this to the ElementHandlerStub. Maybe I was missing something simple?

So the propagation pattern now looks like
ViewWithExceptionThrowingChildStub().ToHandler -> GetHandler -> ViewWithExceptionThrowingChildHandler.SetVirtualView -> ExceptionThrowingViewStub().ToHandler -> GetHandler + ExceptionThrowingViewHandler.SetVirtualView -> throw HandlerPropagatesException.
Compared to the original test that had
ViewStub().ToHandler -> GetHandler -> ViewHandlerWithChildException.SetVirtualView -> ExceptionThrowingStub().ToHandler->GetHandler->ExceptionThrowingViewHandler.SetVirtualView->ExceptionThrowingViewHandler.CreatePlatformView-> throwPlatformViewStubCreatePlatformViewException`

@PureWeen do you happen to know if I'm on the right track for the original intent of the test?

@mdh1418 mdh1418 force-pushed the add_handler_service_type_resolver branch from d31eb32 to b4a30ec Compare February 2, 2024 17:10
@mdh1418 mdh1418 marked this pull request as ready for review February 5, 2024 17:12
@mdh1418 mdh1418 requested a review from a team as a code owner February 5, 2024 17:12
@mdh1418 mdh1418 added the do-not-merge Don't merge this PR label Feb 5, 2024
@mdh1418 mdh1418 force-pushed the add_handler_service_type_resolver branch 2 times, most recently from 23cfb82 to c8da981 Compare February 8, 2024 20:59
@rmarinho
Copy link
Member

/rebase

@mdh1418 mdh1418 force-pushed the add_handler_service_type_resolver branch from c8da981 to 7b1e9c5 Compare February 12, 2024 14:46
@mdh1418 mdh1418 removed the do-not-merge Don't merge this PR label Feb 12, 2024
simonrozsival
simonrozsival previously approved these changes Feb 12, 2024
@rmarinho rmarinho merged commit e2245e0 into dotnet:net9.0 Feb 20, 2024
42 of 47 checks passed
@mdh1418 mdh1418 deleted the add_handler_service_type_resolver branch February 20, 2024 16:43
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-core-hosting Extensions / Hosting / AppBuilder / Startup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants