Add support for assembly attribute containing user secret id #525

natemcmaster · 2016-10-04T17:03:52Z

After vacillating between approaches, I think we've found a solution that will preserve functionality for project.json users and help us move to an MSBuild world.

This adds API for configuring user secret ID from an assembly attribute, UserSecretsIdAttribute.

When using .AddUserSecrets(), this method will first check Assembly.GetEntryAssembly() for the attribute. If no attribute is present, it will fallback to current behavior of reading project.json.

This also adds a code gen build task. Users can set the <UserSecretsId>xxxx-xxxx</UserSecretsId> property in MSBuild and it will generate UserSecretsIdAttribute on build.

NB: the target of this is a rel/1.0.1 release since we need MSBuild support in LTS.

Part of aspnet/DotNetTools#169

cc @Eilon @glennc @muratg

HaoK · 2016-10-04T17:38:43Z

src/Microsoft.Extensions.Configuration.UserSecrets/UserSecretsIdAttribute.cs

+        /// <summary>
+        /// The user secrets id.
+        /// </summary>
+        public string UserSecretsId { get; private set; }


nit: i believe you can just omit the private set; without any change in behavior

Eilon · 2016-10-04T20:21:04Z

src/Microsoft.Extensions.Configuration.UserSecrets/PathHelper.cs

@@ -14,6 +14,18 @@ public class PathHelper
        internal const string Secrets_File_Name = "secrets.json";
        internal const string Config_File_Name = "project.json";

+        /// <summary>
+        ///     <para>
+        ///     This method is obsolete and will be removed in a future version. The recommended alternative is Microsoft.Extensions.Configuration.UserSecrets.PathHelper.GetSecretsPath(string, string).


Can the "alternative" method be an actual doc link?

Eilon · 2016-10-04T20:21:56Z

src/Microsoft.Extensions.Configuration.UserSecrets/UserSecretsIdAttribute.cs

+        {
+            if (string.IsNullOrEmpty(userSecretId))
+            {
+                throw new ArgumentNullException(nameof(userSecretId));


ArgNull is only for null args.

So ArgumentException? ...we should probably clean up a few other places in our code where we do this. 😳

ArgException works, yes. Can you file a bug for other places where we throw the wrong exception?

Fixed it in aspnet/BasicMiddleware@dafe4cd 😄

Eilon · 2016-10-04T20:22:30Z

src/Microsoft.Extensions.Configuration.UserSecrets/project.json

  },
  "dependencies": {
    "Microsoft.Extensions.Configuration.Json": "1.0.0"
  },
  "frameworks": {
    "net451": {},
-    "netstandard1.3": {}
+    "netstandard1.5": {}


I forget. Is this ok? Or is this a breaking change?

It's not a breaking change. All currently supported netstandard platforms will continue to work.

Eilon · 2016-10-04T20:23:47Z

...ons.Configuration.UserSecrets.Test/Microsoft.Extensions.Configuration.UserSecrets.Test.xproj

@@ -14,5 +14,8 @@
  <PropertyGroup>
    <SchemaVersion>2.0</SchemaVersion>
  </PropertyGroup>
+  <ItemGroup>
+    <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />


Is this something important?

VS will keep adding this...

It's VS fluff that's required for Test Explorer. Other projects have it already..

Eilon · 2016-10-04T20:26:17Z

test/Microsoft.Extensions.Configuration.UserSecrets.Test/ConfigurationExtensionTest.cs

+        public void AddUserSecrets_FindsAssemblyAttribute()
+        {
+            var configKey = nameof(AddUserSecrets_FindsAssemblyAttribute);
+            SetSecret("Microsoft.Extensions.Configuration.UserSecrets.Test", configKey, "true");


Not sure I understand this test. What is "true" and "false" here? Is that the "user secrets id"? I.e. the "application name id" that I would use? If so, can we put a clearer dummy value here, e.g. "MyUserSecretsIdValue" or something?

The secret id and values aren't important to the test. I can make them dummy values if it makes the purpose of the test clearer.

Yeah I had to re-read this several times. The problem I had is that "true" and "false" have actual meanings. Changing these to be more meaningless, but perhaps also to be clearer about their semantic meaning, could certainly make this easier to understand.

Eilon · 2016-10-04T20:28:31Z

test/Microsoft.Extensions.Configuration.UserSecrets.Test/ConfigurationExtensionTest.cs

+
+            Assert.Equal("true", config[configKey]);
+
+            SetSecret("Microsoft.Extensions.Configuration.UserSecrets.Test", configKey, "false");


Why is this setting the value again? Is it trying to "undo" the change? Because if so, this isn't safe - the file could be left dirty if this test fails.

Eilon · 2016-10-04T20:28:47Z

test/Microsoft.Extensions.Configuration.UserSecrets.Test/ConfigurationExtensionTest.cs

+            var ex = Assert.Throws<InvalidOperationException>(() => builder.AddUserSecrets());
+            Assert.Equal(Resources.FormatError_Missing_UserSecretsIdAttribute(Assembly.GetEntryAssembly().FullName), ex.Message);
+
+            UserSecretHelper.DeleteTempSecretProject(projectPath);


Is this also cleanup code? This isn't reliable - the test might not get here.

...yeah, I know. I've already fixed up the 1.1 branch.

Eilon · 2016-10-04T20:30:43Z

...tion.UserSecrets/build/netstandard1.0/Microsoft.Extensions.Configuration.UserSecrets.targets

@@ -0,0 +1,44 @@
+<!--


This file is beautiful. I just wanted to say that 😄

XML is as much an art as it is a language. 🎨

Which begs the question: Why are .resx files so ugly? 😈

natemcmaster · 2016-10-04T22:17:34Z

🆙 📅

divega · 2016-10-04T18:54:25Z

src/Microsoft.Extensions.Configuration.UserSecrets/ConfigurationExtensions.cs

+                throw new ArgumentNullException(nameof(assembly));
+            }
+
+            var attribute = assembly.GetCustomAttribute<UserSecretsIdAttribute>();


Did you consider extracting common code between this method and the one above?

Sure, could be done, but as I was implementing it didn't seem like it would make it any cleaner.

Yeah I had a similar thought at first, but I couldn't come up with any meaningful way to refactor. The methods are similar, but there's not much actual common code.

I found one simplification: created a private method for constructing the exception message. I couldn't find other useful reductions.

Eilon · 2016-10-05T17:16:52Z

src/Microsoft.Extensions.Configuration.UserSecrets/UserSecretsIdAttribute.cs

@@ -19,7 +19,7 @@ public UserSecretsIdAttribute(string userSecretId)
        {
            if (string.IsNullOrEmpty(userSecretId))
            {
-                throw new ArgumentNullException(nameof(userSecretId));
+                throw new ArgumentException(nameof(userSecretId));


This now needs a proper message. We often have a Common_StringNullOrEmpty message that we use everywhere. Otherwise this exception has little meaning because it doesn't say what was invalid.

Updated to include error message.

Eilon · 2016-10-05T17:18:29Z

test/Microsoft.Extensions.Configuration.UserSecrets.Test/UserSecretsTestFixture.cs

+        {
+            while (_disposables.Count > 0)
+            {
+                _disposables.Pop()?.Invoke();


Pop() never returns null here, right?

In theory it could return null. In current implementation, it doesn't.

In theory lots of things can happen 😄 Why would it ever be null in any implementation? Presumably we'd only push stuff if there's an actual action to push? It's of course safe to leave it the way it is, though.

Fair enough. Updated :)

natemcmaster · 2016-10-05T22:12:58Z

cc @abpiskunov

dnfclas added the cla-already-signed label Oct 4, 2016

HaoK reviewed Oct 4, 2016

View reviewed changes

Eilon suggested changes Oct 4, 2016

View reviewed changes

divega reviewed Oct 4, 2016

View reviewed changes

Eilon suggested changes Oct 5, 2016

View reviewed changes

natemcmaster mentioned this pull request Oct 5, 2016

Create initial prototype of dotnet-user-secrets with MSBuild support aspnet/DotNetTools#178

Merged

5 tasks

Eilon approved these changes Oct 5, 2016

View reviewed changes

natemcmaster force-pushed the namc/usersecrets-1.0 branch from a6fc338 to 30572db Compare October 5, 2016 23:27

natemcmaster force-pushed the namc/usersecrets-msbuild branch from 6fd21d9 to ff2b07c Compare October 5, 2016 23:28

natemcmaster force-pushed the namc/usersecrets-1.0 branch from 30572db to d617138 Compare October 6, 2016 03:15

natemcmaster force-pushed the namc/usersecrets-msbuild branch from ff2b07c to 9ce21a6 Compare October 6, 2016 03:16

natemcmaster changed the base branch from namc/usersecrets-1.0 to rel/1.0.1 October 6, 2016 17:09

natemcmaster force-pushed the rel/1.0.1 branch from d617138 to 32e552d Compare October 6, 2016 17:42

Add support for assembly attribute containing user secret id

25d9ec8

natemcmaster force-pushed the namc/usersecrets-msbuild branch from 9ce21a6 to 25d9ec8 Compare October 6, 2016 18:30

natemcmaster merged commit 25d9ec8 into rel/1.0.1 Oct 6, 2016

natemcmaster deleted the namc/usersecrets-msbuild branch October 6, 2016 18:40

natemcmaster pushed a commit to aspnet/DotNetTools that referenced this pull request Oct 6, 2016

React to aspnet/Configuration#525

74a067f

natemcmaster mentioned this pull request Oct 6, 2016

Store UserSecretsId in MSBuild project file instead of project.json aspnet/DotNetTools#169

Closed

natemcmaster mentioned this pull request Nov 3, 2016

UserSecrets causes design-time tools to crash #543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for assembly attribute containing user secret id #525

Add support for assembly attribute containing user secret id #525

natemcmaster commented Oct 4, 2016 •

edited

Loading

HaoK Oct 4, 2016

Eilon Oct 4, 2016

Eilon Oct 4, 2016

natemcmaster Oct 4, 2016

Eilon Oct 5, 2016

natemcmaster Oct 5, 2016

Eilon Oct 4, 2016

natemcmaster Oct 4, 2016

Eilon Oct 5, 2016

Eilon Oct 4, 2016

moozzyk Oct 4, 2016

natemcmaster Oct 4, 2016

Eilon Oct 4, 2016

natemcmaster Oct 4, 2016

Eilon Oct 5, 2016

Eilon Oct 4, 2016

Eilon Oct 4, 2016

natemcmaster Oct 4, 2016

Eilon Oct 4, 2016

natemcmaster Oct 4, 2016 •

edited

Loading

dougbu Oct 5, 2016

natemcmaster commented Oct 4, 2016

divega Oct 4, 2016

natemcmaster Oct 4, 2016

Eilon Oct 5, 2016

natemcmaster Oct 5, 2016

Eilon Oct 5, 2016

natemcmaster Oct 5, 2016

Eilon Oct 5, 2016

natemcmaster Oct 5, 2016

Eilon Oct 5, 2016

natemcmaster Oct 5, 2016

natemcmaster commented Oct 5, 2016


		Assert.Equal("true", config[configKey]);

		SetSecret("Microsoft.Extensions.Configuration.UserSecrets.Test", configKey, "false");

Add support for assembly attribute containing user secret id #525

Add support for assembly attribute containing user secret id #525

Conversation

natemcmaster commented Oct 4, 2016 • edited Loading

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

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

natemcmaster Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Oct 4, 2016

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

natemcmaster commented Oct 5, 2016

natemcmaster commented Oct 4, 2016 •

edited

Loading

natemcmaster Oct 4, 2016 •

edited

Loading