Added PROPFIND verb support #897

Merged
merged 1 commit into from Aug 11, 2014

6 participants

@markbates

No description provided.

@kytrinyx
Sinatra member

It's been years since I had anything to do with WebDAV, and I completely forgot about PROPFIND.

If there is no propfind block defined for a pattern, would the sinatra app respond correctly with a 405?

@rkh
Sinatra member
rkh commented Jul 4, 2014

We had the discussion concerning WebDAV words before. IIRC the proposal was to add LIST. I seem unable to find it though, it might predate our switch to GitHub issues.

Anyhow, the reasoning we rejected the idea went something like this: Unless we fully implement WebDAV, we should not add WebDAV-only verbs to Sinatra itself. We should not fully implement WebDAV in Sinatra itself, since it is not a common enough use case. Instead I'd rather like to see a WebDAV extension for Sinatra.

This is of course up to debate.

@markbates are you allowed to tell us what your use case is? ;)

If there is no propfind block defined for a pattern, would the sinatra app respond correctly with a 405?

Not with the current implementation, it should result in a 404. I think we had a patch once to add this behaviour, but it was slow and fragile.

@markbates

@rkh I'm building a simple CardDAV server so that's why the need for PROPFIND.

I agree that Sinatra shouldn't implement WebDAV, but to me an HTTP Verb is an HTTP Verb. It would be nice to easily support the verbs, even if it's just basic support for them.

I could add an implementation like the following if we really need it to return a 405 by default:

def propfind(path, opts = {}, &bk)
  block = bk ? bk : ->{halt 405}
  route 'PROPFIND', path, opts, &block
end

With that said, I think having it return a 405 by default is probably a bit much, but that's just my opinion.

@kytrinyx
Sinatra member

I think having it return a 405 by default is probably a bit much

Yeah, agreed.

I'm on the fence about "an HTTP verb is an HTTP verb". It's a custom HTTP verb used by WebDAV. Would this argument extend to other custom HTTP verbs from other extensions?

@rkh
Sinatra member
@markbates

I'm cool either way. I saw a "missing" verb, and figured I would submit.

I'm not planning on doing this app in Sinatra, but rather Go. I'm just doing a simple spike using Sinatra first. I won't be offended if you reject the PR.

With that said, it would be great if there was at least a contrib package supporting the WebDAV verbs. It is a pretty big extension that's used a lot.

Just my $.02 though.

@yegortimoshenko

Of course it should be an extension, not Sinatra core. Sinatra currently is a little bit heavier than it should be (waiting for 2.0 to solve this problem) and including WebDAV methods will make the situation worse. And, once merged, it will become a part of Sinatra API that should be maintained and supported in future versions.

@JonRowe

How about adding a method to the Sinatra DSL to easily support custom verbs?

@zzak
Sinatra member

Yes. This code could be refactored, but until we have a patch I'm ok with merging this.

@zzak zzak merged commit 68c237d into sinatra:master Aug 11, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@yegortimoshenko

@zzak this is a wrong move.

It is not a mainstream HTTP verb: it is not included in any HTTP RFC (RFC 2616, RFC 1945).

It is included in WebDAV HTTP extension RFC 4918 though. But then patch should include all other WebDAV methods before it's merged. Otherwise, Sinatra won't be consistent.

After cutting the release, #propfind will become a part of public API which should be maintained. It won't be possible to just throw it away in the next minor version, for example.

I believe this merge should be reverted and this functionality introduced in sinatra/webdav.
I will make a patch to sinatra-contrib with WebDAV verbs.

/cc @rkh

@zzak
Sinatra member

@yegortimoschenko Lets see a patch first :)

@rkh
Sinatra member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment