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.

Sign up for GitHub

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

Jump to bottom

Retrieve HybridApp images from mobile BFF (proxied to catalog API) #96

Merged
merged 3 commits into from
Nov 21, 2023
Merged

Retrieve HybridApp images from mobile BFF (proxied to catalog API) #96

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9f40ed
Retrieve HybridApp images from web app
Eilon Nov 16, 2023
4a5edb0
Use catalog-api proxy via Mobile BFF to get product images
Eilon Nov 17, 2023
367f3fe
Forward only specific APIs from Mobile BFF to Catalog API
Eilon Nov 21, 2023
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
3 changes: 2 additions & 1 deletion src/HybridApp/Components/Pages/Item/ItemPage.razor
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
@inject CatalogService CatalogService
@* @inject BasketState BasketState *@
@inject NavigationManager Nav
@inject IProductImageUrlProvider ProductImages

@if (item is not null)
{
Expand All @@ -11,7 +12,7 @@
<SectionContent SectionName="page-header-subtitle">@item.CatalogBrand?.Brand</SectionContent>

<div class="item-details">
<img src="@item.PictureUri" />
<img src="@ProductImages.GetProductImageUrl(item.Id)" />
<div class="description">
<p>@item.Description</p>
<p>
Expand Down
11 changes: 7 additions & 4 deletions src/HybridApp/MauiProgram.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
using eShop.WebAppComponents.Services;
using eShop.HybridApp.Services;
using eShop.WebAppComponents.Services;
using Microsoft.Extensions.Logging;

namespace eShop.HybridApp;

public static class MauiProgram
{
// NOTE: Must have a trailing slash to ensure the full BaseAddress URL is used to resolve relative URLs
private static string MobileBffBaseUrl = "http://localhost:61632/catalog-api/";
// NOTE: Must have a trailing slash on base URLs to ensure the full BaseAddress URL is used to resolve relative URLs
private static string MobileBffHost = "http://localhost:61632";
internal static string MobileBffCatalogBaseUrl = $"{MobileBffHost}/catalog-api/";

public static MauiApp CreateMauiApp()
{
Expand All @@ -25,7 +27,8 @@ public static MauiApp CreateMauiApp()
builder.Logging.AddDebug();
#endif

builder.Services.AddHttpClient<CatalogService>(o => o.BaseAddress = new(MobileBffBaseUrl));
builder.Services.AddHttpClient<CatalogService>(o => o.BaseAddress = new(MobileBffCatalogBaseUrl));
builder.Services.AddSingleton<IProductImageUrlProvider, ProductImageUrlProvider>();

return builder.Build();
}
Expand Down
9 changes: 9 additions & 0 deletions src/HybridApp/Services/ProductImageUrlProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
using eShop.WebAppComponents.Services;

namespace eShop.HybridApp.Services;

public class ProductImageUrlProvider : IProductImageUrlProvider
{
public string GetProductImageUrl(int productId)
=> $"{MauiProgram.MobileBffCatalogBaseUrl}api/v1/catalog/items/{productId}/pic";
Copy link
Member Author

Choose a reason for hiding this comment

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

This was even easier than I thought: the Mobile BFF already proxies the entire catalog API, so I just had to use the correct URL, and the image API was already available.

Copy link
Member

@SteveSandersonMS SteveSandersonMS Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This was even easier than I thought: the Mobile BFF already proxies the entire catalog API, so I just had to use the correct URL, and the image API was already available.

Surely it should not do that. The point of Mobile BFF is to publicly expose some API endpoints given that the underlying services are not publicly reachable (and don't want to be publicly reachable, for whatever security/scaleout/etc reasons). If it proxies everything on CatalogAPI, there's no point even having a Mobile BFF and the app owner might as well set up a public HTTP ingress for CatalogAPI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think right now it seems a bit odd because the Hybrid App is a read-only view of only the catalog and is not yet a full experience. In the future it would likely need to proxy multiple different services (cart, orders, etc.), plus also have some bespoke mobile-specific APIs (maybe tracking some app preferences/settings, usage insights/telemetry, etc.). But the whole HybridApp is really just 2 pages now (catalog list + item details), so the pattern isn't apparent.

It's probably reasonable to argue that instead it should not proxy any entire service, and instead only specific APIs the app needs.

If you or anyone has thoughts on this I'd be happy to adjust to the pattern. I'm not quite sure what principles we are applying here, so I'd like to follow whatever we recommend.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably reasonable to argue that instead it should not proxy any entire service, and instead only specific APIs the app needs.

I would agree. I thought part of the role of BFF is to act as a trust boundary (or at least an additional boundary, not necessarily the only one). Some microservice teams will build their services in a zero-trust way assuming that all callers may be hostile, passing auth tokens all the way through the chain, etc., but also many microservice teams will assume that they only receive calls from trusted services. Proxying from public HTTP ingress to an internal service would then be... problematic. We should be wary about doing that in a sample that people may treat as best-practice recommendation.

cc @adityamandaleeka @davidfowl for any opinions about whether to change this aspect of MobileBFF. Ultimately this doesn't change my PR but we do want to get in @Eilon's fix at the same time otherwise things would be broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give this a try. Given how simple the app is, it's probably not that hard to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, it wasn't entirely trivial, but also not too complicated. It's a bit ugly, though. The challenge is that I had to add a specific mapping for each catalog-api that the mobile-bff needed to expose to the CatalogService (which is shared between WebApp and HybridApp). I kept the exact same API pattern that catalog-api already had because it means we can share the same CatalogService implementation. That's what a bit yucky: the way we have CatalogService right now requires that the exact same catalog APIs are exposed in exactly the same way (except the URL base address). Otherwise we'd need an ICatalogService and two different implementations, which would also be a bit yucky.

Copy link
Member

Choose a reason for hiding this comment

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

That's great - this looks very good now! I don't even think it's ugly TBH - it feels correct to specify each endpoint that you want to expose publicly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Beauty is in the eye of the BFF.

}