From 6215fe1ac5b391e9f2060d4ef3f65a742db8ad4d Mon Sep 17 00:00:00 2001 From: Kenneth Hsu Date: Sun, 30 Sep 2018 10:56:59 -0700 Subject: [PATCH 1/6] Extract string to private constant. This will allow us to provide the string as a parameter to the parent class constructor to avoid referencing virtual members in the constructor. --- Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs | 3 ++- Duplicati/Library/Backend/OneDrive/OneDriveV2.cs | 4 ++-- Duplicati/Library/Backend/OneDrive/SharePointV2.cs | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs b/Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs index 335e42b87..b564bf899 100644 --- a/Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs +++ b/Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs @@ -10,6 +10,7 @@ namespace Duplicati.Library.Backend { private const string GROUP_EMAIL_OPTION = "group-email"; private const string GROUP_ID_OPTION = "group-id"; + private const string PROTOCOL_KEY = "msgroup"; private readonly string drivePath; @@ -46,7 +47,7 @@ namespace Duplicati.Library.Backend public override string ProtocolKey { - get { return "msgroup"; } + get { return MicrosoftGroup.PROTOCOL_KEY; } } public override string DisplayName diff --git a/Duplicati/Library/Backend/OneDrive/OneDriveV2.cs b/Duplicati/Library/Backend/OneDrive/OneDriveV2.cs index 366617c93..6fc678a40 100644 --- a/Duplicati/Library/Backend/OneDrive/OneDriveV2.cs +++ b/Duplicati/Library/Backend/OneDrive/OneDriveV2.cs @@ -7,8 +7,8 @@ namespace Duplicati.Library.Backend public class OneDriveV2 : MicrosoftGraphBackend { private const string DRIVE_ID_OPTION = "drive-id"; - private const string DEFAULT_DRIVE_PATH = "/me/drive"; + private const string PROTOCOL_KEY = "onedrivev2"; private readonly string drivePath; @@ -30,7 +30,7 @@ namespace Duplicati.Library.Backend public override string ProtocolKey { - get { return "onedrivev2"; } + get { return OneDriveV2.PROTOCOL_KEY; } } public override string DisplayName diff --git a/Duplicati/Library/Backend/OneDrive/SharePointV2.cs b/Duplicati/Library/Backend/OneDrive/SharePointV2.cs index f415715b2..a38d5528d 100644 --- a/Duplicati/Library/Backend/OneDrive/SharePointV2.cs +++ b/Duplicati/Library/Backend/OneDrive/SharePointV2.cs @@ -11,6 +11,7 @@ namespace Duplicati.Library.Backend public class SharePointV2 : MicrosoftGraphBackend { private const string SITE_ID_OPTION = "site-id"; + private const string PROTOCOL_KEY = "sharepoint"; private readonly string drivePath; private string siteId = null; @@ -42,7 +43,7 @@ namespace Duplicati.Library.Backend public override string ProtocolKey { - get { return "sharepoint"; } + get { return SharePointV2.PROTOCOL_KEY; } } public override string DisplayName From 7cf830cad7fe0c78846eccc04a150f2976505aa3 Mon Sep 17 00:00:00 2001 From: Kenneth Hsu Date: Sun, 30 Sep 2018 10:59:26 -0700 Subject: [PATCH 2/6] Provide protocol key to MicrosoftGraphBackend constructor. This will help us avoid referencing virtual members in the constructor. --- Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs | 6 +++--- Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs | 2 +- Duplicati/Library/Backend/OneDrive/OneDriveV2.cs | 2 +- Duplicati/Library/Backend/OneDrive/SharePointV2.cs | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs index ebf9a92a6..6ddcbe263 100644 --- a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs +++ b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs @@ -83,12 +83,12 @@ namespace Duplicati.Library.Backend protected MicrosoftGraphBackend() { } // Constructor needed for dynamic loading to find it - protected MicrosoftGraphBackend(string url, Dictionary options) + protected MicrosoftGraphBackend(string url, string protocolKey, Dictionary options) { string authid; options.TryGetValue(AUTHID_OPTION, out authid); if (string.IsNullOrEmpty(authid)) - throw new UserInformationException(Strings.MicrosoftGraph.MissingAuthId(OAuthHelper.OAUTH_LOGIN_URL(this.ProtocolKey)), "MicrosoftGraphBackendMissingAuthId"); + throw new UserInformationException(Strings.MicrosoftGraph.MissingAuthId(OAuthHelper.OAUTH_LOGIN_URL(protocolKey)), "MicrosoftGraphBackendMissingAuthId"); string fragmentSizeStr; if (options.TryGetValue(UPLOAD_SESSION_FRAGMENT_SIZE_OPTION, out fragmentSizeStr) && int.TryParse(fragmentSizeStr, out this.fragmentSize)) @@ -117,7 +117,7 @@ namespace Duplicati.Library.Backend this.fragmentRetryDelay = UPLOAD_SESSION_FRAGMENT_DEFAULT_RETRY_DELAY; } - this.m_client = new OAuthHttpClient(authid, this.ProtocolKey); + this.m_client = new OAuthHttpClient(authid, protocolKey); this.m_client.BaseAddress = new System.Uri(BASE_ADDRESS); // Extract out the path to the backup root folder from the given URI diff --git a/Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs b/Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs index b564bf899..fb3862ffe 100644 --- a/Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs +++ b/Duplicati/Library/Backend/OneDrive/MicrosoftGroup.cs @@ -17,7 +17,7 @@ namespace Duplicati.Library.Backend public MicrosoftGroup() { } // Constructor needed for dynamic loading to find it public MicrosoftGroup(string url, Dictionary options) - : base(url, options) + : base(url, MicrosoftGroup.PROTOCOL_KEY, options) { string groupId = null; string groupEmail; diff --git a/Duplicati/Library/Backend/OneDrive/OneDriveV2.cs b/Duplicati/Library/Backend/OneDrive/OneDriveV2.cs index 6fc678a40..fa151a710 100644 --- a/Duplicati/Library/Backend/OneDrive/OneDriveV2.cs +++ b/Duplicati/Library/Backend/OneDrive/OneDriveV2.cs @@ -15,7 +15,7 @@ namespace Duplicati.Library.Backend public OneDriveV2() { } // Constructor needed for dynamic loading to find it public OneDriveV2(string url, Dictionary options) - : base(url, options) + : base(url, OneDriveV2.PROTOCOL_KEY, options) { string driveId; if (options.TryGetValue(DRIVE_ID_OPTION, out driveId)) diff --git a/Duplicati/Library/Backend/OneDrive/SharePointV2.cs b/Duplicati/Library/Backend/OneDrive/SharePointV2.cs index a38d5528d..d42b7195a 100644 --- a/Duplicati/Library/Backend/OneDrive/SharePointV2.cs +++ b/Duplicati/Library/Backend/OneDrive/SharePointV2.cs @@ -19,7 +19,7 @@ namespace Duplicati.Library.Backend public SharePointV2() { } // Constructor needed for dynamic loading to find it public SharePointV2(string url, Dictionary options) - : base(url, options) + : base(url, SharePointV2.PROTOCOL_KEY, options) { // Check to see if a site ID was explicitly provided string siteIdOption; From f9022fec66cf33cb28e039b3046c31a111008493 Mon Sep 17 00:00:00 2001 From: Kenneth Hsu Date: Sun, 30 Sep 2018 13:43:45 -0700 Subject: [PATCH 3/6] Avoid referencing virtual members in constructor. Since the call to GetRootPathFromUrl may be expensive, we cache the value using a lazy initializer. --- Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs index 6ddcbe263..b2be12e5d 100644 --- a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs +++ b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs @@ -74,13 +74,15 @@ namespace Duplicati.Library.Backend private readonly JsonSerializer m_serializer = new JsonSerializer(); private readonly OAuthHttpClient m_client; - private readonly string m_path; private readonly int fragmentSize; private readonly int fragmentRetryCount; private readonly int fragmentRetryDelay; // In milliseconds private string[] dnsNames = null; + private Lazy rootPathFromURL; + private string m_path => this.rootPathFromURL.Value; + protected MicrosoftGraphBackend() { } // Constructor needed for dynamic loading to find it protected MicrosoftGraphBackend(string url, string protocolKey, Dictionary options) @@ -121,7 +123,7 @@ namespace Duplicati.Library.Backend this.m_client.BaseAddress = new System.Uri(BASE_ADDRESS); // Extract out the path to the backup root folder from the given URI - this.m_path = NormalizeSlashes(this.GetRootPathFromUrl(url)); + this.rootPathFromURL = new Lazy(() => this.GetRootPathFromUrl(url)); } public abstract string ProtocolKey { get; } From f59607d94d4b37424696c193e341f349b3f0d611 Mon Sep 17 00:00:00 2001 From: Kenneth Hsu Date: Sun, 30 Sep 2018 14:01:44 -0700 Subject: [PATCH 4/6] Rename property. --- .../Backend/OneDrive/MicrosoftGraphBackend.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs index b2be12e5d..08afd0a4d 100644 --- a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs +++ b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs @@ -81,7 +81,7 @@ namespace Duplicati.Library.Backend private string[] dnsNames = null; private Lazy rootPathFromURL; - private string m_path => this.rootPathFromURL.Value; + private string RootPath => this.rootPathFromURL.Value; protected MicrosoftGraphBackend() { } // Constructor needed for dynamic loading to find it @@ -169,7 +169,7 @@ namespace Duplicati.Library.Backend // To get the upload session endpoint, we can start an upload session and then immediately cancel it. // We pick a random file name (using a guid) to make sure we don't conflict with an existing file string dnsTestFile = string.Format("DNSNameTest-{0}", Guid.NewGuid()); - UploadSession uploadSession = this.Post(string.Format("{0}/root:{1}{2}:/createUploadSession", this.DrivePrefix, this.m_path, NormalizeSlashes(dnsTestFile)), null); + UploadSession uploadSession = this.Post(string.Format("{0}/root:{1}{2}:/createUploadSession", this.DrivePrefix, this.RootPath, NormalizeSlashes(dnsTestFile)), null); // Canceling an upload session is done by sending a DELETE to the upload URL var request = new HttpRequestMessage(HttpMethod.Delete, uploadSession.UploadUrl); @@ -242,7 +242,7 @@ namespace Duplicati.Library.Backend { string parentFolder = "root"; string parentFolderPath = string.Empty; - foreach (string folder in this.m_path.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries)) + foreach (string folder in this.RootPath.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries)) { string nextPath = parentFolderPath + "/" + folder; DriveItem folderItem; @@ -270,7 +270,7 @@ namespace Duplicati.Library.Backend { try { - return this.Enumerate(string.Format("{0}/root:{1}:/children", this.DrivePrefix, this.m_path)) + return this.Enumerate(string.Format("{0}/root:{1}:/children", this.DrivePrefix, this.RootPath)) .Where(item => item.IsFile && !item.IsDeleted) // Exclude non-files and deleted items (not sure if they show up in this listing, but make sure anyway) .Select(item => new FileEntry( @@ -298,7 +298,7 @@ namespace Duplicati.Library.Backend { try { - var response = this.m_client.GetAsync(string.Format("{0}/root:{1}{2}:/content", this.DrivePrefix, this.m_path, NormalizeSlashes(remotename))).Await(); + var response = this.m_client.GetAsync(string.Format("{0}/root:{1}{2}:/content", this.DrivePrefix, this.RootPath, NormalizeSlashes(remotename))).Await(); this.CheckResponse(response); using (Stream responseStream = response.Content.ReadAsStreamAsync().Await()) { @@ -316,7 +316,7 @@ namespace Duplicati.Library.Backend { try { - this.Patch(string.Format("{0}/root:{1}{2}", this.DrivePrefix, this.m_path, NormalizeSlashes(oldname)), new DriveItem() { Name = newname }); + this.Patch(string.Format("{0}/root:{1}{2}", this.DrivePrefix, this.RootPath, NormalizeSlashes(oldname)), new DriveItem() { Name = newname }); } catch (DriveItemNotFoundException ex) { @@ -340,7 +340,7 @@ namespace Duplicati.Library.Backend { StreamContent streamContent = new StreamContent(stream); streamContent.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream"); - var response = this.m_client.PutAsync(string.Format("{0}/root:{1}{2}:/content", this.DrivePrefix, this.m_path, NormalizeSlashes(remotename)), streamContent).Await(); + var response = this.m_client.PutAsync(string.Format("{0}/root:{1}{2}:/content", this.DrivePrefix, this.RootPath, NormalizeSlashes(remotename)), streamContent).Await(); // Make sure this response is a valid drive item, though we don't actually use it for anything currently. var result = this.ParseResponse(response); @@ -352,7 +352,7 @@ namespace Duplicati.Library.Backend // The documentation seems somewhat contradictory - it states that uploads must be done sequentially, // but also states that the nextExpectedRanges value returned may indicate multiple ranges... // For now, this plays it safe and does a sequential upload. - HttpRequestMessage createSessionRequest = new HttpRequestMessage(HttpMethod.Post, string.Format("{0}/root:{1}{2}:/createUploadSession", this.DrivePrefix, this.m_path, NormalizeSlashes(remotename))); + HttpRequestMessage createSessionRequest = new HttpRequestMessage(HttpMethod.Post, string.Format("{0}/root:{1}{2}:/createUploadSession", this.DrivePrefix, this.RootPath, NormalizeSlashes(remotename))); // Indicate that we want to replace any existing content with this new data we're uploading StringContent createSessionContent = this.PrepareContent(new UploadSession() { Item = new DriveItem() { ConflictBehavior = ConflictBehavior.Replace } }); @@ -431,7 +431,7 @@ namespace Duplicati.Library.Backend public void Delete(string remotename) { - var response = this.m_client.DeleteAsync(string.Format("{0}/root:{1}{2}", this.DrivePrefix, this.m_path, NormalizeSlashes(remotename))).Await(); + var response = this.m_client.DeleteAsync(string.Format("{0}/root:{1}{2}", this.DrivePrefix, this.RootPath, NormalizeSlashes(remotename))).Await(); try { this.CheckResponse(response); @@ -447,7 +447,7 @@ namespace Duplicati.Library.Backend { try { - string rootPath = string.Format("{0}/root:{1}", this.DrivePrefix, this.m_path); + string rootPath = string.Format("{0}/root:{1}", this.DrivePrefix, this.RootPath); DriveItem rootFolder = this.Get(rootPath); } catch (DriveItemNotFoundException ex) From ee14862db56184cf888345aec0ed606d976cde9f Mon Sep 17 00:00:00 2001 From: Kenneth Hsu Date: Sun, 30 Sep 2018 15:06:29 -0700 Subject: [PATCH 5/6] Add comment to clarify use of Lazy. --- Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs index 08afd0a4d..3c61c10b5 100644 --- a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs +++ b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs @@ -122,7 +122,8 @@ namespace Duplicati.Library.Backend this.m_client = new OAuthHttpClient(authid, protocolKey); this.m_client.BaseAddress = new System.Uri(BASE_ADDRESS); - // Extract out the path to the backup root folder from the given URI + // Extract out the path to the backup root folder from the given URI. Since this can be an expensive operation, + // we will cache the value using a lazy initializer. this.rootPathFromURL = new Lazy(() => this.GetRootPathFromUrl(url)); } From 024bc5bea2745bc21ccdfdf865211913d8c397a1 Mon Sep 17 00:00:00 2001 From: Kenneth Hsu Date: Sun, 30 Sep 2018 15:45:42 -0700 Subject: [PATCH 6/6] Mark immutable field as readonly. --- Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs index 3c61c10b5..ecc0b09de 100644 --- a/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs +++ b/Duplicati/Library/Backend/OneDrive/MicrosoftGraphBackend.cs @@ -80,7 +80,7 @@ namespace Duplicati.Library.Backend private string[] dnsNames = null; - private Lazy rootPathFromURL; + private readonly Lazy rootPathFromURL; private string RootPath => this.rootPathFromURL.Value; protected MicrosoftGraphBackend() { } // Constructor needed for dynamic loading to find it