Call to undefined method getETag()

[jurgenhaas]

I know this has been reported and "fixed" in #428 but I found another occurance in the 2.x branch. In Sabre\DAV\Server.php line 1347 there is $etag = $node->getETag(); and in my case the $node is an instance of SimpleCollection which doesn't implement that method.

Should the Method getETag be implemented in Node and by default return NULL?

[Hywan]

getETag is required by the IFile interface, so for files, not directories. https://github.com/fruux/sabre-dav/blob/2.1/lib/DAV/Server.php#L1347 must be protected by an instanceof IFile at least. /cc @evert

[evert]

I agree with @Hywan that checking the correct instance is the correct solution.

[evert]

Can you tell us how you triggered this @jurgenhaas , as far as I can tell this only happens in buggy clients. If that's the case it's not really a critical issue (but still worth fixing).

[jurgenhaas]

I'm currently implementing SabreDAV into Drupal (http://drupal.org) and I have an endpoint /sabredav in Drupal's menu router and each enabled submodule (one for cards and one for calendars) implements additional endpoints like /sabredav/addressbooks and /sabredav/calendars. Now, when calling http://www.example.com/sabredav I should be getting the directory that shows addressbooks and calendars as available childs. This is working fine when that line is commented out but it fails if enabled. This is because the $node in that context is a SimpleCollection (implementing Directory and not File) and that class doesn't come with a getETag method.

[evert]

Yes but as far as I can tell this would only be triggered in a case where a client sends an If-Match header (or If-None-Match, which has the same problem) along with a request that never defined an ETag header in the first place.

Without the (broken) If-Match / If-None-Match header, the bug would not occur.

[jurgenhaas]

I don't get it. My client is a web browser and I'm calling a page where I get the BrowserPlugin class to deliver the root directory. And that directory never has an ETag, so it shouldn't be called.

[evert]

So why is your browser sending If-Match, is there something in drupal that automatically generates etags?

[evert]

Actually it's probably If-None-Match in that case.

[jurgenhaas]

That's what I'm not getting: what do you mean by "sending If-Match", I'm not doing anything like that.

[evert]

The error can only be thrown if the browser sends an If-None-Match header. The browser will only send this header if it knows about an etag that the server sent. sabredav does not send etags for collections, thus there is something else that generated an etag.

[jurgenhaas]

OK, as far as I can tell, Drupal is never setting those headers. However, I can confirm that incoming HTTP requests are having that header set if one of the directories is called the second or subsequent times. When I unset that header before initializing the SabreDAV library then I no longer get the error.

Is that Apache that sets that header maybe?

[evert]

It's possible but it would be a very weird and undesirable setting. What is the content of the header? Can you find out when the browser first receives it?

[jurgenhaas]

When receiving a request that contains that header then these are the header values:

AUTHORIZATION: Basic .....
HOST: localhost
USER_AGENT: "Mozilla..."
ACCEPT: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
ACCEPT_LANGAUGE: de,en-US;q=0.7,en;q=0.3
ACCEPT_ENCODING: gzip, deflate
CONNECTION: keep-alive
IF_MODIFED_SINCE: Thu, 21 May 2015 13:31:46 GMT
IF_NONE_MATCH: "1432215106-gzip"
CACHE_CONTROL: max-age=0

[evert]

Interesting =) Do you have some kind of gzip module in your webserver? Try turning it off and wipe your browser cache.

[jurgenhaas]

Should be good for now. I'm not sure where the headers are coming from but the current approach to unset that header in the specific context is working and I need to get something delivered to my client, so I need to get back to that before I can offer more help in debugging this.

[Hywan]

@jurgenhaas I suggest you to not delete the header while it fixes your issue. This is an important one :-p.

[jurgenhaas]

@Hywan you mean not to delete the "whole" header? I'm currently only deleting the IF_NONE_MATCH and leave all the other untouched. And this only happens for the sabredav endpoint, not for all the other paths on that same server.

Or am I misunderstanding your suggestion?

[Hywan]

@jurgenhaas I meant: If-None-Match is also very useful for sabre/dav, so we better might understand how this error happen instead of using such a fix that can be “dangerous” in a short time.

[jurgenhaas]

@Hywan thanks for your clarification. I've looked into the communication a bit deeper and here is what I found:

Drupal, by default responds to HTTP request with an etag in the header which is set to the current timestamp. The client (i.e. the browser) receives the etag with an appended "-gzip" and I'm uncertain where that is coming from but ignore that for now.

The client is then using the etag-value from the previous request as the "If-None-Match" header value for the subsequent request which is what I'm seeing in Drupal as described above.

So, it sounds like I need to avoid that Drupal is adding that etag header for its responses. Is that correct? And if so, under which circumstances should that be removed and when is it important to be imcluded?

[evert]

I would simply suggest to ensure that drupal never does this.

The problem is that both drupal and sabredav may generate and validate etags. If drupal acts as the 'proxy' in front of sabredav, it should fully handle its own etags and not pass them on, while also internally managing the etags coming in and out of sabredav. This is much more sophisticated than drupal's currently functionality, so it's best to disable this altogether.

The problem with just removing the header, is that you effectively turn off conflict detection, so multiple clients may overwrite resources.

[evert]

This is now fixed in master. If you ran master you would no longer see this error, but please keep in mind that there's more subtle, hidden errors that are not immediately prevalent, but will cause loss of data. The commits only fix the error, not your root cause. The only way to fix that, is to fix drupal.

[jurgenhaas]

Thanks @evert for fixing this and also for clarifying the background to me. I don't think there really is an issue with the Drupal implementation as Drupal won't be involved with the client communication when it comes down to updating and syncing cards. Drupal is only delivering html pages to browsers that are reading the tree and that's where Drupal includes the etag in the header. But that's not the case when talking to a WebDAV client.

[evert]

I think that's incorrect. WebDAV clients still use etags, and still use plain HTTP. There's no reason to believe Drupal doesn't do the exact same there, but I did all I could! Good luck.

[jurgenhaas]

Yes they do, but my Drupal implementation routes them to different parts in the code where Drupal's delivery mechanism is bypassed and therefore it won't ever set the header as in the issue handled here.

[evert]

Ah ok, good to hear