Public default settings #191

sorenhansen · 2020-04-15T14:47:45Z

Continuing along the lines of PR #189, this PR makes
DefaultMessagePropertyToIdentitySasUri and DefaultSasTokenValidationTime public.

Again, the reason being that the default values can be used by consumers in order (for example) to implement cleanup.

SeanFeldman · 2020-04-15T14:49:40Z

❌ Build ServiceBus.AttachmentPlugin 0.0.765-preview failed (commit 30cf7520a7 by @)

SeanFeldman · 2020-04-15T14:53:55Z

✅ Build ServiceBus.AttachmentPlugin 0.0.766-preview completed (commit 4825d46896 by @)

SeanFeldman · 2020-04-15T16:41:13Z

README.source.md

-```c#
-new AzureStorageAttachmentConfiguration(storageConnectionString).WithSasUri(messagePropertyToIdentifySasUri: "mySasUriProperty");
-```
+Default SAS token validation time is 7 days. The value is available via `AzureStorageAttachmentConfigurationExtensions.DefaultSasTokenValidationTime` constant.


👍 on documenting the default value.

SeanFeldman · 2020-04-15T16:41:41Z

README.source.md

-```
+Default SAS token validation time is 7 days. The value is available via `AzureStorageAttachmentConfigurationExtensions.DefaultSasTokenValidationTime` constant.
+
+snippet: Configure_blob_sas_uri_override


Thank you for replacing hardcoded snippet with a verifiable code snippet 👏

SeanFeldman · 2020-04-15T16:43:16Z

....AttachmentPlugin.Tests/ApprovalFiles/ApiApprovals.AzureStorageAttachmentPlugin.approved.txt

@@ -11,6 +11,8 @@ namespace Microsoft.Azure.ServiceBus
    }
    public static class AzureStorageAttachmentConfigurationExtensions
    {
+        public const string DefaultMessagePropertyToIdentitySasUri = "$attachment.sas.uri";
+        public static readonly System.TimeSpan DefaultSasTokenValidationTime;


What's the value of exposing this? If the default is documented and known, how you would use this publicly exposed member field in your code? I'd like to understand how it would be used in the customer/user code.

The argument is the same as on #185

Focusing on ContainerName and MessagePropertyToIdentifyAttachmentBlob let's assume you're using the defaults. Both properties, ContainerName and MessagePropertyToIdentifyAttachmentBlob have their defaults provided by the plugin which you'd need to use. Those are well documented and will not change to keep wire compatibility. You can use those strings in your code. My understanding is you don't like to use the strings and rather have something strongly types exposed by the AzureStorageAttachmentConfiguration class. That can be done, don't see an issue with it.

I would like to have access to a strongly typed values, so I don't have to worry about the values, even though I know they won't change. It reduces complexity (albeit not much) in my code.

I hope it makes sense.

I would like to have access to a strongly typed values, so I don't have to worry about the values, even though I know they won't change.

Absolutely. That's why it makes sense to have access to the defaults of the SAS URI property, the container name, and the blob identifier property. I can trace how you'd be using that in your code if you need to access the SAS URI property/container/message property for blob name. What I don't see is how the default SAS token validation time is used?

It reduces complexity (albeit not much) in my code.

@sorenhansen, could you show please demonstrate how AzureStorageAttachmentConfigurationExtensions.DefaultSasTokenValidationTime would help with that?

@SeanFeldman Sorry, I didn't catch that you specifically meant DefaultSasTokenValidationTime

I agree that making DefaultSasTokenValidationTime public could be a bit much. It just made sense to me to make all default values public in case there would be a use case for them. In my particular case, I only need DefaultMessagePropertyToIdentitySasUri.

I guess you would rather leave DefaultSasTokenValidationTime internal for now, and then only expose it when it's absolutely needed?

Yeah, I've been trained over the time not to reveal anything that is not absolutely necessary. Once it's public, you need to worry about deprecation and migration. When it stays internal, can do whatever is needed with it.

I would suggest removing that bit only. It's still good to mention the default value in the documentation for readers to have an idea. Additionally, some XML documentation can be added to the code, on the parameter, mentioning the default value here:

ServiceBus.AttachmentPlugin/src/ServiceBus.AttachmentPlugin/AzureStorageAttachmentConfigurationExtensions.cs

Line 18 in 7dfff04

/// <param name="sasTokenValidationTime">The time blob SAS URI is valid for.</param>

@sorenhansen, will you do the update or should I?

@SeanFeldman I'll do the update. Just been busy the last few days 🙂

I plan on doing it later today

@SeanFeldman I've updated the code, making DefaultSasTokenValidationTime internal again, and documenting the default value in the AzureStorageAttachmentConfigurationExtensions.WithBlobSasUri method.

Thank you. I have reviewed and left a comment.
Sorry for nitpicking. Don't want to release your good changes without ensuring we're not causing more work to the users.

You're not at all nitpicking 🙂
When I made DefaultMessagePropertyToIdentitySasUri public, I was a bit unhappy with it being in another class than the other constants, so I'm glad you pushed me to refactor it.

I've updated the PR with your proposed changes. I've left DefaultSasTokenValidationTime in AzureStorageAttachmentConfigurationExtensions as it's still internal

- Document default value in constructor.

SeanFeldman · 2020-04-17T13:21:17Z

✅ Build ServiceBus.AttachmentPlugin 0.0.768-preview completed (commit a4315a9ecf by @)

SeanFeldman

While I've reviewed the code, I have realized that API wise it's awkward to have these constants that logically belong to the same group , to be spread across several types. This is what it looks from the user perspective:

Would be weird to access AzureStorageAttachmentConfigurationExtensions for one constant and AzureStorageAttachmentConfiguration for another two.

Perhaps those should be united under AzureStorageAttachmentConfigurationConstants in the root namespace Microsoft.Azure.ServiceBus? That way it's not hidden, will be side-by-side with the AzureStorageAttachmentConfiguration which is used anyways, and will be easily discoverable.

…figurationConstants class

SeanFeldman · 2020-04-17T16:40:49Z

✅ Build ServiceBus.AttachmentPlugin 0.0.769-preview completed (commit f12c5eeaf7 by @)

SeanFeldman · 2020-04-17T18:14:10Z

src/ServiceBus.AttachmentPlugin/AzureStorageAttachmentConfigurationExtensions.cs

-        internal static TimeSpan DefaultSasTokenValidationTime = TimeSpan.FromDays(7);
+        /// <summary>
+        /// Default time the blob SAS URI is valid for.
+        /// </summary>


This comment probably can go away

SeanFeldman · 2020-04-17T18:39:34Z

✅ Build ServiceBus.AttachmentPlugin 0.0.770-preview completed (commit acac856c1b by @)

SeanFeldman · 2020-04-17T20:35:33Z

Thank you, @sorenhansen!

Søren Hansen added 3 commits April 10, 2020 00:46

Make default configuration values publicly available as constants.

449cc61

Merge remote-tracking branch 'upstream/develop' into develop

bea8b19

Make default settings public, so they can be accessed from the outside.

fa41b10

sorenhansen requested a review from SeanFeldman as a code owner April 15, 2020 14:47

Add public settings to api

f567b14

SeanFeldman reviewed Apr 15, 2020

View reviewed changes

SeanFeldman added the enhancement label Apr 15, 2020

SeanFeldman added this to the 6.2.0 milestone Apr 15, 2020

- Revert DefaultSasTokenValidationTime to internal

0a7171a

- Document default value in constructor.

SeanFeldman requested changes Apr 17, 2020

View reviewed changes

Move default configuration constants to the AzureStorageAttachmentCon…

f5e6301

…figurationConstants class

SeanFeldman reviewed Apr 17, 2020

View reviewed changes

Removed superfluous comment

7a52fad

SeanFeldman self-requested a review April 17, 2020 20:17

SeanFeldman approved these changes Apr 17, 2020

View reviewed changes

SeanFeldman merged commit 2a22067 into SeanFeldman:develop Apr 17, 2020

sorenhansen deleted the public-default-settings branch April 17, 2020 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public default settings #191

Public default settings #191

sorenhansen commented Apr 15, 2020

SeanFeldman commented Apr 15, 2020

SeanFeldman commented Apr 15, 2020

SeanFeldman Apr 15, 2020

SeanFeldman Apr 15, 2020

SeanFeldman Apr 15, 2020

sorenhansen Apr 15, 2020

SeanFeldman Apr 15, 2020

sorenhansen Apr 15, 2020

SeanFeldman Apr 15, 2020

SeanFeldman Apr 16, 2020

sorenhansen Apr 17, 2020

sorenhansen Apr 17, 2020

SeanFeldman Apr 17, 2020

sorenhansen Apr 17, 2020 •

edited

Loading

SeanFeldman commented Apr 17, 2020

SeanFeldman left a comment

SeanFeldman commented Apr 17, 2020

SeanFeldman Apr 17, 2020

sorenhansen Apr 17, 2020

SeanFeldman commented Apr 17, 2020

SeanFeldman commented Apr 17, 2020

Public default settings #191

Public default settings #191

Conversation

sorenhansen commented Apr 15, 2020

SeanFeldman commented Apr 15, 2020

SeanFeldman commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenhansen Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

SeanFeldman commented Apr 17, 2020

SeanFeldman left a comment

Choose a reason for hiding this comment

SeanFeldman commented Apr 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanFeldman commented Apr 17, 2020

SeanFeldman commented Apr 17, 2020

sorenhansen Apr 17, 2020 •

edited

Loading