From 3246f6135e6e0a2107625320df09a9a3e067bf81 Mon Sep 17 00:00:00 2001 From: ryanfkeepers Date: Tue, 3 Jan 2023 14:50:12 -0700 Subject: [PATCH] add newClient constructor in api --- src/cmd/getM365/getItem.go | 5 ++- src/internal/connector/exchange/api/api.go | 29 ++++++++++++--- .../connector/exchange/api/contacts.go | 35 +++---------------- src/internal/connector/exchange/api/events.go | 28 +++------------ src/internal/connector/exchange/api/mail.go | 28 +++------------ .../exchange/folder_resolver_test.go | 7 ++-- .../connector/exchange/iterators_test.go | 16 +++++---- .../exchange/mail_folder_cache_test.go | 5 ++- .../connector/exchange/restore_test.go | 11 ++---- .../connector/exchange/service_functions.go | 11 ++++-- .../connector/exchange/service_iterators.go | 7 +++- .../connector/exchange/service_restore.go | 7 +++- .../connector/graph_connector_test.go | 7 ++-- 13 files changed, 86 insertions(+), 110 deletions(-) diff --git a/src/cmd/getM365/getItem.go b/src/cmd/getM365/getItem.go index 44da83567..10648006a 100644 --- a/src/cmd/getM365/getItem.go +++ b/src/cmd/getM365/getItem.go @@ -101,7 +101,10 @@ func runDisplayM365JSON( cat = graph.StringToPathCategory(category) ) - ac := api.Client{Credentials: creds} + ac, err := api.NewClient(creds) + if err != nil { + return err + } switch cat { case path.EmailCategory, path.EventsCategory, path.ContactsCategory: diff --git a/src/internal/connector/exchange/api/api.go b/src/internal/connector/exchange/api/api.go index f03765f34..8c8ad3392 100644 --- a/src/internal/connector/exchange/api/api.go +++ b/src/internal/connector/exchange/api/api.go @@ -50,17 +50,38 @@ type GraphRetrievalFunc func( // from the exchange package's broader intents. type Client struct { Credentials account.M365Config + + // The stable service is re-usable for any non-paged request. + // This allows us to maintain performance across async requests. + stable graph.Servicer } +// NewClient produces a new exchange api client. Must be used in +// place of creating an ad-hoc client struct. +func NewClient(creds account.M365Config) (Client, error) { + s, err := newService(creds) + if err != nil { + return Client{}, err + } + + return Client{creds, s}, nil +} + +// service generates a new service. Used for paged and other long-running +// requests instead of the client's stable service, so that in-flight state +// within the adapter doesn't get clobbered func (c Client) service() (*graph.Service, error) { + return newService(c.Credentials) +} + +func newService(creds account.M365Config) (*graph.Service, error) { adapter, err := graph.CreateAdapter( - c.Credentials.AzureTenantID, - c.Credentials.AzureClientID, - c.Credentials.AzureClientSecret, + creds.AzureTenantID, + creds.AzureClientID, + creds.AzureClientSecret, ) if err != nil { return nil, errors.Wrap(err, "generating graph api service client") } - return graph.NewService(adapter), nil } diff --git a/src/internal/connector/exchange/api/contacts.go b/src/internal/connector/exchange/api/contacts.go index 89395a9fb..293671b8e 100644 --- a/src/internal/connector/exchange/api/contacts.go +++ b/src/internal/connector/exchange/api/contacts.go @@ -19,16 +19,11 @@ func (c Client) CreateContactFolder( ctx context.Context, user, folderName string, ) (models.ContactFolderable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - requestBody := models.NewContactFolder() temp := folderName requestBody.SetDisplayName(&temp) - return service.Client().UsersById(user).ContactFolders().Post(ctx, requestBody, nil) + return c.stable.Client().UsersById(user).ContactFolders().Post(ctx, requestBody, nil) } // DeleteContactFolder deletes the ContactFolder associated with the M365 ID if permissions are valid. @@ -37,12 +32,7 @@ func (c Client) DeleteContactFolder( ctx context.Context, user, folderID string, ) error { - service, err := c.service() - if err != nil { - return err - } - - return service.Client().UsersById(user).ContactFoldersById(folderID).Delete(ctx, nil) + return c.stable.Client().UsersById(user).ContactFoldersById(folderID).Delete(ctx, nil) } // RetrieveContactDataForUser is a GraphRetrievalFun that returns all associated fields. @@ -50,12 +40,7 @@ func (c Client) RetrieveContactDataForUser( ctx context.Context, user, m365ID string, ) (serialization.Parsable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - - return service.Client().UsersById(user).ContactsById(m365ID).Get(ctx, nil) + return c.stable.Client().UsersById(user).ContactsById(m365ID).Get(ctx, nil) } // GetAllContactFolderNamesForUser is a GraphQuery function for getting @@ -65,17 +50,12 @@ func (c Client) GetAllContactFolderNamesForUser( ctx context.Context, user string, ) (serialization.Parsable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - options, err := optionsForContactFolders([]string{"displayName", "parentFolderId"}) if err != nil { return nil, err } - return service.Client().UsersById(user).ContactFolders().Get(ctx, options) + return c.stable.Client().UsersById(user).ContactFolders().Get(ctx, options) } func (c Client) GetContactFolderByID( @@ -83,11 +63,6 @@ func (c Client) GetContactFolderByID( userID, dirID string, optionalFields ...string, ) (models.ContactFolderable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - fields := append([]string{"displayName", "parentFolderId"}, optionalFields...) ofcf, err := optionsForContactFolderByID(fields) @@ -95,7 +70,7 @@ func (c Client) GetContactFolderByID( return nil, errors.Wrapf(err, "options for contact folder: %v", fields) } - return service.Client(). + return c.stable.Client(). UsersById(userID). ContactFoldersById(dirID). Get(ctx, ofcf) diff --git a/src/internal/connector/exchange/api/events.go b/src/internal/connector/exchange/api/events.go index a3636f2ba..9ea34568e 100644 --- a/src/internal/connector/exchange/api/events.go +++ b/src/internal/connector/exchange/api/events.go @@ -19,15 +19,10 @@ func (c Client) CreateCalendar( ctx context.Context, user, calendarName string, ) (models.Calendarable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - requestbody := models.NewCalendar() requestbody.SetName(&calendarName) - return service.Client().UsersById(user).Calendars().Post(ctx, requestbody, nil) + return c.stable.Client().UsersById(user).Calendars().Post(ctx, requestbody, nil) } // DeleteCalendar removes calendar from user's M365 account @@ -36,12 +31,7 @@ func (c Client) DeleteCalendar( ctx context.Context, user, calendarID string, ) error { - service, err := c.service() - if err != nil { - return err - } - - return service.Client().UsersById(user).CalendarsById(calendarID).Delete(ctx, nil) + return c.stable.Client().UsersById(user).CalendarsById(calendarID).Delete(ctx, nil) } // RetrieveEventDataForUser is a GraphRetrievalFunc that returns event data. @@ -50,29 +40,19 @@ func (c Client) RetrieveEventDataForUser( ctx context.Context, user, m365ID string, ) (serialization.Parsable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - - return service.Client().UsersById(user).EventsById(m365ID).Get(ctx, nil) + return c.stable.Client().UsersById(user).EventsById(m365ID).Get(ctx, nil) } func (c Client) GetAllCalendarNamesForUser( ctx context.Context, user string, ) (serialization.Parsable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - options, err := optionsForCalendars([]string{"name", "owner"}) if err != nil { return nil, err } - return service.Client().UsersById(user).Calendars().Get(ctx, options) + return c.stable.Client().UsersById(user).Calendars().Get(ctx, options) } // TODO: we want this to be the full handler, not only the builder. diff --git a/src/internal/connector/exchange/api/mail.go b/src/internal/connector/exchange/api/mail.go index ba7059dba..79b2c90f4 100644 --- a/src/internal/connector/exchange/api/mail.go +++ b/src/internal/connector/exchange/api/mail.go @@ -19,17 +19,12 @@ func (c Client) CreateMailFolder( ctx context.Context, user, folder string, ) (models.MailFolderable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - isHidden := false requestBody := models.NewMailFolder() requestBody.SetDisplayName(&folder) requestBody.SetIsHidden(&isHidden) - return service.Client().UsersById(user).MailFolders().Post(ctx, requestBody, nil) + return c.stable.Client().UsersById(user).MailFolders().Post(ctx, requestBody, nil) } func (c Client) CreateMailFolderWithParent( @@ -60,12 +55,7 @@ func (c Client) DeleteMailFolder( ctx context.Context, user, folderID string, ) error { - service, err := c.service() - if err != nil { - return err - } - - return service.Client().UsersById(user).MailFoldersById(folderID).Delete(ctx, nil) + return c.stable.Client().UsersById(user).MailFoldersById(folderID).Delete(ctx, nil) } // RetrieveMessageDataForUser is a GraphRetrievalFunc that returns message data. @@ -74,12 +64,7 @@ func (c Client) RetrieveMessageDataForUser( ctx context.Context, user, m365ID string, ) (serialization.Parsable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - - return service.Client().UsersById(user).MessagesById(m365ID).Get(ctx, nil) + return c.stable.Client().UsersById(user).MessagesById(m365ID).Get(ctx, nil) } // GetMailFoldersBuilder retrieves all of the users current mail folders. @@ -113,17 +98,12 @@ func (c Client) GetMailFolderByID( userID, dirID string, optionalFields ...string, ) (models.MailFolderable, error) { - service, err := c.service() - if err != nil { - return nil, err - } - ofmf, err := optionsForMailFoldersItem(optionalFields) if err != nil { return nil, errors.Wrapf(err, "options for mail folder: %v", optionalFields) } - return service.Client().UsersById(userID).MailFoldersById(dirID).Get(ctx, ofmf) + return c.stable.Client().UsersById(userID).MailFoldersById(dirID).Get(ctx, ofmf) } // FetchMessageIDsFromDirectory function that returns a list of all the m365IDs of the exchange.Mail diff --git a/src/internal/connector/exchange/folder_resolver_test.go b/src/internal/connector/exchange/folder_resolver_test.go index 0cc219761..68c17e408 100644 --- a/src/internal/connector/exchange/folder_resolver_test.go +++ b/src/internal/connector/exchange/folder_resolver_test.go @@ -44,17 +44,20 @@ func (suite *CacheResolverSuite) TestPopulate() { ctx, flush := tester.NewContext() defer flush() + ac, err := api.NewClient(suite.credentials) + require.NoError(suite.T(), err) + eventFunc := func(t *testing.T) graph.ContainerResolver { return &eventCalendarCache{ userID: tester.M365UserID(t), - ac: api.Client{Credentials: suite.credentials}, + ac: ac, } } contactFunc := func(t *testing.T) graph.ContainerResolver { return &contactFolderCache{ userID: tester.M365UserID(t), - ac: api.Client{Credentials: suite.credentials}, + ac: ac, } } diff --git a/src/internal/connector/exchange/iterators_test.go b/src/internal/connector/exchange/iterators_test.go index fd784138a..2ae9abf2b 100644 --- a/src/internal/connector/exchange/iterators_test.go +++ b/src/internal/connector/exchange/iterators_test.go @@ -85,7 +85,7 @@ func (suite *ExchangeIteratorSuite) TestCollectionFunctions() { tests := []struct { name string - queryFunc func(account.M365Config) api.GraphQuery + queryFunc func(*testing.T, account.M365Config) api.GraphQuery scope selectors.ExchangeScope iterativeFunction func( container map[string]graph.Container, @@ -95,16 +95,20 @@ func (suite *ExchangeIteratorSuite) TestCollectionFunctions() { }{ { name: "Contacts Iterative Check", - queryFunc: func(amc account.M365Config) api.GraphQuery { - return api.Client{Credentials: amc}.GetAllContactFolderNamesForUser + queryFunc: func(t *testing.T, amc account.M365Config) api.GraphQuery { + ac, err := api.NewClient(amc) + require.NoError(t, err) + return ac.GetAllContactFolderNamesForUser }, transformer: models.CreateContactFolderCollectionResponseFromDiscriminatorValue, iterativeFunction: IterativeCollectContactContainers, }, { name: "Events Iterative Check", - queryFunc: func(amc account.M365Config) api.GraphQuery { - return api.Client{Credentials: amc}.GetAllCalendarNamesForUser + queryFunc: func(t *testing.T, amc account.M365Config) api.GraphQuery { + ac, err := api.NewClient(amc) + require.NoError(t, err) + return ac.GetAllCalendarNamesForUser }, transformer: models.CreateCalendarCollectionResponseFromDiscriminatorValue, iterativeFunction: IterativeCollectCalendarContainers, @@ -119,7 +123,7 @@ func (suite *ExchangeIteratorSuite) TestCollectionFunctions() { service, err := createService(m365) require.NoError(t, err) - response, err := test.queryFunc(m365)(ctx, userID) + response, err := test.queryFunc(t, m365)(ctx, userID) require.NoError(t, err) // Iterator Creation diff --git a/src/internal/connector/exchange/mail_folder_cache_test.go b/src/internal/connector/exchange/mail_folder_cache_test.go index 9568747bb..3cd58be91 100644 --- a/src/internal/connector/exchange/mail_folder_cache_test.go +++ b/src/internal/connector/exchange/mail_folder_cache_test.go @@ -81,9 +81,12 @@ func (suite *MailFolderCacheIntegrationSuite) TestDeltaFetch() { for _, test := range tests { suite.T().Run(test.name, func(t *testing.T) { + ac, err := api.NewClient(suite.credentials) + require.NoError(t, err) + mfc := mailFolderCache{ userID: userID, - ac: api.Client{Credentials: suite.credentials}, + ac: ac, } require.NoError(t, mfc.Populate(ctx, test.root, test.path...)) diff --git a/src/internal/connector/exchange/restore_test.go b/src/internal/connector/exchange/restore_test.go index 4904eaa4c..df1a94114 100644 --- a/src/internal/connector/exchange/restore_test.go +++ b/src/internal/connector/exchange/restore_test.go @@ -45,15 +45,8 @@ func (suite *ExchangeRestoreSuite) SetupSuite() { require.NoError(t, err) suite.credentials = m365 - suite.ac = api.Client{Credentials: m365} - - // adpt, err := graph.CreateAdapter( - // m365.AzureTenantID, - // m365.AzureClientID, - // m365.AzureClientSecret) - // require.NoError(t, err) - - // suite.gs = graph.NewService(adpt) + suite.ac, err = api.NewClient(m365) + require.NoError(t, err) require.NoError(suite.T(), err) } diff --git a/src/internal/connector/exchange/service_functions.go b/src/internal/connector/exchange/service_functions.go index 3dc36abcc..579a7915c 100644 --- a/src/internal/connector/exchange/service_functions.go +++ b/src/internal/connector/exchange/service_functions.go @@ -41,25 +41,30 @@ func PopulateExchangeContainerResolver( cacheRoot string ) + ac, err := api.NewClient(qp.Credentials) + if err != nil { + return nil, err + } + switch qp.Category { case path.EmailCategory: res = &mailFolderCache{ userID: qp.ResourceOwner, - ac: api.Client{Credentials: qp.Credentials}, + ac: ac, } cacheRoot = rootFolderAlias case path.ContactsCategory: res = &contactFolderCache{ userID: qp.ResourceOwner, - ac: api.Client{Credentials: qp.Credentials}, + ac: ac, } cacheRoot = DefaultContactFolder case path.EventsCategory: res = &eventCalendarCache{ userID: qp.ResourceOwner, - ac: api.Client{Credentials: qp.Credentials}, + ac: ac, } cacheRoot = DefaultCalendar diff --git a/src/internal/connector/exchange/service_iterators.go b/src/internal/connector/exchange/service_iterators.go index 834e06a21..e8074ae12 100644 --- a/src/internal/connector/exchange/service_iterators.go +++ b/src/internal/connector/exchange/service_iterators.go @@ -35,7 +35,6 @@ func filterContainersAndFillCollections( ) error { var ( errs error - ac = api.Client{Credentials: qp.Credentials} // folder ID -> delta url or folder path lookups deltaURLs = map[string]string{} currPaths = map[string]string{} @@ -44,6 +43,12 @@ func filterContainersAndFillCollections( tombstones = makeTombstones(dps) ) + // TODO(rkeepers): pass in the api client instead of generating it here. + ac, err := api.NewClient(qp.Credentials) + if err != nil { + return err + } + getJobs, err := getFetchIDFunc(ac, qp.Category) if err != nil { return support.WrapAndAppend(qp.ResourceOwner, err, errs) diff --git a/src/internal/connector/exchange/service_restore.go b/src/internal/connector/exchange/service_restore.go index 9babfdaad..183bd50bf 100644 --- a/src/internal/connector/exchange/service_restore.go +++ b/src/internal/connector/exchange/service_restore.go @@ -439,7 +439,6 @@ func CreateContainerDestinaion( caches map[path.CategoryType]graph.ContainerResolver, ) (string, error) { var ( - ac = api.Client{Credentials: creds} newCache = false user = directory.ResourceOwner() category = directory.Category() @@ -447,6 +446,12 @@ func CreateContainerDestinaion( newPathFolders = append([]string{destination}, directory.Folders()...) ) + // TODO(rkeepers): pass the api client into this func, rather than generating one. + ac, err := api.NewClient(creds) + if err != nil { + return "", err + } + switch category { case path.EmailCategory: if directoryCache == nil { diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index 28481a26e..1b3f9e925 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -134,10 +134,9 @@ func (suite *GraphConnectorUnitSuite) TestUnionSiteIDsAndWebURLs() { type GraphConnectorIntegrationSuite struct { suite.Suite - connector *GraphConnector - user string - acct account.Account - credentials account.M365Config + connector *GraphConnector + user string + acct account.Account } func TestGraphConnectorIntegrationSuite(t *testing.T) {