Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Makes android embedding to send full uri #41836

Merged
merged 3 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -525,16 +525,7 @@ private String maybeGetInitialRouteFromIntent(Intent intent) {
if (host.shouldHandleDeeplinking()) {
Uri data = intent.getData();
if (data != null) {
String fullRoute = data.getPath();
if (fullRoute != null && !fullRoute.isEmpty()) {
if (data.getQuery() != null && !data.getQuery().isEmpty()) {
fullRoute += "?" + data.getQuery();
}
if (data.getFragment() != null && !data.getFragment().isEmpty()) {
fullRoute += "#" + data.getFragment();
}
return fullRoute;
}
return data.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

In general you should not rely on the behavior of tostring in production code. https://www.bekk.christmas/post/2019/4/never-use-tostring()-for-behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find a better way to convert uri to a serializable representation. all the suggestions i found is to use to string

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case it's probably ok, since Uri provides both a toString and a parse method that knows how to deserialize from the string produced by toString...

}
}
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ public void flutterEngineGroupGetsInitialRouteFromIntent() {
ArgumentCaptor<FlutterEngineGroup.Options> optionsCaptor =
ArgumentCaptor.forClass(FlutterEngineGroup.Options.class);
verify(flutterEngineGroup, times(1)).createAndRunEngine(optionsCaptor.capture());
assertEquals("/initial_route", optionsCaptor.getValue().getInitialRoute());
assertEquals("foo://example.com/initial_route", optionsCaptor.getValue().getInitialRoute());
Copy link
Contributor

Choose a reason for hiding this comment

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

When these strings to go the framework, are they handled as dart Uri objects? If so this is probably fine, but if not won't this cause breakages in framework apps?

Even if so, won't this cause breakages to people using toString on the Uri?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes the framework is treated it as uri object after flutter/flutter#119968

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if so, won't this cause breakages to people using toString on the Uri?

I am not sure if i understand, can you add more details?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see now, thanks for the link ot the framework PR.

}

@Test
Expand Down Expand Up @@ -688,7 +688,7 @@ public void itForwardsOnRequestPermissionsResultToFlutterEngine() {

// Verify that the navigation channel was given the initial route message.
verify(mockFlutterEngine.getNavigationChannel(), times(1))
.setInitialRoute("/custom/route?query=test");
.setInitialRoute("http://myApp/custom/route?query=test");
}

@Test
Expand Down Expand Up @@ -716,7 +716,7 @@ public void itForwardsOnRequestPermissionsResultToFlutterEngine() {

// Verify that the navigation channel was given the initial route message.
verify(mockFlutterEngine.getNavigationChannel(), times(1))
.setInitialRoute("/custom/route?query=test#fragment");
.setInitialRoute("http://myApp/custom/route?query=test#fragment");
}

@Test
Expand Down Expand Up @@ -744,7 +744,7 @@ public void itForwardsOnRequestPermissionsResultToFlutterEngine() {

// Verify that the navigation channel was given the initial route message.
verify(mockFlutterEngine.getNavigationChannel(), times(1))
.setInitialRoute("/custom/route#fragment");
.setInitialRoute("http://myApp/custom/route#fragment");
}

@Test
Expand All @@ -771,7 +771,8 @@ public void itForwardsOnRequestPermissionsResultToFlutterEngine() {
delegate.onStart();

// Verify that the navigation channel was given the initial route message.
verify(mockFlutterEngine.getNavigationChannel(), times(1)).setInitialRoute("/custom/route");
verify(mockFlutterEngine.getNavigationChannel(), times(1))
.setInitialRoute("http://myApp/custom/route");
}

@Test
Expand Down Expand Up @@ -809,19 +810,19 @@ public void itSendsPushRouteInformationMessageWhenOnNewIntent() {
// --- Execute the behavior under test ---
// The FlutterEngine is set up in onAttach().
delegate.onAttach(ctx);
String expected = "http://myApp/custom/route?query=test";

Intent mockIntent = mock(Intent.class);
when(mockIntent.getData()).thenReturn(Uri.parse("http://myApp/custom/route?query=test"));
when(mockIntent.getData()).thenReturn(Uri.parse(expected));
// Emulate the host and call the method that we expect to be forwarded.
delegate.onNewIntent(mockIntent);

// Verify that the navigation channel was given the push route message.
verify(mockFlutterEngine.getNavigationChannel(), times(1))
.pushRouteInformation("/custom/route?query=test");
verify(mockFlutterEngine.getNavigationChannel(), times(1)).pushRouteInformation(expected);
}

@Test
public void itDoesNotSendPushRouteInformationMessageWhenOnNewIntentIsNonHierarchicalUri() {
public void itDoesSendPushRouteInformationMessageWhenOnNewIntentIsNonHierarchicalUri() {
when(mockHost.shouldHandleDeeplinking()).thenReturn(true);
// Create the real object that we're testing.
FlutterActivityAndFragmentDelegate delegate = new FlutterActivityAndFragmentDelegate(mockHost);
Expand All @@ -839,7 +840,7 @@ public void itDoesNotSendPushRouteInformationMessageWhenOnNewIntentIsNonHierarch
delegate.onNewIntent(mockIntent);

// Verify that the navigation channel was not given a push route message.
verify(mockFlutterEngine.getNavigationChannel(), times(0))
verify(mockFlutterEngine.getNavigationChannel(), times(1))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was added #29766

Which seems like a bug to me, it could have send the non hierarchical uri.

Copy link
Contributor

Choose a reason for hiding this comment

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

The linked patch was trying to avoid a crash. This assertion is probably unnecessary.

However, we should update the comment and have some kind of assertion about what arguments actually got sent through.

.pushRouteInformation("mailto:[email protected]");
}

Expand All @@ -852,16 +853,15 @@ public void itSendsPushRouteInformationMessageWhenOnNewIntentWithQueryParameterA
// --- Execute the behavior under test ---
// The FlutterEngine is set up in onAttach().
delegate.onAttach(ctx);
String expected = "http://myApp/custom/route?query=test#fragment";

Intent mockIntent = mock(Intent.class);
when(mockIntent.getData())
.thenReturn(Uri.parse("http://myApp/custom/route?query=test#fragment"));
when(mockIntent.getData()).thenReturn(Uri.parse(expected));
// Emulate the host and call the method that we expect to be forwarded.
delegate.onNewIntent(mockIntent);

// Verify that the navigation channel was given the push route message.
verify(mockFlutterEngine.getNavigationChannel(), times(1))
.pushRouteInformation("/custom/route?query=test#fragment");
verify(mockFlutterEngine.getNavigationChannel(), times(1)).pushRouteInformation(expected);
}

@Test
Expand All @@ -873,15 +873,15 @@ public void itSendsPushRouteInformationMessageWhenOnNewIntentWithFragmentNoQuery
// --- Execute the behavior under test ---
// The FlutterEngine is set up in onAttach().
delegate.onAttach(ctx);
String expected = "http://myApp/custom/route#fragment";

Intent mockIntent = mock(Intent.class);
when(mockIntent.getData()).thenReturn(Uri.parse("http://myApp/custom/route#fragment"));
when(mockIntent.getData()).thenReturn(Uri.parse(expected));
// Emulate the host and call the method that we expect to be forwarded.
delegate.onNewIntent(mockIntent);

// Verify that the navigation channel was given the push route message.
verify(mockFlutterEngine.getNavigationChannel(), times(1))
.pushRouteInformation("/custom/route#fragment");
verify(mockFlutterEngine.getNavigationChannel(), times(1)).pushRouteInformation(expected);
}

@Test
Expand All @@ -893,15 +893,15 @@ public void itSendsPushRouteInformationMessageWhenOnNewIntentNoQueryParameter()
// --- Execute the behavior under test ---
// The FlutterEngine is set up in onAttach().
delegate.onAttach(ctx);
String expected = "http://myApp/custom/route#fragment";

Intent mockIntent = mock(Intent.class);
when(mockIntent.getData()).thenReturn(Uri.parse("http://myApp/custom/route"));
when(mockIntent.getData()).thenReturn(Uri.parse(expected));
// Emulate the host and call the method that we expect to be forwarded.
delegate.onNewIntent(mockIntent);

// Verify that the navigation channel was given the push route message.
verify(mockFlutterEngine.getNavigationChannel(), times(1))
.pushRouteInformation("/custom/route");
verify(mockFlutterEngine.getNavigationChannel(), times(1)).pushRouteInformation(expected);
}

@Test
Expand Down