refactor(graph): use BaseGraphService.publicBaseURL consistently

drive.WebUrl, driveItem.WebUrl and the public share link WebUrl all
derive from the same config value (graph.spaces.webdav_base), but only
driveItem.WebUrl used the pre-parsed BaseGraphService.publicBaseURL.
The other two re-parsed the config on every call.

Add webURLForResource and webURLForPublicShareToken methods on
BaseGraphService, and convert the cs3ResourceToDriveItem and
formatDriveItems free functions into methods so they pick up logger
and publicBaseURL from the receiver. This also aligns them with the
surrounding code: BaseGraphService already exposes ~15 similar methods,
so the two free functions were the odd ones out.

Net: all three WebUrls are now constructed from a single pre-parsed
URL, and the (g.logger, g.publicBaseURL) plumbing at 7 call sites
disappears.

Author: dschmidt

services/graph/pkg/service/v0/api_driveitem_permissions.go

@@ -407,7 +407,7 @@ func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID
 
 	driveItems := make(driveItemsByResourceID, 1)
 	// we can use the statResponse to build the drive item before fetching the shares
-	item, err := cs3ResourceToDriveItem(s.logger, s.publicBaseURL, statResponse.GetInfo())
+	item, err := s.cs3ResourceToDriveItem(statResponse.GetInfo())
 	if err != nil {
 		return collectionOfPermissions, err
 	}

services/graph/pkg/service/v0/base.go

@@ -52,6 +52,24 @@ type BaseGraphService struct {
 	publicBaseURL   *url.URL
 }
 
+// webURLForResource returns the public web URL pointing at the given resource
+// (e.g. https://cloud.example.com/f/<resource-id>), using the pre-parsed
+// publicBaseURL held by the service.
+func (g BaseGraphService) webURLForResource(rid *storageprovider.ResourceId) *string {
+	u := *g.publicBaseURL
+	u.Path = path.Join(u.Path, "f", storagespace.FormatResourceID(rid))
+	return libregraph.PtrString(u.String())
+}
+
+// webURLForPublicShareToken returns the public web URL for a public share link
+// (e.g. https://cloud.example.com/s/<token>), using the pre-parsed
+// publicBaseURL held by the service.
+func (g BaseGraphService) webURLForPublicShareToken(token string) *string {
+	u := *g.publicBaseURL
+	u.Path = path.Join(u.Path, "s", token)
+	return libregraph.PtrString(u.String())
+}
+
 func (g BaseGraphService) getDriveItem(ctx context.Context, ref *storageprovider.Reference) (*libregraph.DriveItem, error) {
 	gatewayClient, err := g.gatewaySelector.Next()
 	if err != nil {
@@ -66,7 +84,7 @@ func (g BaseGraphService) getDriveItem(ctx context.Context, ref *storageprovider
 		refStr, _ := storagespace.FormatReference(ref)
 		return nil, fmt.Errorf("could not stat %s: %s", refStr, res.GetStatus().GetMessage())
 	}
-	return cs3ResourceToDriveItem(g.logger, g.publicBaseURL, res.GetInfo())
+	return g.cs3ResourceToDriveItem(res.GetInfo())
 }
 
 func (g BaseGraphService) CS3ReceivedSharesToDriveItems(ctx context.Context, receivedShares []*collaboration.ReceivedShare) ([]libregraph.DriveItem, error) {
@@ -217,14 +235,6 @@ func (g BaseGraphService) cs3SpacePermissionsToLibreGraph(ctx context.Context, s
 }
 
 func (g BaseGraphService) libreGraphPermissionFromCS3PublicShare(createdLink *link.PublicShare) (*libregraph.Permission, error) {
-	webURL, err := url.Parse(g.config.Spaces.WebDavBase)
-	if err != nil {
-		g.logger.Error().
-			Err(err).
-			Str("url", g.config.Spaces.WebDavBase).
-			Msg("failed to parse webURL base url")
-		return nil, err
-	}
 	lt, actions := linktype.SharingLinkTypeFromCS3Permissions(createdLink.GetPermissions())
 	perm := libregraph.NewPermission()
 	perm.Id = libregraph.PtrString(createdLink.GetId().GetOpaqueId())
@@ -235,8 +245,7 @@ func (g BaseGraphService) libreGraphPermissionFromCS3PublicShare(createdLink *li
 		LibreGraphQuickLink:   libregraph.PtrBool(createdLink.GetQuicklink()),
 	}
 	perm.LibreGraphPermissionsActions = actions
-	webURL.Path = path.Join(webURL.Path, "s", createdLink.GetToken())
-	perm.Link.SetWebUrl(webURL.String())
+	perm.Link.SetWebUrl(*g.webURLForPublicShareToken(createdLink.GetToken()))
 
 	// set expiration date
 	if createdLink.GetExpiration() != nil {

services/graph/pkg/service/v0/driveitems.go

@@ -204,7 +204,7 @@ func (g Graph) GetRootDriveChildren(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	files, err := formatDriveItems(g.logger, g.publicBaseURL, lRes.GetInfos())
+	files, err := g.formatDriveItems(lRes.GetInfos())
 	if err != nil {
 		g.logger.Error().Err(err).Msg("error encoding response as json")
 		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
@@ -269,7 +269,7 @@ func (g Graph) GetDriveItem(w http.ResponseWriter, r *http.Request) {
 		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, res.GetStatus().GetMessage())
 		return
 	}
-	driveItem, err := cs3ResourceToDriveItem(g.logger, g.publicBaseURL, res.GetInfo())
+	driveItem, err := g.cs3ResourceToDriveItem(res.GetInfo())
 	if err != nil {
 		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
 		return
@@ -337,7 +337,7 @@ func (g Graph) GetDriveItemChildren(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	files, err := formatDriveItems(g.logger, g.publicBaseURL, res.GetInfos())
+	files, err := g.formatDriveItems(res.GetInfos())
 	if err != nil {
 		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
 		return
@@ -385,10 +385,10 @@ func (g Graph) getRemoteItem(ctx context.Context, root *storageprovider.Resource
 	return item, nil
 }
 
-func formatDriveItems(logger *log.Logger, publicBaseURL *url.URL, mds []*storageprovider.ResourceInfo) ([]*libregraph.DriveItem, error) {
+func (g BaseGraphService) formatDriveItems(mds []*storageprovider.ResourceInfo) ([]*libregraph.DriveItem, error) {
 	responses := make([]*libregraph.DriveItem, 0, len(mds))
 	for i := range mds {
-		res, err := cs3ResourceToDriveItem(logger, publicBaseURL, mds[i])
+		res, err := g.cs3ResourceToDriveItem(mds[i])
 		if err != nil {
 			return nil, err
 		}
@@ -402,19 +402,16 @@ func cs3TimestampToTime(t *types.Timestamp) time.Time {
 	return time.Unix(int64(t.GetSeconds()), int64(t.GetNanos()))
 }
 
-func cs3ResourceToDriveItem(logger *log.Logger, publicBaseURL *url.URL, res *storageprovider.ResourceInfo) (*libregraph.DriveItem, error) {
+func (g BaseGraphService) cs3ResourceToDriveItem(res *storageprovider.ResourceInfo) (*libregraph.DriveItem, error) {
 	size := new(int64)
 	*size = int64(res.GetSize()) // TODO lurking overflow: make size of libregraph drive item use uint64
 
 	driveItem := &libregraph.DriveItem{
-		Id:   libregraph.PtrString(storagespace.FormatResourceID(res.GetId())),
-		Size: size,
+		Id:     libregraph.PtrString(storagespace.FormatResourceID(res.GetId())),
+		Size:   size,
+		WebUrl: g.webURLForResource(res.GetId()),
 	}
 
-	webURL := *publicBaseURL
-	webURL.Path = path.Join(webURL.Path, "f", storagespace.FormatResourceID(res.GetId()))
-	driveItem.WebUrl = libregraph.PtrString(webURL.String())
-
 	if name := path.Base(res.GetPath()); name != "" {
 		driveItem.Name = &name
 	}
@@ -453,10 +450,10 @@ func cs3ResourceToDriveItem(logger *log.Logger, publicBaseURL *url.URL, res *sto
 	}
 
 	if res.GetArbitraryMetadata() != nil {
-		driveItem.Audio = cs3ResourceToDriveItemAudioFacet(logger, res)
-		driveItem.Image = cs3ResourceToDriveItemImageFacet(logger, res)
-		driveItem.Location = cs3ResourceToDriveItemLocationFacet(logger, res)
-		driveItem.Photo = cs3ResourceToDriveItemPhotoFacet(logger, res)
+		driveItem.Audio = cs3ResourceToDriveItemAudioFacet(g.logger, res)
+		driveItem.Image = cs3ResourceToDriveItemImageFacet(g.logger, res)
+		driveItem.Location = cs3ResourceToDriveItemLocationFacet(g.logger, res)
+		driveItem.Photo = cs3ResourceToDriveItemPhotoFacet(g.logger, res)
 	}
 
 	return driveItem, nil

services/graph/pkg/service/v0/driveitems_weburl_test.go

@@ -26,7 +26,8 @@ func TestCS3ResourceToDriveItemPopulatesWebUrl(t *testing.T) {
 		base, err := url.Parse("https://example.com")
 		require.NoError(t, err)
 
-		item, err := cs3ResourceToDriveItem(&logger, base, res)
+		g := BaseGraphService{logger: &logger, publicBaseURL: base}
+		item, err := g.cs3ResourceToDriveItem(res)
 		require.NoError(t, err)
 		require.NotNil(t, item.WebUrl)
 		assert.Equal(t, "https://example.com/f/storage-1$space-1%21item-1", *item.WebUrl)
@@ -36,7 +37,8 @@ func TestCS3ResourceToDriveItemPopulatesWebUrl(t *testing.T) {
 		base, err := url.Parse("https://example.com/cloud")
 		require.NoError(t, err)
 
-		item, err := cs3ResourceToDriveItem(&logger, base, res)
+		g := BaseGraphService{logger: &logger, publicBaseURL: base}
+		item, err := g.cs3ResourceToDriveItem(res)
 		require.NoError(t, err)
 		require.NotNil(t, item.WebUrl)
 		assert.Equal(t, "https://example.com/cloud/f/storage-1$space-1%21item-1", *item.WebUrl)

services/graph/pkg/service/v0/drives.go

@@ -850,17 +850,7 @@ func (g Graph) cs3StorageSpaceToDrive(ctx context.Context, baseURL *url.URL, spa
 		drive.Root.WebDavUrl = libregraph.PtrString(webDavURL.String())
 	}
 
-	webURL, err := url.Parse(g.config.Spaces.WebDavBase)
-	if err != nil {
-		logger.Error().
-			Err(err).
-			Str("url", g.config.Spaces.WebDavBase).
-			Msg("failed to parse webURL base url")
-		return nil, err
-	}
-
-	webURL.Path = path.Join(webURL.Path, "f", storagespace.FormatResourceID(spaceRid))
-	drive.WebUrl = libregraph.PtrString(webURL.String())
+	drive.WebUrl = g.webURLForResource(spaceRid)
 
 	if space.Owner != nil && space.Owner.Id != nil {
 		drive.Owner = &libregraph.IdentitySet{

services/graph/pkg/service/v0/follow.go

@@ -94,7 +94,7 @@ func (g Graph) FollowDriveItem(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	driveItem, err := cs3ResourceToDriveItem(g.logger, g.publicBaseURL, statRes.GetInfo())
+	driveItem, err := g.cs3ResourceToDriveItem(statRes.GetInfo())
 	if err != nil {
 		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
 		return