-
-
Notifications
You must be signed in to change notification settings - Fork 837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attributed Metadata allowing overriding of metadata creation via an IMetadataProvider interface on metadata attributes #519
Conversation
I'm sorry it's taken a bit to address your request. I've been trying to think about how this would be used "in the wild" and how supportable such a thing might be. Once we take something in, we pretty much commit to having to support it for a really long time so it's important to get it right. Something I'm concerned about is the limited flexibility of the feature. The idea that it's limited to key/value pairs makes me think about the next logical step - supporting arbitrary objects with more hierarchical data. I'm curious if you could get the feature to work and allow for arbitrary generation of metadata. One idea I thought of is using a known interface to override the metadata scanning in attributes. This is just an idea. Might work, might not, but I figured I'd raise it. For example, you could have an interface like this:
You could use that to override the functionality of the
The The dictionary it returns would be added directly to the set of metadata in the The
You would have a slightly less clean set of attributes, but not by much:
The provider attribute could then scan the type using the other attributes and accomplish a slightly more flexible generation of metadata - you could include more than just name/value pairs; you could have a whole object hierarchy. You could even write hooks so existing attributes that aren't metadata attributes could still generate metadata - things like You'll probably notice I didn't put Again, this is one idea - one way to be a little more flexible with the feature. There are probably other ways, too, and we're totally open to those. Would you be interested in trying out a more flexible design? |
I spent some time tonight trying to make this a little more flexible. I liked the To get the effect that I wanted to achieve, I added a The next change I made I feel like is a little more controversial and I could see it being a little less acceptable since it involved a change to the core library. As I was doing all the metadata grouping stuff, I found myself having to re-implement the
Example With this change, the metadata can be fully recursive, populating strongly typed objects, or appearing as a recursive So now, metadata which looks like this: [AttributeUsage(AttributeTargets.Class, AllowMultiple=true)]
public class PersonAttribute : Attribute, IMultipleAttributedMetadata
{
public PersonAttribute(string name, int age)
{
Name = name;
Age = age;
}
public string Name { get; private set; }
public int Age { get; private set; }
}
[MetadataGroup("People", typeof(PersonAttribute))]
[Person("Alice", 42)]
[Person("Bob", 27)]
class PersonGroup { }
class PeopleMetadata
{
public Person[] People{ get; set; }
public class Person
{
public string Name { get; set; }
public int Age { get; set; }
}
} Can be accessed either like this: var metadata = MetadataHelper.GetMetadata(typeof(PersonGroup ));
var nested = metadata.Where(p => p.Key == "People").Select(kv => kv.Value).FirstOrDefault() as IDictionary<string, object>[];
var names = nested.Select(n => n["Name"]);
var ages = nested.Select(n => n["Age"]); Or like this: ...
var container = builder.Build();
var withMetaObject = container.Resolve<Meta<PersonGroup, PeopleMetadata>>();
var alice = withMetaObject.Metadata.People.Where(a => a.Name == "Alice").SingleOrDefault();
var bob = withMetaObject.Metadata.People.Where(a => a.Name == "Bob").SingleOrDefault(); I feel like this is more flexible than last week's iteration because:
Sorry that was a little lengthy. I wanted to try to fully explain what I did so that its a little easier to review the changes I made. I feel my latest changes, while certainly making this much more flexible, are a little more far-reaching since it affects the core metadata library. While all the tests still pass, I was wondering what the general guidelines for writing tests were. Identifying all of the test cases needed has never been my strong suit and I feel like I should have more than just one or two tests to test my change to the Thanks for taking the time to look at this. Any further suggestions and comments are greatly appreciated. |
I admit I got a little lost while looking at the changes in some cases. There's some complexity here that I'm curious if we could reduce. Thinking about the grouping feature... I'm wondering if that's something that Autofac should provide as part of the framework, or if maybe that's something you might keep in your code. As a framework, it's good to provide the flexible building blocks to support people to create what they need to, but it's also good to find that balance between providing not enough (lack of flexibility) and providing too much (features that might be used by very few people but are things we have to commit to owning long-term). You can find some examples of these sorts of things if you browse StackOverflow. There are some great questions about things like how to conditionally apply decorators based on configuration values and that's something we could offer in the framework... but it's a fairly complex feature that not a lot of people would use. Instead, we provide the ability for others to create it, and if we see there are a lot of people posting about how to do XYZ or asking about it in places, then it might be something to consider adding. Thinking about the changes to Core... It'd be nice if we could keep the changes to the attribute metadata assembly. There's a whole metadata infrastructure in Core that supports metadata and metadata relationships in a certain way. The intent of the extras assembly is to enable interaction with that system via attributes. If we're doing something that requires the change to the core assembly (more than, say, a one-liner sort of thing) it might indicate the integration has gone a little far. In that case, it would be good to step back and figure out what the overall shortcomings of the metadata feature in Core Autofac are so we can figure out whether the changes are warranted. (Changes in Core are sort of scary because things that seem tiny and still allow tests to pass can have really big effects. See Issue #397 for a great example of me making some seemingly innocuous changes and causing some pretty bad memory leaks.) Thinking about the recursive scanning of nested classes... I think this is interesting, but I'm curious how often it'd be used. It appears that doing this is part of what causes the complexity, and I'm wondering if it wouldn't be better to potentially let the |
I see your point. As I was making the changes to create the If I were to just include the If that is a better option and that seems like a more viable (and slimmer) candidate for inclusion, I can re-do the changes to just include that and force push to this pull request, or I could create a new pull request. |
I think that'd probably be good. Small, simple, not too complex, and enough that you can get done what you want to get done while also providing something fairly flexible for other folks. Thanks again for your work on this. Great stuff! |
Ok, I have packaged up and squashed just the |
This is looking nice and clean. Sweet! Two minor things and I'm ready to click the Merge button.
Again, thanks for the great work on this. Very cool. |
Any MetadataAttribute which also implements IMetadataProvider will have its GetMetadata method called to create its metadata rather than having its properties scanned.
Alright, I believe that should do it. I changed the return type of |
Attributed Metadata allowing overriding of metadata creation via an IMetadataProvider interface on metadata attributes
Looks great. Thanks again! I'll update the version of the assembly/package and see if I can get a new version pushed shortly. |
This will go out in version 3.2.0 of Autofac.Extras.Attributed. It's running the build now. |
3.2.0 just went live. |
I found in a project I was working on that I wanted to have sequences of groupings of data defined by metadata attributes.
For example, let's say we had an object which always represents a specific group of People. We could have the object manually say that it represents those people using a Property, but then each instance would essentially be providing the exact same data which says to me that it needs to use some sort of Metadata facility. I really like attributed metadata, so I make an attribute called
PersonAttribute
which has aName
andAge
property and is allowed to be applied multiple times to a class. However, when I register this object, anArgumentException
results since two Name values and two Age values were defined and it fails when they are added to the MetadataIDictionary
.What I have done is modified the Autofac.Extras.Attributed library to do the following:
IDictionary
will be anT[]
where T is the type of the Metadata Value. Values with the same Key but different Types will cause anArgumentException
to be thrown. Order is preserved among groups of attributes (as will be illustrated in my example).IDictionary
will be T where T is the type of the Metadata Value (just like it always has been).Example to illustrate how this could be used
When
PeopleGroup
is resolved with its metadata (represented byPeopleMetadata
here to illustrate the type that the metadata values are resolved to), it will place the values forName
andAge
into the two arrays. So,Age
would contain either[42,27]
or[27,42]
andName
would contain either["Alice","Bob"]
or["Bob","Alice"]
. Now, the problem is keeping the association between whichName
goes with whichAge
. To solve this, the metadata values coming out of the attributes are kept in groups according to which attribute they came from until they are "consolidated" into eitherKeyValuePair<string, T>
orKeyValuePair<string, T[]>
which both are stored in the MetadataIDictionary
asKeyValuePair<string, object>
(as it always has been done). So long as the number of values in each key of a group is the same, when the groups ofKeyValuePair
s are consolidated intoT[]
the ordering will be preserved as the variousT[]
s are constructed. So, ifName[0] == "Bob"
thenAge[0] == 27
. Similarly, ifName[0] == "Alice"
thenAge[0] == 42
.I use Autofac on a daily basis and I think something like this could be useful for people who use (or perhaps overuse 😛, like myself) Metadata via attributes. I know this new feature likely wouldn't be compatible with other libraries that use
MetadataAttributeAttribute
stuff to define their metadata since I don't know how they would handle multiple decorations with the same metadata attribute, but I wrote this mainly with Autofac in mind. It doesn't change any existing functionality as far as I can tell and those who need to have compatibility for other libraries can simply not useAllowMultiple
-enabled metadata. I hope this will be considered for inclusion and all existing tests pass as well as some additional tests I added for this new functionality.Suggestions for improvement and comments are appreciated