Stop special-casing StorageAccountAttribute. #1056

MikeStall · 2017-03-11T00:36:38Z

Make the storage attributes (Blob,Table,Queue) derive from StorageAccountAttribute. This gives Blob* attribute a Connection property, just like all the other attributes.
This is not a breaking change - and no change to public tests.

The key is in TypeUtility's new GetAttr function. It still walks the hierarchy and collapses attributes into a single attr.

The motivation here is:

Stop special-casing blob storage. All other resources need something similar here.
Provides a 1:1 between Attributes and Function.json that lets us removes corner cases in Script and tooling

Make the storage attributes (Blob,Table,Queue) derive from StorageAccountAttribute. This gives Blob* attribute a Connection property, just like all the other attributes. This is not a breaking change - and no change to public tests. The key is in TypeUtility's new GetAttr function. It still walks the hierarchy and collapses attributes into a single attr. The motivation here is: 1. Stop special-casing blob storage. All other resources need something similar here. 2. Provides a 1:1 between Attributes and Function.json that lets us removes corner cases in Script and tooling

mathewc · 2017-03-13T21:31:54Z

src/Microsoft.Azure.WebJobs.Host/TypeUtility.cs

+            // higher up in the hiearchy
+            ICustomAttributeProvider provider = parameter;
+
+            while (provider != null)


There is already an existing GetHierarchicalAttributeOrNull that was doing this walk up from parameter/method/class. Why the new code for this? Recommend we just use what we already have - it is fully tested, etc.

Also the naming makes it clear from the caller that a hierarchical walk for the attribute will be performed. This isn't clear from "GetAttr"

GetHierarchicalAttributeOrNull doesn't handle base classes. We do leverage existing tests on the new code. I can probably remove GetHierarchicalAttributeOrNull so that there's only one copy of this.

Just to help clarify - this will hierarchically merge the properties of all found attributes which are of type TAttribute or a base class of TAttribute?

As a more concrete example, GetAttr merges StorageAccountAttribute as well as TableAttribute (because TableAttribute : StorageAccountAttribute)

mathewc · 2017-03-13T21:32:57Z

src/Microsoft.Azure.WebJobs.Host/TypeUtility.cs

+                                if (currentValue == null)
+                                {
+                                    var value = prop.GetValue(attr);
+                                    prop.SetValue(attributeSource, value);


Why is this getter function setting things?

We have to merge the properties together into a single attr. Say we have:
[Blob] [Storage(account="X"]
We need to set Blob.account = x

I think we need to come up with a better way to do this. This attribute merging is hacky, and isn't a pattern we want to set in stone like this. We're not going to promote a pattern of attributes deriving from another attribute, with base or derived instances found up and down the hierarchy.

That's the heart of the proposal... We started down this path as soon as we split out a single attribute into multiple attributes. We need some means to associated the different attributes together.

mathewc · 2017-03-13T21:42:57Z

src/Microsoft.Azure.WebJobs/QueueAttribute.cs

@@ -26,7 +26,7 @@ namespace Microsoft.Azure.WebJobs
    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1813:AvoidUnsealedAttributes")]
    [AttributeUsage(AttributeTargets.Parameter)]
    [DebuggerDisplay("{QueueName,nq}")]
-    public class QueueAttribute : Attribute
+    public class QueueAttribute : StorageAccountAttribute


The "is a" relationship here kind of doesn't make sense. If we had a StorageAttribute (rather than StorageAccountAttribute) it does make more sense. QueueAttribute "is a" StorageAttribute. What if we just introduced this new StorageAccount base type and used that? That's a cleaner type model I think.

We'd then leave StorageAccountAttribute as is for back compat (as well as method/class level specification), and adjust the GetStorageAccountAsync logic accordingly.

Ideally, we want (and I think these are in priority order):

No breaking changes

no redundant attributes (having StorageAccount and Storage would be confusing and is unnecessary from a binding perspective)

good names

I don't think we can get all 3. The current PR does 1&2.

Adding the new StorageAttribute base class would be non breaking. All the types make sense together, and in the next major version, we can simply remove the Parameter level usage from StorageAccountAttribute - it then stays around for class/method level overrides.

I'm thinking of the future when we might have more "storage" common properties. Those would make sense to add to a StorageAttribute base class, but not a StorageAccountAttribute class - the naming there is specifically for holding a single bit of info - the account name. StorageAttribute base class sets us up nicely for future extensibility.

I think it violates #2 ... a separate Storage and StorageAccount attribute seems confusing. What's the difference between them? What's the customer value in having 2 separate attributes?

The value is to us as framework authors. We have a base attribute class that is extensible and makes more sense from a type perspective. StorageAccountAttribute is for specifying a storage account name value - that's why I named it the way I did. It was not designed as a base class - only to specify a single account name. StorageAttribute (abstract base class) is the base class for all storage binding attributes.

This won't be confusing for our users - the base class is abstract and they won't even think about it.

Does StorageAccount : Storage; Blob : Storage? What's a specific difference between Storage and StorageAccount?

mathewc · 2017-03-15T21:19:30Z

src/Microsoft.Azure.WebJobs.Host/TypeUtility.cs

+                var attrs = provider.GetCustomAttributes(typeof(Attribute), false);
+                foreach (var attr in attrs)
+                {
+                    if (attributeSource == null)


It's not clear to me at all what this code is doing. Some good code comments in this method would go a long way. What it looks like to me is that if we're processing say GetAttr, if it wasn't found on the ParameterInfo initially above, you're looking for it on the Method/Class as you walk up. That doesn't make sense - our binding attributes are only parameter level attributes.

mamaso · 2017-03-14T22:05:05Z

src/Microsoft.Azure.WebJobs.Host/TypeUtility.cs

+            // higher up in the hiearchy
+            ICustomAttributeProvider provider = parameter;
+
+            while (provider != null)


Just to help clarify - this will hierarchically merge the properties of all found attributes which are of type TAttribute or a base class of TAttribute?

As a more concrete example, GetAttr merges StorageAccountAttribute as well as TableAttribute (because TableAttribute : StorageAccountAttribute)

mamaso · 2017-03-14T22:17:53Z

src/Microsoft.Azure.WebJobs.Host/TypeUtility.cs

+                        continue;
+                    }
+
+                    if (attr.GetType().IsAssignableFrom(typeof(TAttribute)))


I think this could benefit from more comments or splitting this function up into multiple pieces to help with readability (basically, separate the walk step from the merge step)

ahmelsayed · 2017-03-21T22:15:11Z

doesn't ServiceBusAccountAttribute.cs have similar special handling?

dnfclas added the cla-already-signed label Mar 11, 2017

ahmelsayed added the in progress label Mar 11, 2017

mathewc reviewed Mar 13, 2017

View reviewed changes

mathewc reviewed Mar 15, 2017

View reviewed changes

mamaso reviewed Mar 15, 2017

View reviewed changes

mathewc mentioned this pull request Mar 23, 2017

Adding Account properties onto base Azure Storage binding attributes #1084

Merged

mathewc closed this Mar 29, 2017

ahmelsayed removed the in progress label Mar 29, 2017

MikeStall deleted the account branch July 20, 2017 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop special-casing StorageAccountAttribute. #1056

Stop special-casing StorageAccountAttribute. #1056

MikeStall commented Mar 11, 2017

mathewc Mar 13, 2017 •

edited

Loading

mathewc Mar 13, 2017

MikeStall Mar 13, 2017

mamaso Mar 14, 2017

MikeStall Mar 15, 2017

mathewc Mar 13, 2017

MikeStall Mar 13, 2017

mathewc Mar 15, 2017 •

edited

Loading

MikeStall Mar 15, 2017

mathewc Mar 13, 2017 •

edited

Loading

MikeStall Mar 13, 2017

mathewc Mar 13, 2017 •

edited

Loading

MikeStall Mar 13, 2017

mathewc Mar 14, 2017

MikeStall Mar 14, 2017

mathewc Mar 15, 2017 •

edited

Loading

mamaso Mar 14, 2017

mamaso Mar 14, 2017

ahmelsayed commented Mar 21, 2017

Stop special-casing StorageAccountAttribute. #1056

Stop special-casing StorageAccountAttribute. #1056

Conversation

MikeStall commented Mar 11, 2017

mathewc Mar 13, 2017 • 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

mathewc Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Mar 13, 2017 • 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

mathewc Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmelsayed commented Mar 21, 2017

mathewc Mar 13, 2017 •

edited

Loading

mathewc Mar 15, 2017 •

edited

Loading

mathewc Mar 13, 2017 •

edited

Loading

mathewc Mar 13, 2017 •

edited

Loading

mathewc Mar 15, 2017 •

edited

Loading