Skip to content

JSON Hyper-Schema usage with Express: #1

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

Closed
wants to merge 6 commits into from

Conversation

bpanahij
Copy link

I want to use express as a hypermedia server, for a JSON Hyper-Schema driven API. The JSON-Schema core spec defines in section 8.1 a profile parameter, on Content-type headers; That parameter accepts a uri, including the http, "://", fqdn, and path, as well as a potential #hash-tag. This PR adds the ability for media-typer to handle this type of profile parameter.

Without this change, media-typer would parse the following profile parameter:

Content-type: application/passport+json; profile=http://localhost:8080/schema/homes/home/schema.json#;

as:

profile=http

With this change:

profile=http://localhost:8080/schema/homes/home/schema.json#

Also, this change still successfully parses:

application/json; charset=utf-8; profile=http://localhost:8080/schema/homes/home/schema.json#

as:

charset=utf-8
profile=http://localhost:8080/schema/homes/home/schema.json#

dead-horse and others added 3 commits July 6, 2014 01:41
* Adding capability to parse a content-type header like so:
Content-Type: application/my-media-type+json;
          profile=http://example.com/my-hyper-schema#
 * See: http://json-schema.org/latest/json-schema-core.html, section 8.1. Correlation by means of the "Content-Type" header
* Only need on slash to match slashes
@dougwilson
Copy link
Contributor

Well, you can't just change a regular express that matches an RFC. Can you either show me how the regular expression does not correctly match RFC 2616, or give me what RFC your change matches?

* Updating version for npm
@dougwilson
Copy link
Contributor

Anyway, application/json; charset=utf-8; profile=http://localhost:8080/schema/homes/home/schema.json# is invalid; the value of a parameter must be a token or a quoted string and a token cannot contain a / character.

The valid content-type would be application/json; charset=utf-8; profile="http://localhost:8080/schema/homes/home/schema.json#"

@dougwilson
Copy link
Contributor

Though parsing the parameter profile=http://localhost:8080/schema/homes/home/schema.json# as profile=http is a bug; it should actually be throwing an error for that case.

@dougwilson
Copy link
Contributor

FYI, the description for the profile parameter in http://json-schema.org/latest/json-schema-core.html is correct, but the example is invalid. I checked the new HTTP RFCs, and it's still invalid according to RFC 7230.

@dougwilson
Copy link
Contributor

I'm going to re-open to think on this for a while because RFC 6838 states there is no actual defined format for parameter values, but the transport (HTTP in this case) can impose their own restriction (like, for example, that : and / are not valid outside a quoted string).

For example, a solution I may do here is parse according to RFC 6838 (which would parse your example correctly), but provide an option to stringify as RFC 6838 or RFC 7230. I'm still thinking about it.

@dougwilson dougwilson reopened this Aug 10, 2014
@dougwilson dougwilson self-assigned this Aug 10, 2014
@bpanahij
Copy link
Author

Hey Doug,

Thanks for reviewing this PR.

I see what you are saying that there is no defined syntax for parameter values, and yet still, some transfer protocols (http) may enforce their own syntax. I like your idea to make it an option. I am trying to think of ways in which this change would affect end users of your package. I know express uses media-typer, and express does use an HTTP protocol.

express:lib/util.js

exports.setCharset = function(type, charset){
  if (!type || !charset) return type;

  // parse type
  var parsed = typer.parse(type);

  // set charset
  parsed.parameters.charset = charset;

  // format type
  return typer.format(parsed);
};

Here format also imposes some syntax, by quoting certain parameters:

// append parameters
  if (parameters && typeof parameters === 'object') {
    var param
    var params = Object.keys(parameters).sort()

    for (var i = 0; i < params.length; i++) {
      param = params[i]

      if (!tokenRegExp.test(param)) {
        throw new TypeError('invalid parameter name')
      }

      string += '; ' + param + '=' + qstring(parameters[param])
    }
  }

Perhaps this is enough; although it would be nice not to quote a profile parameter value. (I also considered making this change PR: removing the qstring call, and just assigning the raw value)

There is no defined syntax for parameter values. Therefore, registrations MUST specify parameter value syntax. Additionally, some transports impose restrictions on parameter value syntax, so care needs be taken to limit the use of potentially problematic syntaxes; e.g., pure binary valued parameters, while permitted in some protocols, are best avoided.

Note that a protocol can impose further restrictions on parameter value syntax, depending on how it chooses to represent parameters. Both MIME [RFC2045] [RFC2231] and HTTP [RFC2045] [RFC5987] allow binary parameters as well as parameter values expressed in a specific charset, but other protocols may be less flexible.

@dougwilson
Copy link
Contributor

Well, I know how express is using it, because I wrote it in there :)

although it would be nice not to quote a profile parameter value

that's just not something I'm going to allow if the media type is going to end up in the content-type header, which is clearly specified in the protocol's documentation about what it's valid format is. allowing a server to send out non-valid content-type values can cause problems in clients that do not expect that. typically for compatibility, a server should parse incoming data in a forgiving way, but emit data in a strict way.

@bpanahij
Copy link
Author

Doug,

Thanks for clarifying. Now I see how qstring is necessary for following the HTTP protocol used in express. Does it seem to you that JSON Hyper-Schema is disobeying the HTTP spec?

The reason I ask is because, I am running into another issue, where a client library that follows the JSON-Schema spec is doing the following:
I am currently working with a client library that doesn't handle the quoted string well: https://github.com/ericgj/json-schema-suite

This repo appends the quoted profile value to the current url, and asks for the schema there:

GET: http://www.mysite.com/api/v1/homes%20http://localhost:8080/schema/homes/home/schema.json#

and makes a schema request to the profile value when responded without quotes.

GET: http://localhost:8080/schema/homes/home/schema.json#

I can follow up with this repo owner, and I'd also like you feedback, because the JSON Schema seems to be wrong here.

@@ -1,7 +1,7 @@
{
"name": "media-typer",
"description": "Simple RFC 6838 media type parser and formatter",
"version": "0.2.0",
"version": "0.2.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please never, ever, do this in PR's.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 Thanks, I'll remove the modification to the package.json commit.

@dougwilson
Copy link
Contributor

Does it seem to you that JSON Hyper-Schema is disobeying the HTTP spec?

Only if that string example is what is literally set as the Content-Type header, yes. If the example is protocol-independent (i.e. it's not a literal example for use in HTTP), then no.

The reason I ask is because, I am running into another issue, where a client library that follows the JSON-Schema spec is doing the following

Sorry, I'm not following what you are seeing from that quote and below in your comment. We are talking about the Content-Type header here. Looking at their code, they parse the content-type with https://github.com/ericgj/json-schema-suite/blob/master/build.js#L4484 which is clearly invalid. That is their bug, though. It is some "standard" naïve parsing which would run into various issue parsing things correctly.

This is one of the reasons we are working to get things like this library in the jshttp organization that allows people to parse HTTP-related things correctly and not just sometimes right.

@bpanahij
Copy link
Author

Doug,

As far is this PR, Do you still think it's worth considering changing the regex?

Thanks for pointing out that line. I'll follow up with the other repo owner.

@dougwilson
Copy link
Contributor

As far is this PR, Do you still think it's worth considering changing the regex?

The regex changed specifically is meant to exactly match that from the RFC in the comment above it, so the regex there will never change. But this PR still brings up a couple issues that I'll want to address:

  1. profile=http://blah should NOT parse as {profile: 'http'}; it should throw
  2. I have to think about if I want this module to parse non-HTTP stuff and how exactly that would work.

@bpanahij
Copy link
Author

Thanks for thinking about this.

What would a valid profile parameter on a Content-type look like for HTTP protocol?

@dougwilson
Copy link
Contributor

What would a valid profile parameter on a Content-type look like for HTTP protocol?

Here is the example I commented above:

application/json; charset=utf-8; profile="http://localhost:8080/schema/homes/home/schema.json#"

@bpanahij
Copy link
Author

Thanks Doug.

* Reverting version update
* Don't need to add quotations around Media Type parameter
@bpanahij
Copy link
Author

Thanks for all your help Doug,

I know how to support what I want to do, and it involves removing the qstring and changing the regex.

After adding quotes to profile (before handing it off to express' setHeader method), media-typer still strips out the url value. That won't work for JSON Hyper Schema.

If you would like I can leave this PR open, or I can close it for now.

I am going to use my own fork of this repo in my project until I better understand how I can make this functionality work for everyone.

@dougwilson
Copy link
Contributor

If you would like I can leave this PR open

Yes, please just leave it open for now, because it brings up some issues in here, so it's a good reminder.

media-typer still strips out the url value

I have no idea what you mean. Can you show me code that does that? I'm using this:

var typer = require('media-typer')
var type = 'application/json; charset=utf-8; profile="http://localhost:8080/schema/homes/home/schema.json#"'
var parsed = typer.parse(type)
parsed.parameters.charset = 'iso-8859-1'
console.log(typer.format(parser))
// logs application/json; charset=iso-8859-1; profile="http://localhost:8080/schema/homes/home/schema.json#"

and as you can see, it's clearly not stripping out the quoted profile.

@bpanahij
Copy link
Author

and as you can see, it's clearly not stripping out the quoted profile.

OK, my mistake, I can verify it works like you show, when using your test script. I guess I just needed to add the quotations...and that solves my issue.

Thanks again.

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

Successfully merging this pull request may close these issues.

4 participants