Skip to content

Add URI translation, package:// URI scheme & bundle spec schemas #362

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 7, 2017
Merged

Add URI translation, package:// URI scheme & bundle spec schemas #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
167108f
Add URI translation for retrieval & add local copies of spec schema
erayd Feb 24, 2017
61c3c69
Use dist copies of schemas
erayd Feb 24, 2017
cd1ca02
Move package:// translation after all other rules
erayd Feb 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 174 additions & 0 deletions dist/schema/json-schema-draft-03.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
{
"$schema": "http://json-schema.org/draft-03/schema#",
"id": "http://json-schema.org/draft-03/schema#",
"type": "object",

"properties": {
"type": {
"type": [ "string", "array" ],
"items": {
"type": [ "string", { "$ref": "#" } ]
},
"uniqueItems": true,
"default": "any"
},

"properties": {
"type": "object",
"additionalProperties": { "$ref": "#" },
"default": {}
},

"patternProperties": {
"type": "object",
"additionalProperties": { "$ref": "#" },
"default": {}
},

"additionalProperties": {
"type": [ { "$ref": "#" }, "boolean" ],
"default": {}
},

"items": {
"type": [ { "$ref": "#" }, "array" ],
"items": { "$ref": "#" },
"default": {}
},

"additionalItems": {
"type": [ { "$ref": "#" }, "boolean" ],
"default": {}
},

"required": {
"type": "boolean",
"default": false
},

"dependencies": {
"type": "object",
"additionalProperties": {
"type": [ "string", "array", { "$ref": "#" } ],
"items": {
"type": "string"
}
},
"default": {}
},

"minimum": {
"type": "number"
},

"maximum": {
"type": "number"
},

"exclusiveMinimum": {
"type": "boolean",
"default": false
},

"exclusiveMaximum": {
"type": "boolean",
"default": false
},

"minItems": {
"type": "integer",
"minimum": 0,
"default": 0
},

"maxItems": {
"type": "integer",
"minimum": 0
},

"uniqueItems": {
"type": "boolean",
"default": false
},

"pattern": {
"type": "string",
"format": "regex"
},

"minLength": {
"type": "integer",
"minimum": 0,
"default": 0
},

"maxLength": {
"type": "integer"
},

"enum": {
"type": "array",
"minItems": 1,
"uniqueItems": true
},

"default": {
"type": "any"
},

"title": {
"type": "string"
},

"description": {
"type": "string"
},

"format": {
"type": "string"
},

"divisibleBy": {
"type": "number",
"minimum": 0,
"exclusiveMinimum": true,
"default": 1
},

"disallow": {
"type": [ "string", "array" ],
"items": {
"type": [ "string", { "$ref": "#" } ]
},
"uniqueItems": true
},

"extends": {
"type": [ { "$ref": "#" }, "array" ],
"items": { "$ref": "#" },
"default": {}
},

"id": {
"type": "string",
"format": "uri"
},

"$ref": {
"type": "string",
"format": "uri"
},

"$schema": {
"type": "string",
"format": "uri"
}
},

"dependencies": {
"exclusiveMinimum": "minimum",
"exclusiveMaximum": "maximum"
},

"default": {}
}
150 changes: 150 additions & 0 deletions dist/schema/json-schema-draft-04.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
{
"id": "http://json-schema.org/draft-04/schema#",
"$schema": "http://json-schema.org/draft-04/schema#",
"description": "Core schema meta-schema",
"definitions": {
"schemaArray": {
"type": "array",
"minItems": 1,
"items": { "$ref": "#" }
},
"positiveInteger": {
"type": "integer",
"minimum": 0
},
"positiveIntegerDefault0": {
"allOf": [ { "$ref": "#/definitions/positiveInteger" }, { "default": 0 } ]
},
"simpleTypes": {
"enum": [ "array", "boolean", "integer", "null", "number", "object", "string" ]
},
"stringArray": {
"type": "array",
"items": { "type": "string" },
"minItems": 1,
"uniqueItems": true
}
},
"type": "object",
"properties": {
"id": {
"type": "string",
"format": "uri"
},
"$schema": {
"type": "string",
"format": "uri"
},
"title": {
"type": "string"
},
"description": {
"type": "string"
},
"default": {},
"multipleOf": {
"type": "number",
"minimum": 0,
"exclusiveMinimum": true
},
"maximum": {
"type": "number"
},
"exclusiveMaximum": {
"type": "boolean",
"default": false
},
"minimum": {
"type": "number"
},
"exclusiveMinimum": {
"type": "boolean",
"default": false
},
"maxLength": { "$ref": "#/definitions/positiveInteger" },
"minLength": { "$ref": "#/definitions/positiveIntegerDefault0" },
"pattern": {
"type": "string",
"format": "regex"
},
"additionalItems": {
"anyOf": [
{ "type": "boolean" },
{ "$ref": "#" }
],
"default": {}
},
"items": {
"anyOf": [
{ "$ref": "#" },
{ "$ref": "#/definitions/schemaArray" }
],
"default": {}
},
"maxItems": { "$ref": "#/definitions/positiveInteger" },
"minItems": { "$ref": "#/definitions/positiveIntegerDefault0" },
"uniqueItems": {
"type": "boolean",
"default": false
},
"maxProperties": { "$ref": "#/definitions/positiveInteger" },
"minProperties": { "$ref": "#/definitions/positiveIntegerDefault0" },
"required": { "$ref": "#/definitions/stringArray" },
"additionalProperties": {
"anyOf": [
{ "type": "boolean" },
{ "$ref": "#" }
],
"default": {}
},
"definitions": {
"type": "object",
"additionalProperties": { "$ref": "#" },
"default": {}
},
"properties": {
"type": "object",
"additionalProperties": { "$ref": "#" },
"default": {}
},
"patternProperties": {
"type": "object",
"additionalProperties": { "$ref": "#" },
"default": {}
},
"dependencies": {
"type": "object",
"additionalProperties": {
"anyOf": [
{ "$ref": "#" },
{ "$ref": "#/definitions/stringArray" }
]
}
},
"enum": {
"type": "array",
"minItems": 1,
"uniqueItems": true
},
"type": {
"anyOf": [
{ "$ref": "#/definitions/simpleTypes" },
{
"type": "array",
"items": { "$ref": "#/definitions/simpleTypes" },
"minItems": 1,
"uniqueItems": true
}
]
},
"allOf": { "$ref": "#/definitions/schemaArray" },
"anyOf": { "$ref": "#/definitions/schemaArray" },
"oneOf": { "$ref": "#/definitions/schemaArray" },
"not": { "$ref": "#" }
},
"dependencies": {
"exclusiveMaximum": [ "maximum" ],
"exclusiveMinimum": [ "minimum" ]
},
"default": {}
}
38 changes: 37 additions & 1 deletion src/JsonSchema/Uri/UriRetriever.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@
*/
class UriRetriever implements BaseUriRetrieverInterface
{
/**
* @var array Map of URL translations
*/
protected $translationMap = array(
// use local copies of the spec schemas
'|^https?://json-schema.org/draft-(0[34])/schema#?|' => 'package://dist/schema/json-schema-draft-$1.json'
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, I thought I would add https:// support once it's supported but it's even better :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels vaguely not right to me... can't quite put my finger on it, but something about baking these specific use cases into what was previously a very generic class. I'm not totally sure I understand the concept--the idea is that we can sort of intercept what would otherwise be an external request for a resource, and instead proxy one of the local files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shmax The feature itself is generic - it's essentially a regex URI rewriter; you can transform any URI into any other URI.

There are two predefined rules:

  • A rule that allows you to load any schema file from within the package by using a package:// URL prefix;
  • A rule that rewrites official spec schema URLs into local ones.

The user can add more rules, or not, as they see fit.

Are you able to clarify in more detail what aspect of this you're uncomfortable with? If I can get a better understanding of what you're objecting to, then I can try to come up with an approach you're happier with.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It just seems that the feature is entirely bespoke for the needs of the schema validator feature you're working on. You supply an API for adding new translations, but then instead of using it you just pre-config this generic class for your specific needs. Minimally, I would wonder if you could instantiate an instance of this class in-place where you need it, and eat your own dog food. I guess I'm not even too sure if redirecting what one expects to be an external request to a local file is even a good idea? I know we don't generally expect the remote file to change, but what if it does, particularly down the road when we get to draft 5? If the goal is to avoid remote requests, I'm wondering if a better idea might be to add a /cache directory with some kind of expiry or hash mechanism, go ahead and do the remote request, and store a copy of the file. If you're worried about remote being unavailable, you could prime the directory with the files, and check it in.

I'm just spouting ideas, here. I don't know that we necessarily have to solve all this now, and if everybody else is okay with this as-is I can live with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes, the original motivation for adding this was the schema validation stuff. It has usefulness beyond just that though - I figured that as long as I was creating a mechanism for bundling local files anyway, then I might as well make it as widely useful as possible, rather than writing something that only serves my specific use-case.
  2. I preconfigured this specific use-case, because it seems like the one that will be overwhelmingly useful to lots of people, and the users it's most likely to help are also the users least likely to explicitly enable it, because reaching that class through the current API is both a pain in the ass and non-obvious. Doing it this way will also benefit the people who dereference spec URLs anywhere, not just specifically inside the schema-validation feature. It's also a significant security improvement over requesting the spec via HTTP.
  3. I put the preconfiguration in that class specifically, because it seemed like the best place to put it - UrlRetriever is where it's actually used, and the factory isn't available from inside UrlRetriever or any of its related classes.
  4. Is part of your argument that this behavior shouldn't be default-enabled? I personally feel that having it default-enabled is a good thing (per reasons above), but if you disagree then that does change things somewhat.
  5. If the URL asks for anything other than the existing draft-03 or draft-04 specs, the request will just go through unaltered. It only intercepts those specific files, and leaves everything else alone.
  6. If the file located at that URL changes, then IMO the most likely cause is either some kind of error / infrastructure problem (in which case getting a valid copy of the file is problematic), or malicious action (in which case the file can't be trusted). It's a widely-used spec; that file should never change - if there is an error in the spec, then a new version of the spec should be released, rather than silently changing the validation behavior of an existing spec version.
  7. Adding a cache directory implies write access to the filesystem, which comes with its own problems. Personally, I feel that's a road we shouldn't go down - but I'd be interested to hear your thoughts on why it might be a good idea.

You're right, we don't need to solve all this now - but it seems worth having the discussion. There's no huge rush to get any of this stuff merged; it's all nice-to-haves rather than anything critical. If this PR goes nowhere, I'd be OK with that - it just seemed like a good way to solve the current problem while also providing additional value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shmax If I've made you feel like you're having to pick battles, then something is wrong with my approach to discussing this, and for that I am sincerely sorry. I consider your input valuable, and I'm also new to this project - I have no desire or right to be stepping on toes.

I know you've said LGTM above, but would you prefer it if we just put this whole thing on hold for a while and worked on other things, and come back to it later?

I do still need to fix that recursion bug, but I can refrain from any feature-add work for a while if you feel that a break would be helpful. I don't want you to feel like I'm pressuring you; no objection to robust discussion on the merits, but if it's getting to the point of you wanting to back out, then I can't help but feel I've been a bit too robust with the way I've gone about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, no, you're doing great. I only meant that none of my concerns are really heavy enough to warrant stalling this if it will hold up your other PR (which is the fun one). If you want to keep mulling it, one idea would be to merge it into your other PR. Nothing else really depends on this, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shmax Oh, gotcha - thanks for clarifying!

I think that perhaps this warrants a bit more thought - letting it sit here a while is not a problem; as you say, nothing else depends on it - and the schema-validation thing is something I'm doing because there was an issue that asked about it, I have a bit of time to contribute, and I figured it would be helpful. There's no pressing need for that to be finished in a hurry.

I'll base #357 on this PR for now - that way I can keep working on that one, and it won't hold anything up over there - and we can revisit this again one before the schema-validation stuff is merged to see whether I / anyone else still thinks this is a good idea once it's sat quietly for a bit. If we all like it at that point, great! If not, then I'm happy to refactor to a different approach.

Does that sound like a good plan?

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM 🥇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet :-).

);

/**
* @var null|UriRetrieverInterface
*/
Expand Down Expand Up @@ -134,7 +142,7 @@ public function resolvePointer($jsonSchema, $uri)
/**
* {@inheritdoc}
*/
public function retrieve($uri, $baseUri = null)
public function retrieve($uri, $baseUri = null, $translate = true)
{
$resolver = new UriResolver();
$resolvedUri = $fetchUri = $resolver->resolve($uri, $baseUri);
Expand All @@ -146,6 +154,11 @@ public function retrieve($uri, $baseUri = null)
$fetchUri = $resolver->generate($arParts);
}

// apply URI translations
if ($translate) {
$fetchUri = $this->translate($fetchUri);
}

$jsonSchema = $this->loadSchema($fetchUri);

// Use the JSON pointer if specified
Expand Down Expand Up @@ -291,4 +304,27 @@ public function isValid($uri)

return !empty($components);
}

/**
* Set a URL translation rule
*/
public function setTranslation($from, $to)
{
$this->translationMap[$from] = $to;
}

/**
* Apply URI translation rules
*/
public function translate($uri)
{
foreach ($this->translationMap as $from => $to) {
$uri = preg_replace($from, $to, $uri);
}

// translate references to local files within the json-schema package
$uri = preg_replace('|^package://|', sprintf('file://%s/', realpath(__DIR__ . '/../../..')), $uri);

return $uri;
}
}
Loading