Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handling no content-type header returned #498

Closed
wants to merge 1 commit into from

Conversation

joernhees
Copy link
Member

@cliffxuan moved here from #497

@joernhees joernhees added enhancement New feature or request parsing Related to a parsing. labels Jul 20, 2015
@joernhees joernhees added this to the rdflib 4.2.1 milestone Jul 20, 2015
@cliffxuan
Copy link
Contributor

the server doesn't necessarily have to return a content-type header, and you probably don't want to make this a show stopper?

The following is the http response for http://dev.w3.org/2000/10/swap/test/cwm/fam-rules.n3, and it doesn't have a content-type header.

HTTP/1.1 200 OK
Date: Mon, 20 Jul 2015 12:48:50 GMT
Server: Apache/2.2.22 (Debian)
Last-Modified: Fri, 30 Jan 2004 16:08:09 GMT
ETag: "122aa2-96-3d22471246c40"
Accept-Ranges: bytes
Content-Length: 150
Cache-Control: max-age=172800
Expires: Wed, 22 Jul 2015 12:48:50 GMT
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive

@joernhees
Copy link
Member Author

it's correct that content-type isn't mandatory: http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1

the thing i'm not sure about is whether we should return None or '' or just raise an exception right here... what speaks against '' is the handling of content_type in Graph.parse(). If we return '' it won't enter the default handling of if format is None... but then the default handling would be wrong in this case and the comments there show that we should maybe be explicit and raise an exception.

So if at all, i'd rather return None:

         self.content_type = file.info().get('content-type')
-        self.content_type = self.content_type.split(";", 1)[0]
+        if self.content_type is not None:
+             self.content_type = self.content_type.split(";", 1)[0]

@cliffxuan
Copy link
Contributor

both empty string and None are falsy.

        if format is None:
            format = source.content_type
        if format is None:
            # raise Exception("Could not determine format for %r. You can" + \
            # "expicitly specify one with the format argument." % source)
            format = "application/rdf+xml"

I would change the above to:

if not format:
    format = source.content_type or 'application/rdf+xml'

http://stackoverflow.com/questions/9573244/most-elegant-way-to-check-if-the-string-is-empty-in-python

@joernhees
Copy link
Member Author

True, but this would change our API semantics. We usually treat None as default (as in not specified) and '' as in argument was explicitly set to empty string. See the signature of Graph.parse(..., format=None, ...).

If a content-type is specified in HTTP it can't be an empty string, so i think it's worse to suddenly introduce a second undefined '' rather than just return None.

In favor of a smaller change i won't touch Graph.parse() for this... see #499

@joernhees joernhees closed this in 9c38699 Jul 20, 2015
joernhees added a commit that referenced this pull request Jul 20, 2015
fix handling URLInputSource without content-type, closes #498
@joernhees joernhees self-assigned this Jul 20, 2015
@joernhees
Copy link
Member Author

still thanks for reporting this and sorry for opting another way ;)

@cliffxuan
Copy link
Contributor

that's ok. i don't mind too much, to be honest.

@cliffxuan
Copy link
Contributor

a side question, are you also the maintainer of Fuxi? what is the status of it? i created these fixes because i'm playing with the examples in Fuxi.

@joernhees
Copy link
Member Author

I'd say @chimezie and @gjhiggins, but see RDFLib/FuXi#9 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request parsing Related to a parsing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants