Retrieve HybridApp images from mobile BFF (proxied to catalog API) #96
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| public class ProductImageUrlProvider : IProductImageUrlProvider | ||
| { | ||
| public string GetProductImageUrl(int productId) | ||
| => $"{MauiProgram.MobileBffCatalogBaseUrl}api/v1/catalog/items/{productId}/pic"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll give this a try. Given how simple the app is, it's probably not that hard to do.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Beauty is in the eye of the BFF.
|
@SteveSandersonMS the PR is updated with your suggestion. |
Eilon
changed the title
| @@ -2,10 +2,7 @@ | |||
| { | |||
| public static void AddApplicationServices(this IHostApplicationBuilder builder) | |||
| { | |||
| builder.Services | |||
| .AddReverseProxy() | |||
There was a problem hiding this comment.
This change removes the free-for-all reverse proxy and instead maps only specific URL patterns (see Program.cs below), and forwards those as needed.
|
@SteveSandersonMS - because this is a PR to your PR, please merge at your convenience so I don't accidentally trounce any work you might have pending. |
@SteveSandersonMS - here's a PR to your PR to have the .NET MAUI Hybrid App retrieve images from the mobile BFF.
This change follows a pattern similar to what the web app does. In this case the HybridApp talks to the Mobile BFF, which then proxies to the Catalog API, which has the product images.