Avoidance of KV-Exceptions

[Objective-Cloud]

First of all: Great work on GRMustache. We have integrated GRMustache deeply into our open source framework for web development at Objective-Cloud. We are using it in production and for our own "applications" that are running in the cloud. However there is one problem which causes a lot of problems: The NSUndefinedKey-exceptions that come up.

In general, a thrown exception (even if it is caught), is not nice. This is especially true for something that is running in the cloud (all the time). Every thrown exception causes memory leaks. This could be used by an attacker: She could constantly request the rendering of a template that is known to cause lots of exceptions (internally). In the end of the day the app will use more and more memory.

There is a workaround: The compiler (more precise: the ARC-subsystem) can be told to generate exception-safe code. The downside is that the code generated by ARC becomes a lot bigger.

The other workaround is, as documented, to call

[GRMustache preventNSUndefinedKeyExceptionAttack];

The documentation says, this should be for debugging only and I understand why: It is swizzeling valueForUndefinedKey: which is a hack in and of itself in my opinion.

Another workaround is (and that is what we have done for our own applications) is to use mustache in a way so that no undefined key exception is thrown: So we don't really use the context stack-feature. This works but sometimes it is "hard" to do certain things because the context stack is such an integral feature of mustache.

I have thought about a proper solution for this problem. I would like to describe my solution and make hereby a request for comments. If you think that my solution is okay then I would go ahead and implement it.

The solution:

The main problem is, that you call -valueForKey: and you don't really know if the key is defined for the object in question. You are not to blame for that since this is/was the only viable solution up until a few months ago. In my opinion GRMustache is the perfect use-case for keyed-subscripting.

Here is how it should work in my opinion:

1. Before GRMustache is calling -valueForKey on an object it should check to see if the following is true: [object respondsToSelector:@selector(objectForKeyedSubscript:)]. In other words: Do not call -valueForKey: if the object supports keyed-subscripting.
   1. (object does not support keyed-subscripting): Call -valueForKey: and we are done (backwards compatibility)
   2. (object supports keyed-subscripting): Call objectForKeyedSubscript:.
2. (objectForKeyedSubscript: was called) 1 returned something non-nil: Use the return value as the resulting value and render it. 2 returned nil: Go up in the stack and repeat the whole procedure.

Rationale:

objectForKeyedSubscript: is often implemented in a way that does simply return nil if the key was not found. A developer can add GRMustache support to his custom class by simply implemented keyed-subscripting which is trivial in most circumstances.

What do you think?

[groue]

I think that the problem is real, and it has been floating around for a while.

I like that you put objectForKeyedSubscript: on the table. My friends at Fotonauts use it when fetching keys for their Handlebars implementation (a Mustache derivative): https://github.com/fotonauts/handlebars-objc/blob/master/doc/ContextObjects.md#why-limiting-the-attributes-accessible-using-key-value-coding-

Would you please read this article first, and check if their solution would be OK for you? That's because I've been considering going their way.

[Objective-Cloud]

I have read the article. Thanks for pointing me to it.

Their proposal is exactly what I would do if I were you - with a few minor modifications. Please tell me if you want me to elaborate.

[groue]

I noticed the Handlebars article because it was talking about security (rendering unsafe templates) - I only realize now that it could also address the KVC exceptions.

Two subjects in this response.

1) How bad are KVC exceptions in Mustache rendering?

Historically, for me, KVC exceptions in Mustache rendering are not that much of a big deal.

AFAIK, the main downside of catching exceptions is the potential leaking of memory. But we're talking of KVC exceptions here. And most usually, KVC exceptions happen in NSObject's implementation. And AFAIK again (testing with Instruments.app), this implementation does not leak.

So KVC exceptions raised by GRMustache should not harm anybody, except people who provide their own (leaky) implementation of valueFor[Undefined]Key. And that's why I try to hint the user by aboundantly documenting those exceptions.

[GRMustache preventNSUndefinedKeyExceptionAttack] only targets development phase, when the debugger is said to stop on every exception - which is a chore.

So, it would nice of you if you had time to elaborate on the very reason KVC exceptions would be inherently bad.

2) GRMustache is a tool, not a framework

A templating library is like a JSON parser/encoder, a HTML processor, a hammer or a screw driver : a convenience tool nobody wants to dedicate much brain to. It should just work out of the box, without requiring much setup. All advanced GRMustache features exist only for solving tricky situations. There is nothing advanced or complex to do for plain rendering, thanks to valueForKey:: type a Mustache {{ tag }}, get the rendering without modifying any class, and go to the next item on the todo list.

I'd like to keep this spirit.

[Objective-Cloud]

Hello again,

With regards to 1):

I think the question you are asking (how bad are exceptions in Mustache rendering?) is "wrong". I think it is fair to say that if the source code of an application/framework/tool/whatever contains "@try" and "@catch" there is something not right... This was true before ARC and is still true with ARC.

As I wrote in my first comment here: I see nothing wrong with using -valueForKey: either. But now that there are better alternatives I, personally think that we/you should make use of these alternatives.

I have not profiled the code around the try-catch block. Let me quote the clang documentation instead:

"The standard Cocoa convention is that exceptions signal programmer error and are not intended to be recovered from. Making code exceptions-safe by default would impose severe runtime and code size penalties on code that typically does not actually care about exceptions safety. Therefore, ARC-generated code leaks by default on exceptions, which is just fine if the process is going to be immediately terminated anyway. Programs which do care about recovering from exceptions should enable the option."

Source: http://clang.llvm.org/docs/AutomaticReferenceCounting.html#exceptions

So even if you made sure that there are no leaks, I personally still consider this a hack. A hack is not bad if there are no better alternatives but now there are better alternatives...

With regards to 2:

I see your point. But always remember: My proposal still has valueForKey: as a fall back. So if a programmer does nothing he gets the same behaviour GRMustache provides currently. The implementation of my proposal are approximately 10 lines of code. The proposal you linked requires more work but it is also more advanced.

What do you think about the following suggestion: Let me send you a pull request. This request will contain very little changes. It will be 100% backwards compatible. It will allow developers to make their code "exception" free with very little effort. Then you can try "my" version of GRMustache and if you like it simply merge my changes. What do you think?

[groue]

The method you'll want to modify is +[GRMustacheKeyAccess valueForMustacheKey:inObject:], isn't it? I'll look, and very much probably merge your pull request, now that I better understand your point, and you plans :-)

Thanks for your dedication!

[groue]

@Objective-Cloud I took the opportunity to complete a work in progress regarding thread-safety (issue #65). Now the latest version is v6.8.4: this should be the base of your contribution. Please accept my apologies for pulling the rug from under your changes.

[groue]

@Objective-Cloud I'm on it :-)

[groue]

@Objective-Cloud Here you go: v6.9.0 has support for objectForKeyedSubscript:, just as you suggested. Thanks a lot for the contribution :smiley:

[groue]

I close this issue. Let me know if something is still wrong. Thanks for reporting your trouble, @Objective-Cloud !

[anegmawad]

Hi groue!

First of all I want to introduce myself. I'm "the other developer" of Objective-Cloud. The above comments are written by Chris, not by me. I hope this will not lead to confusions. :-)

The solution you and Chris elaborated has a problem: Implementing keyed subscript turns off the context stack. I do not know, why Chris said that we do not need it, but this is not true. At least he cannot know, because the feature can be used by our customers.

Let's go back to the very beginning. You faced the problem that a key can be a key in the current context or in a super-context. You try to find out, whether it is valid in the current context, if not, you go up. This is the basic strategy. Did I get it right?

Using KVC there is no possibility to find out, whether a key is appropriate for the current context. (Man, more than one time I wished a method like -validKeys in KVC …) So you do, what you have to do: You try it and KVC throws an exception, if it is unknown. If you catches one, you get one step up.

Chris came around with the idea to offer a keyed-subscript implementation that does not throw an exception. This is nice for the reasons he mentioned (ARC, exception safeness, …), but leads to the problem that if a class implements keyed-subscript, no contexts are supported anymore, because there is no possibility for GRMustache to detect, that a key is absent and it should go a context up.

So actually I have the decision:

• Implementing keyed-subscript and losing contexts.
• Having contexts, but don't having the convenience, keyed-subscript offers to me in many situations.

This is no good result, because there can be dozens of reasons for implementing keyed-subscript. GRMustache needs the ability to check for a valid key. I think the best way to handle that is that the class is asked, whether it has a specific key, before keyed-subscrip is applied. To test that, I added a method to the safe key access protocol (because I found no better location, you will):

@protocol GRMustacheSafeKeyAccess <NSObject>
…
@optional
+ (NSSet *)allowedKeysForKeyedSubscript;
@end

I use that in -valueForMustacheKey:inObject:unsafeKeyAccess::

if ([object respondsToSelector:@selector(objectForKeyedSubscript:)])
{
  if (   ![object respondsToSelector:@selector(allowedKeysForKeyedSubscript)] // no key restriction
      ||  [[object allowedKeysForKeyedSubscript] containsObject:key])         // restricted, but valid key
  {
    return [object objectForKeyedSubscript:key];
  }
}

It seems to work, at least for me.

What do you think about that?

---

The background:

I'm developing an ORM for Objective-Cloud. For convenience I want to offer keyed-subscript for all persistent classes. Doing so, I lose the context stack. Since this is not acceptable for me and our customers, I would remove the generic keyed-subscript support, what would be a pity.

On the other hand it is very easy for me, to find out the allowed keys in a generic manner.

[groue]

Hi @anegmawad. Let me jump to one of your paragraphs. You write:

the problem that if a class implements keyed-subscript, no contexts are supported anymore, because there is no possibility for GRMustache to detect, that a key is absent and it should go a context up.

You are wrong. Let me explain:

The rule is: evaluation goes up the context stack if and only if the key is missing. And the key is missing if f(object, key) is nil, whatever f actually is (KVC or keyed-subscripting).

"Going up the context" is exactly equivalent to "missing key" which is exactly equivalent to "nil value".

"Missing key" is equivalent to "nil value" because it makes all objects behave like NSDictionary. An object which is known by everybody, dull like mud, yet solid as a rock. Nil? Missing key. Not nil? Existing key. Simple. No one discusses that.

Let's keep exploring NSDictionary, and our friend JSON. They match closely.

{
  "name": "root",
  "items": [
    { "name": "child" },
    { "name": null },
    { },
  ]
}

If you render {{#items}}<{{name}}>{{/items}} against it, you get <child><><root>,.

It is quite normal : the last child is the only one that does not define the name key, and has the evaluation go up the context and find the "root" name.

NSNull is the natural Objective-C object which stands for "explicit empty value" (which is not the same as "missing value"). @{ @"name": [NSNull null] } is not equivalent to @{}.

GRMustache has built-in support for NSNull:

• NSNull is false: {{#null}}...{{/null}} does not render, when {{^null}}...{{/null}} does.
• NSNull does not render anything: {{null}} renders the empty string.
• NSNull (not a missing value) stops the context stack lookup (as seen in the above example).

You do not mention NSNull it in your message. Please investigate that option. And let's keep on discussing if you can't find a nice way to introduce NSNull in your scenarios.

[groue]

The background: I'm developing an ORM for Objective-Cloud. For convenience I want to offer keyed-subscript for all persistent classes.

I can understand why you have a problem. ORMs have never been good at dealing with "missing column" vs. "existing columns whose value is NULL". That's a pity, but you're not the first one having this issue. Some would say ORMs are broken by design.

Anyway, you are working on a ORM.

Here is my advice (actually this is what I should have done first):

Declare the public protocol GRMustacheObject as follow:

/**
The protocol for objects that 100% manage their Mustache keys:
*/
@protocol GRMustacheObject
- (id)valueForMustacheKey:(NSString *)mustacheKey;
@end

Add the following lines in GRMustacheKeyAccess.m:

+ (id)valueForMustacheKey:(NSString *)key inObject:(id)object unsafeKeyAccess:(BOOL)unsafeKeyAccess
{
    if (object == nil) {
        return nil;
    }

    // Those lines, below:
    // Try valueForMustacheKey: first (see https://github.com/groue/GRMustache/issues/66:)
    if ([object respondsToSelector:@selector(valueForMustacheKey:)]) {
        return [object valueForMustacheKey:key];
    }

    // The rest of the method

Now your ORM objects are free to conform to GRMustacheObject if you want them to. You are free to invoke objectForKeyedSubscript: for keys that belongs to allowedKeysForKeyedSubscript if you want: now this logic fully belongs to your ORM. The allowedKeysForKeyedSubscript method belongs to your framework, not to GRMustache.

I may well ship GRMustacheObject is a next release, actually... Thank you, and Objective-Cloud :-)

[anegmawad]

The pointer to ORM was just a background information. Simply forget about ORM, if it makes things to complicated.

I don't know, whether it is quite normal to get a value of a different object, if an existing object in the inner context has that key with a value of nil. I would say that this a nil means missing anti pattern similar to the zero means null anti pattern.

However, NSNull is no solution, because keyed-subscript is used here and none would expect to get [NSNull null] on a existing key with a nil value in the rest of the code. Once keyed-subscripting is introduced in a class it can be used by everyone and everyone can expect the behavior he will find at the rest of Cocoa. This is returning nil for nil values, not [NSNull null].

[anegmawad]

To your second comment:

Of course that can be done inside my code. But this does not solve the nil means missing problem. The basic problem is that GRMustache tries to infer the existence of a key from its value. It would be more straight-forward by far, simply to ask the object, whether it has a key. Doing that optional would keep the existing inference in work.

For an instance of NSDictionary it is quite usual to have [NSNull null] as a nil placeholder. For custom classes it is quite unusual.

[groue]

The basic problem is that GRMustache tries to infer the existence of a key from its value.

You don't like the way C, Objective-C and its core frameworks infers the existence of a key from its value. I'm sorry about that, but GRMustache can't go against its ecosystem. Blame C. At least the ObjC guys gave us a standard way to represent an explicit Null value, and that makes us able to work with any dictionary-like objects in a canonical way.

OK now. I did propose above you a solution. The last step is easy: in your implementation of valueForMustacheKey, all you have to do is to return NSNull when the key is known, and nil when the key is unknown. This method won't pollute the rest of your code, and everybody will be happy.

[groue]

The last bits:

@interface ORMObject(GRMustache) : GRMustacheObject
@end

@implementation ORMObject(GRMustache)
- (id)valueForMustacheKey:(NSString *)key {
    if ([self hasColumn:key]) {
        return self[key] ?: [NSNull null];
    } else {
        return nil;
    }
}
@end

Just plug that. And play: you job is done.

[anegmawad]

Objective-C and the core frameworks does not do that. I. e. the standard implementation of -valueForKey: throws an exception and does not return nil. The consequences of such a behavior would be drastic.

Only NSDictionary et al. behaves that way, because they cannot store nil.

[anegmawad]

But I have to change +valueForMustacheKey:inObject:unsafeKeyAccess:, too? Did I understand that correctly?

Why is it a problem for you, to solve the problem "What keys are supported?" by a method with the meaning "Here are the keys I support!" Is it too straight forward? I would like to understand, why you do not want that?

[groue]

But I have to change +valueForMustacheKey:inObject:unsafeKeyAccess:, too? Did I understand that correctly?

Yes : insert the lines below "// Try valueForMustacheKey".

Objective-C and the core frameworks does not do that. I. e. the standard implementation of -valueForKey: throws an exception and does not return nil. The consequences of such a behavior would be drastic. Only NSDictionary et al. behaves that way, because they cannot store nil.

Yes, but why can't NSDictionary store nil? Think a little bit. Even more. Yes, this is because with NSNull, NSDictionary is able to distinguish a missing key from a key containing a missing value.

Why is it a problem for you, to solve the problem "What keys are supported?" by a method [...]?

Because to the question "how do help GRMustache tell apart a missing key from a missing value?", I provide an answer (NSNull), which has many advantages:

• That answer is complete. It fully answers the question.
• That answer is consistent with the Foundation framework. The library user does not have to learn a new concept.
• That answer is unique. The library user does not have to choose between different options.

That's it.

Now, there must be an API which lets the library user fit in this mold.

And the current GRMustache API is not good. It does not even answer the question. It involves three methods:

• valueForKey:
• objectForKeyedSubscript:
• safeMustacheKeys

Of the three methods, two are standards (good), and one is introduced by the library. Unfortunately safeMustacheKeys does not help answering the initial question. It is only there to avoid KVC exceptions.

With the GRMustacheObject (I don't like that name) proposed above, the answer to the question would be complete. It would involve only two methods:

• valueForKey:
• valueForMustacheKey:

Your proposal involves even more methods on top of an already broken API. My proposal reduces the number of involved methods, and fully answer the question.

[anegmawad]

Yes, but why can't NSDictionary store nil? Think a little bit. Even more. Yes, this is because with NSNull, NSDictionary is able to distinguish a missing key from a key containing a missing value.

No, really not, no, it is not true that NSDictionary does not store nil, because it stores [NSNull null]. It is the other way round: It stores [NSNull null], because it cannot store nil. This is the reason for having NSNull.

The NSNull class defines a singleton object used to represent null values in collection objects (which don’t allow nil values).

.

Because to the question "how do help GRMustache tell apart a missing key from a missing value?", I provide an answer (NSNull), which has many advantages:

That answer is complete. It fully answers the question.

This applies to a "allowed keys" (or "has key" or "valid keys" or whatever) solution", too.

That answer is consistent with the Foundation framework. The library user does not have to learn a new concept.

That's simply not true. Thousand of Cocoa classes return nil on KVC, because the value of the key is nil and throw an exception on a missing key. This applies to the basic implementation, valid for ten thousand of classes of Cocoa and custom classes, of -valueForKey:.

The default implementation of valueForUndefinedKey: raises an NSUndefinedKeyException; subclasses can override this behavior.

Additionally it applies to generic classes like NSManagedObject

If key is not a property defined by the model, the method raises an exception.

Outside KVC again you do not have a nil means missing pattern:

• I. e. -primitiveValueForKey: throws an exception, too, IIRC. (Pretty sure)
• CAAnimation's + defaultValueForKey:'s behavior is not returning nil, but undefined

If key is not a known for property of the class, the result of the method is undefined.

You can make that list as long as you want. Solely NSDictionary and NSMutableDictionary behaves the way, you wrote. Therefore it is a > 99 % behavior in Cocoa, to throw an exception on a missing key and a < 1 % behavior to return nilon a missing key. Definetly you cannot call a < 1 % behavior a (common) pattern.

Therefore the idea, to use nil as a marker for missing keys and [NSNull null]as an marker for nil (read that again: GRMustache uses nil for signaling missing key and [NSNull null] for nil, instead of using nil for nil. Isn't that weird?) is a technique that is not consistent with the foundation framework.

That answer is unique. The library user does not have to choose between different options.

For KVC you have to install a exception handler. For keyed-subscript you do not. (BTW: Following your approach, this is not completely correct, because implementations of subscript are allowed to throw exceptions on a missing key. It is implementation defined.)

It is the other way around: Having an availableKeys (or hasKey:) approach, you could use that for all ways accessing the object's properties, KVC, keyed-subscript, direct ivar access, if you want. This is access vector independent. Additionally you would get rid of the problems with the exception handler while debugging: The user could add such a method and will never see exceptions as long as he does everything correct. He will see exceptions, if he does something incorrect. This is really a well-known pattern in Cocoa!

On the other hand I really do not see any caveat for a availableKeys approach, especially if you keep in mind that it is optional.

Your proposal involves even more methods on top of an already broken API. My proposal reduces the number of involved methods, and fully answer the question.

No, as I mentioned before, the keyed-subscript method is not my idea. My idea is to have an allowedKeys approach. So you would have only:

• -valueForKey:
• -allowedKeys or -hasKey: (optional)

This is pretty simple.

[groue]

OK. It looks like we won't find an agreement, and this conversation is going nowhere. You can still use the solution I gave you, since it is a solution to your problem. Have a nice day.