Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

GCDWebServerIsValidByteRange is wrong #317

Closed
spearway opened this issue Jun 28, 2017 · 12 comments
Closed

GCDWebServerIsValidByteRange is wrong #317

spearway opened this issue Jun 28, 2017 · 12 comments
Labels

Comments

@spearway
Copy link

spearway commented Jun 28, 2017

The following code is wrong I suppose the intent was to put an &&

static inline BOOL GCDWebServerIsValidByteRange(NSRange range) {
  return ((range.location != NSUIntegerMax) || (range.length > 0));
}
@swisspol
Copy link
Owner

It's intentional, it's not to be interpreted as a regular NSRange but I didn't feel like creating a custom struct.

See

@property(nonatomic, readonly) NSRange byteRange;
.

@spearway
Copy link
Author

But it does not work, what is the syntax to say you want the entire file.

@spearway
Copy link
Author

spearway commented Jun 28, 2017

What I mean is this is used in the GCDWebServerFileResponse in Swift the Range is not optional so you have to reimplement the logic that is already on ObjC to find the file length.

hasByteRange always return true even if you pass a Range(0,-1)

  BOOL hasByteRange = GCDWebServerIsValidByteRange(range);
  if (hasByteRange) {
    if (range.location != NSUIntegerMax) {
      range.location = MIN(range.location, fileSize);
      range.length = MIN(range.length, fileSize - range.location);
    } else {
      range.length = MIN(range.length, fileSize);
      range.location = fileSize - range.length;
    }
    if (range.length == 0) {
      return nil;  // TODO: Return 416 status code and "Content-Range: bytes */{file length}" header
    }
  } else {
    range.location = 0;
    range.length = fileSize;
  }

@swisspol
Copy link
Owner

GCDWebServerIsValidByteRange () implementation is consistent with the docs:

Returns the parsed "Range" header or (NSUIntegerMax, 0) if absent or malformed.

This in turns matches the way the "Range" HTTP header is parsed:
https://github.com/swisspol/GCDWebServer/blob/master/GCDWebServer/Core/GCDWebServerRequest.m#L185

That said I'm not sure why you're trying to achieve. Either you want to return a range of the file, and use the range arg, or you want the entire file and pass a (NSUIntegerMax, 0) range OR use one of the other GCDWebServerFileResponse initializers without a range argument.

@spearway
Copy link
Author

This is the code:
// We should be able to pass an empty Range but there is a bug in the Web Server
let attr = try self.fileManager.attributesOfItem(atPath: reourceURL.path)
let fileSize = attr[FileAttributeKey.size] as! Int
response = GCDWebServerFileResponse(file: reourceURL.path, byteRange: NSRange(location: 0, length: fileSize), isAttachment: false, mimeTypeOverrides: self.mimeTypes)!

I need to use the full constructor to pass the mime type

@swisspol
Copy link
Owner

Then just pass (NSUIntegerMax, 0)?

@spearway
Copy link
Author

I am in swift I tried to pass (Int.max, -1) but it does not work.

@spearway
Copy link
Author

This make the API very obscure anyway

@spearway
Copy link
Author

spearway commented Jun 29, 2017

I still don't understand why you say that GCDWebServerIsValidByteRange implementation is correct.

The Range is valid if the location is not NSUIntegerMax AND the length is more than zero. This is not what the code says.

I am just asking for the correction of the OR to AND.

@swisspol
Copy link
Owner

Per the docs:

Returns the parsed "Range" header or (NSUIntegerMax, 0) if absent or malformed.

INVALID = (NSUIntegerMax, 0) -> ((range.location == NSUIntegerMax) && (range.length == 0))
VALID = anything else -> ((range.location != NSUIntegerMax) || (range.length > 0))

Simple boolean logic 😄

@spearway
Copy link
Author

Hold on the documentation you are referring to is for the request not for the response, and because the documentation say so it does not means it is right, a range of (0,0) is invalid as the length is 0. and certainly one that is (0,-1) is also invalid.

The pb is that is used at two places for different reasons. It check that the range in the request is valid and you may be right, and it checks that the range in the params of the response constructor is valid. In the second case it is used to detect if the client (the developer) has supplied a range.

I am not interested in fixing the behaviour in the request case but in the response. This can be done in either of 2 ways. by fixing the implementation of the check or better by making the range optional and providing a default value. so that we don't need to pass an invalid range which does not make for a good API anyway.

@swisspol
Copy link
Owner

The doc for response is consistent with doc for request and is also very clear:

/**
 *  Initializes a response like -initWithFile: but restricts the file contents
 *  to a specific byte range. This range should be set to (NSUIntegerMax, 0) for
 *  the full file, (offset, length) if expressed from the beginning of the file,
 *  or (NSUIntegerMax, length) if expressed from the end of the file. The "offset"
 *  and "length" values will be automatically adjusted to be compatible with the
 *  actual size of the file.

So just pass (NSUIntegerMax, 0) or whatever the equivalent is in Swift, and you'll get the whole file. You may not like the API, but there's no bug here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants