Method with provides condition does not match request without Accept header #684

Closed
zooml opened this Issue Mar 15, 2013 · 8 comments

Comments

Projects
None yet
3 participants
@zooml

zooml commented Mar 15, 2013

According to the standard:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
"If no Accept header field is present, then it is assumed that the client accepts all media types."

Looks like the problem is in base.rb:

def preferred_type(*types)
  return accept.first if types.empty?
  types.flatten!
  accept.detect do |pattern|
    type = types.detect { |t| File.fnmatch(pattern, t) }
    return type if type
  end
end

Maybe something like this, with the assumption that the provides condition lists types in order of decreasing preference:

def preferred_type(*types)
  accepts = accept # just evaluate once
  return accepts.first if types.empty?
  types.flatten!
  return types.first if accepts.empty?
  accepts.detect do |pattern|
    type = types.detect { |t| File.fnmatch(pattern, t) }
    return type if type
  end
end
@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Mar 16, 2013

Member

Seems good. Extra karma points if you turn it into a PR and add a test.

Member

rkh commented Mar 16, 2013

Seems good. Extra karma points if you turn it into a PR and add a test.

@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Mar 16, 2013

Contributor

👍 @zooml I've run into this before and just chastised myself for not sending the accept header....

Contributor

JonRowe commented Mar 16, 2013

👍 @zooml I've run into this before and just chastised myself for not sending the accept header....

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Mar 17, 2013

Member

An alternative would be to just do a

env['HTTP_ACCEPT'] ||= "*/*"

instead in some middleware or something.

Not sure if that's better or worse.

Member

rkh commented Mar 17, 2013

An alternative would be to just do a

env['HTTP_ACCEPT'] ||= "*/*"

instead in some middleware or something.

Not sure if that's better or worse.

@zooml

This comment has been minimized.

Show comment Hide comment
@zooml

zooml Mar 18, 2013

@JonRowe As the HTTP standard is used by literally billions of devices world wide it might be best if sinatra were to conform rather than making the billions of devices use a workaround :-)

@rich Adding the header does the job but would be better to just fix the code and not make it look like the client sent the header.

I'll fork and submit a PR.

zooml commented Mar 18, 2013

@JonRowe As the HTTP standard is used by literally billions of devices world wide it might be best if sinatra were to conform rather than making the billions of devices use a workaround :-)

@rich Adding the header does the job but would be better to just fix the code and not make it look like the client sent the header.

I'll fork and submit a PR.

@zooml

This comment has been minimized.

Show comment Hide comment
@zooml

zooml Mar 18, 2013

Made the code changes, per my original post above, added/modified unit tests, pushed to missing-accept-hdr branch off master in this forked repo: https://github.com/zooml/sinatra

Created pull request.

Thanks.

zooml commented Mar 18, 2013

Made the code changes, per my original post above, added/modified unit tests, pushed to missing-accept-hdr branch off master in this forked repo: https://github.com/zooml/sinatra

Created pull request.

Thanks.

@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Mar 18, 2013

Contributor

@zooml Sending the correct header isn't really a workaround ;) and in any case I was suggesting this is a good thing!

Contributor

JonRowe commented Mar 18, 2013

@zooml Sending the correct header isn't really a workaround ;) and in any case I was suggesting this is a good thing!

@zooml

This comment has been minimized.

Show comment Hide comment
@zooml

zooml Mar 18, 2013

@JonRowe Not sure you're understanding the HTTP standard, the absence of an Accept header means that the client will accept any mime type. A request without an Accept header is correct. If sinatra is to conform to the HTTP standard then it must interpret the absence of an Accept header as the client will accept any mime type.

zooml commented Mar 18, 2013

@JonRowe Not sure you're understanding the HTTP standard, the absence of an Accept header means that the client will accept any mime type. A request without an Accept header is correct. If sinatra is to conform to the HTTP standard then it must interpret the absence of an Accept header as the client will accept any mime type.

@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Mar 18, 2013

Contributor

@zooml No I understand the HTTP standard. I'm agreeing with you.

Contributor

JonRowe commented Mar 18, 2013

@zooml No I understand the HTTP standard. I'm agreeing with you.

rkh added a commit that referenced this issue Mar 20, 2013

Merge pull request #689 from zooml/missing-accept-hdr
#684 missing accept header should be same as */* per http standard

@rkh rkh closed this Mar 21, 2013

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