dotliquid / dotliquid

.NET Port of Tobias Lütke's Liquid template language.
http://dotliquidmarkup.org
Other
1.06k stars 297 forks source link

Making safe/drop as root type? #88

Open AaronLS opened 10 years ago

AaronLS commented 10 years ago

If I inherit form Drop or RegisterSafeType, can an instance of that object be the root of the liquid model? In other words, as far as I can tell you have to do this:

Template template = Template.Parse("hi {{Name}}, {{OtherField}}"); template.Render(Hash.FromAnonymousObject(new { Name = instanceOfSomeDrop.Name, OtherField = instanceOfSomeDrop.OtherField, //... umpteen other fields }));

This isn't viable in a generic implementation where I'm trying to reuse some code without it being specific to what fields might be in the Drop.

OR reqire template designers to use dot notation(this may seem trivial to us, but it is certainly a learning curve for non programmers).

Template template = Template.Parse("hi {{Data.Name}}, {{Data.OtherField}}"); template.Render(Hash.FromAnonymousObject(new { Data = instanceOfSomeDrop }));

My hope is that if something is a drop or registered safe type, I can pass it directly without nesting it inside an anonymous object and have the field references in that tempalte be rooted at the object being passed in:

Template template = Template.Parse("hi {{Name}}, {{OtherField}}"); template.Render(Hash.FromObject(instanceOfSomeDrop)); //throws exception if not Drop or safe type

Of course depending on the object structure, assuming nested types are also drops/safetypes, you might have template references like {{Supervisor.Name}} but the point being is that you eliminate one redundant level of dot notation without requiring hardcoding the properties in an anom object by allowing Hash.FromObject. In my case I am using compeltely flat objects and thus the user will never need to use dot notation for simple templates.

Even if you decided to make it something like Hash.FromDropObject(instanceOfSomeDrop) and only allowed Drops to be root objects, not allowing registeredsafetypes, just to enforce some compile time restriction on what can be passed in, I would be grateful. I don't think that would really make sense IMO though, as currently there isn't really any compile time safety with Hash.FromAnonymousObject since any of the properties could be mapped to non-safetypes/drops, and thus you only know until runtime if you're doing something bad. So given that line of thinking, I would think Hash.FromObject for any type would be no more risky than Hash.FromAnonymousObject, since in both cases you would be imposing the same runtime requirements.

Thanks.

tgjones commented 10 years ago

The approach I'd probably take is this:

Dictionary<string, object> myDictionary = new Dictionary<string, object>();
foreach (PropertyInfo myProperty in objectInstance.GetType().GetProperties())
  myDictionary[myProperty.PropertyName] = myProperty.GetValue(objectInstance, null);
Hash localVariables = Hash.FromDictionary(myDictionary);
template.Render(localVariables);

(I haven't run that through a compiler so I apologise in advance for syntax errors.)

I guess the Hash.FromObject method that you proposed would be implemented something like this. Do you think it should be added to DotLiquid? I think it's straightforward enough, and also there's enough potential for users to want to customise how it works, that it doesn't need to be in DotLiquid, but I'm interested in your opinion.

AaronLS commented 10 years ago

I do think it'd be a very useful addition. So far the use cases for us have been that we create a strongly typed object for each scenario we'd let users define a template for, each being in a different context so certain data is made available. The data layer populates the object and once it gets back to the layer where we render the template, we already have a strongly typed object that we've been explicit in inheriting from Drop on.

I think of it much like View and ViewModels in MVC, here's this view(the template) and here is the viewmodel(the strongly typed object), except that the template can be rewritten by different users, so the viewmodel is applicable to many use cases. so I'm going to make the code that populates that viewmodel common and reusable to those scenarios. And there will probably be different viewmodels for contexts that differ enough. If I'm restricted to only using an anonymous object, then I'm forced to hold on to and pass around a bunch of different pieces of data through the framework until the last possible moment that I need to compose them together into an anonymous object.

I took a look at HashFromAnonymousObject and see that it is nearly identical to the reflection technique you described. The reason it was failing for me was actually because I had inherited from Drop. Drop contains an index property, and GetProperty(...,null) throws an exception on that property.

If I remove Drop from the root object, I can do this and it works fine!

class Dog
{
    public string Name { get; set; }    
}

Dog animal = new Dog{Name ="bob"};
Template template = Template.Parse("hi {{Name}}");
string output = template.Render(Hash.FromAnonymousObject(animal));

So I assumed(perhaps incorrectly) that your design of FromAnonymousObject was intended to force users of your API to be "safe by default"(which I can agree with) in that they'd have to be very conscious and explicit of what data they added to the anonymous object, and then any nested properties would need Drop/RegisterSafe, or similar. However, it seems after all, what I want to do is perfectly fine, so long as my root object doesn't inherit from Drop(or implement an index property), and all nested child objects would inherit from Drop as usual.

So all in all I think you actually have a bit of an issue if that's your intention, as there is no check on the root object to ensure it is anon. or implements Drop/RegisterSafe etc.

My suggestion would be to keep FromAnonymousObject as it is supporting strongly typed object and modify it to: -If type is not anonymous, then perform same safety checks you perform on properties, ensuring the passed type implementing Drop or similar -Exclude properties from the inherited Drop type. Calling GetValue(,null) throws exception on the index proeprty and of course you don't want those properties accessible from the Liquid template.

This might be breaking change though, because someone else like myself may have discovered this already, and am passing a strongly typed object which does not inherit from Drop.

Then it supports both, which I don't see any reason why it shouldn't other than it should be probably named "FromObject" since both strong types and anon. are both "objects" and both supported by that method. It'll have to stay named as-is for backwards compatibility, but maybe add another method that calls into the same code just so people realize it is supported. And maybe one day across some major version revision you can mark the old one as deprecated and wait a few versions to remove the old method name.

Thanks. I have what I need though. Take care.

schotime commented 10 years ago

I've been using these too methods to accomplish the same thing.

public static Hash FromObject(object obj)
{
    Hash result = new Hash();
    if (obj != null)
        foreach (PropertyInfo property in obj.GetType().GetProperties())
            result[Template.NamingConvention.GetMemberName(property.Name)] = property.GetValue(obj, null);
    return result;
}

public static void RegisterSafeType(Type type)
{
    if (Template.GetSafeTypeTransformer(type) != null)
        return;

    foreach (PropertyInfo property in type.GetProperties().Where(x => x.PropertyType.IsClass && x.PropertyType != typeof(string)))
    {
        RegisterSafeType(property.PropertyType);
        Template.RegisterSafeType(property.PropertyType, x => new DropProxy(x, x.GetType().GetMembers(BindingFlags.Instance | BindingFlags.Public).Select(y => y.Name).ToArray()));
    }
}
haf commented 8 years ago

I think these functions and this functionality would be a very welcome addition to dotliquid. Would be great to get PRs for them.

AaronLS commented 8 years ago

@haf Did you intend to close this?

haf commented 8 years ago

@AaronLS I'm afraid so, yes. I'm trying to keep the issue tracker actionable and it's not clear what this issue is really tracking. I've set it up so that every merge to master releases a new version, too. So keep the PRs coming! :)

AaronLS commented 8 years ago

@haf "it's not clear what this issue is really tracking" The issue being tracked is that you cannot use a concrete type inheriting Drop as a root type. The solution would be to add something like a Hash.FromDrop/FromObject similar to schotime's recommendation that allows this. I think that's a pretty focused requirement definition so I think it is actionable, it's just a matter of someone taking action. In a couple months I should be done with my current project and can get setup to make a contribution. Somehow everytime I try to update a GIT repo from upstream I manage to do something wrong and not have a proper history to do a clean pull request.

The below works( workaround for anyone reading this ). Passing a concrete type works as long as it does not inherit from Drop. If the below was class Dog:Drop then it will not work. It's just very counter intuitive because I never guessed it would work given the name FromAnonymousObject implies only anonymous objects are allowed. In every use case I've had for dotliquid, we've needed to use concrete types as the root type.

class Dog
{
    public string Name { get; set; }    
}

Dog animal = new Dog{Name ="bob"};
Template template = Template.Parse("hi {{Name}}");
string output = template.Render(Hash.FromAnonymousObject(animal));
AaronLS commented 8 years ago

I've been working on this. The one problem with passing a Drop for example, is it contains an indexer property. FromAnonymousObject throws an exception when encountering this property. 1) The error is misleading. 2) The larger issue is reflecting over a Drop or similar IIndexible will expose properties not intended to be exposed.

I am going to make FromAnonymousObject check for IIndexeable, and throw a more specific informative exception in this case.

Note that FromAnonymousObject has always allowed concrete non-anonymous types. A workaround has been to have root types which don't inherit from Drop to avoid the exception. So I am not changing that behavior. FromAnoynmousObject will still fail when passed a Drop, and will still succeed when passed a concrete type which is not a Drop. This is the same behavior in place now. The only difference is the exception will be more informative. To tighten FromAnonymousObject to truly only allow anonymous objects would break code for anyone using this workaround. So I am not changing that.

I am adding FromObject which conveys to users that objects other than anonymous objects are allowed. Should users truly want to pass a regular concrete type, including Drops, this would be the method for them instead of FromAnonymousObject.

I check to see if the object implements IIndexable. To properly respect Drops and IIndexable objects, I iterate over reflected properties, and check IIndexable.ContainsKey before adding to Hash, ensuring that only properties intended to be exposed make it into the Hash.

Other objects would simply be reflected over in the same manner anonymous objects are reflected. I will probably need to exclude some special compiler generated properties such as those for operators.

Thus FromObject will essentially handle any of the above. Regular concrete types, anonymous types, Drops, IIndexable... these are all strictly speaking objects. If you are explicitly passing an object to FromObject, it is understood you trust exposing its properties. If it implements any of the dotliquid indicates to restrict properties within the object though, we want to respect that. Thus why we check IIndexable.ContainsKey before hashing.

The challenge I am seeing is that RenderParameters/Hashes are really a different approach from how the rest of the object graph is handled. For example, IIndexable does not expose IEnumerable. The index operator alone cannot be enumerated. I must iterate over properties and check IIndexable.ContainsKey(). This works, but there are other cases that isn't so simple. This doesn't really respect an implementation of BeforeMethod for example. Essentially the capability available to any object that appears as a property beyond the root type has alot more behavior than the root type does. Converting a Drop to a Hash or RenderParameters loses some functionality such as BeforeMethod, and thus in some edge cases the way you access it via liquid will be inconsistent when the type appears in the root of the object graph versus elsewhere deeper in the object graph.

Another edge case is where IIndexable exposes something that isn't even a property. There could be a key that the index operator is written to recognize that isn't a property on the object. So the approach of iterating over the properties and checking .ContainsKey will not work. You can't enumerate an index operator, so I can't discover the keys in a way that let's me convert them to a hash. My approach would make that key inaccessible when the object is passed as the root object.

So while my first pass at FromObject would give users alot more options, it would lead to some inconsistencies in certain edge cases that will most certainly be very confusing and inconsistent.

Having observed these details, to me, it seems like the more proper approach is to have another .Render overload that doesn't require a hash or Render Parameters. Context.Liquidize is already capable of handling all of the permissible types. Render just doesn't allow you to pass the full set of permissible types, because it forces you to convert to an enumerable Hash or RenderParameters. It doesn't seem like there's any reason that a liquid reference to a root property would need to behave different.

I am getting a little tripped up trying to understand some of the internal plumbing though to try and figure out best way to go about this.

Maybe it's not important, but why does IIndexable use objects as keys instead of strings? When I poke around I see unsafe casts to string, so it is seemingly assumed they are always strings, and you'd certainly get exceptions on failed casts if anyone ever used something besides a string. Not challenging, just feels like I'm missing something.