Skip to content

Conversation

@miikka
Copy link
Contributor

@miikka miikka commented Aug 12, 2016

This avoids downloading a schema from the Internet if it refers to itself. E.g. ring-swagger downloads the Swagger schema from the Internet even though it's bundled into ring-swagger.

See http://json-schema.org/latest/json-schema-core.html#anchor30.

@ikitommi
Copy link
Member

If some customers can break with this, it should be a MINOR version bump to 0.4.0-SNAPSHOT? Anyways, looks Good.

@miikka
Copy link
Contributor Author

miikka commented Aug 13, 2016

Fair point, I'll amend the patch.

This avoids downloading a schema from the Internet if it refers to
itself.

See <http://json-schema.org/latest/json-schema-core.html#anchor30>.
@miikka miikka force-pushed the inline-dereferencing branch from d047813 to ab943e2 Compare August 13, 2016 06:38
@miikka
Copy link
Contributor Author

miikka commented Aug 13, 2016

@lvh: I'm thinking of enabling inline dereferencing as default in scjsv. I played around a bit and it seems like the "do what I mean" behavior to me. Since you were also looking at this, let me know if you think it's a bad idea.

@lvh
Copy link
Contributor

lvh commented Aug 13, 2016

It seems obvious to me as well :)

@lvh
Copy link
Contributor

lvh commented Aug 13, 2016

The code I have for this is marginally different:

(def ^:private ^JsonSchemaFactory json-schema-factory
  (let [loading-cfg (-> (LoadingConfiguration/byDefault)
                        (.thaw)
                        (.dereferencing Dereferencing/INLINE)
                        (.freeze))]
    (-> (JsonSchemaFactory/byDefault)
        (.thaw)
        (.setLoadingConfiguration loading-cfg)
        (.freeze))))

So, the difference is that I thaw the default into a factory and then just set inline dereferencing. One of the interesting things about the default is that it already has the JSON Schema spec itself preloaded, so you can use references to it. The way I read the documentation for that, if you use your own builder, you must either preload that schema yourself -- it is part of the default JsonSchemaFactory, but not the default JsonSchemaFactoryBuilder. I did not write a test to verify that that is the case. If you want to write such a test, here's an excerpt from the Swagger spec, which defines a subset of JSON Schema (in JSON Schema):

        "title": {
          "$ref": "http://json-schema.org/draft-04/schema#/properties/title"
        },

@miikka
Copy link
Contributor Author

miikka commented Aug 13, 2016

The documentation is not very clear on this, but if you look at the sources of JsonSchemaFactory and LoadingConfiguration, you'll see that thawing has the same result as using newBuilder. That is, the JSON Schema schema is preloaded in both cases.

@lvh
Copy link
Contributor

lvh commented Aug 13, 2016

Aweome -- like I said, I didn't investigate particularly hard once I got it working :) Other than that, this patch looks great to me and would remove some code from my project :D

@lvh
Copy link
Contributor

lvh commented Aug 15, 2016

Huh. You should actually hold off on this for a bit; I found an interesting bug with Swagger verification that only happens with inline dereferencing, and I haven't quite figured out where the bug is yet. I'm going to go to lunch and then do a write-up :)

@lvh
Copy link
Contributor

lvh commented Aug 15, 2016

@lvh
Copy link
Contributor

lvh commented Aug 15, 2016

Stuff like OAI/OpenAPI-Specification#135 would suggest that maybe inline dereferencing is not what you want if you have to deal with other people's schemas :)

@miikka
Copy link
Contributor Author

miikka commented Aug 16, 2016

Ah well. Thanks for the information – I'll try to understand it and figure
out what to do. I'm away this week so it'll have to wait a bit.

On 15. elokuuta 2016 at 11.12.57, lvh ([email protected]) wrote:

Stuff like OAI/OpenAPI-Specification#135
OAI/OpenAPI-Specification#135 would suggest
that maybe inline dereferencing is not what you want if you have to deal
with other people's schemas :)


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#3 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AACHL6R9MnRhwSaHPE3ZIBXn6sd_2IUsks5qgB-JgaJpZM4JjL73
.

@lvh
Copy link
Contributor

lvh commented Aug 16, 2016

The plot thickens! I want to not load stuff, so I've been trying to preload the JSON Schema. Somehow, preloading a schema makes it break. As I mentioned in that ticket I've been testing this with the JSON Schema for Swagger and the petstore-minimal Swagger.

(defn ^:private ^JsonSchemaFactory json-schema-factory
  [preloads]
  (let [lcfg-builder (reduce
                      (fn [builder preload]
                        (.preloadSchema ^LoadingConfigurationBuilder builder
                                        (-> preload
                                            json/generate-string
                                            JsonLoader/fromString)))
                      (.thaw (LoadingConfiguration/byDefault))
                      preloads)]
    (-> (JsonSchemaFactory/byDefault)
        (.thaw)
        (.setLoadingConfiguration (.freeze lcfg-builder))
        (.freeze))))

(the json ns is cheshire)

Not preloading: works fine. Preloading:

{:level "error",
:schema {:loadingURI "http://swagger.io/v2/schema.json#",
:pointer "/definitions/responses"},
:instance {:pointer "/paths/~1pets/get/responses"},
:domain "validation",
:keyword "additionalProperties",
:message "object instance has properties which are not allowed by the schema: [\"200\"]", :unwanted ["200"]}

@lvh
Copy link
Contributor

lvh commented Aug 16, 2016

Aaaaa! Ignore that last comment. Turns out it was a problem with camel-case-kebab quietly transforming some JSON for me.... You probably should enable inline dereferencing by default anyway; I'll see if it works for swagger. The swagger spec still says that it only supports canonical dereferencing, but I'm still not sure what it even means for a spec to say that.

I figured out that inline dereferencing is not the right default after
all. I had misunderstood it and in most cases the canonical mode does
the right thing whereas the inline mode also allows weird behavior.

Therefore, let's not make it default, but let's allow enabling it anyway
for experimenting.
@miikka miikka changed the title Default to inline dereferencing Make it possible to enable inline dereferencing Oct 1, 2016
@miikka
Copy link
Contributor Author

miikka commented Oct 1, 2016

I've ended up thinking that inline dereferencing is not the right default after all, but we should nevertheless have a way to enable it. Hence the latest version of this patch.

@lvh
Copy link
Contributor

lvh commented Oct 3, 2016

This looks good to me :)

@ikitommi
Copy link
Member

ikitommi commented Oct 3, 2016

👍

@miikka miikka merged commit fa73950 into metosin:master Oct 3, 2016
@miikka miikka deleted the inline-dereferencing branch October 3, 2016 09:23
@lvh
Copy link
Contributor

lvh commented Oct 3, 2016

If you cut a release with this change, I'd be much obliged :)

@miikka
Copy link
Contributor Author

miikka commented Oct 4, 2016

Released 0.4.0.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants