search

RepreZen / KaiZen-OpenApi-Parser

High-performance Parser, Validator, and Java Object Model for OpenAPI 3.x
130 stars 31 forks source link

pathInParent value has no assessor in version 2.0.0.201803231800 #155

Closed dhoffer closed 6 years ago

dhoffer commented 6 years ago

Our code as well as the sample code that shows how to traverse the parsed model is missing the Path#getPathInParent() method in version 2.0.0.201803231800. This was available in 0.0.3.201803041924. This is preventing us from using the new version.

andylowry commented 6 years ago

@dhoffer This part of the API has been redesigned. See #151 for a brief summary, or https://github.com/RepreZen/JsonOverlay/pull/2 for a more complete discussion.

From here on out you should expect this project and the recently split out JsonOverlay project to follow semantic version rules, so moving to a new major release will involve breaking changes.

dhoffer commented 6 years ago

Okay changing to Overlay.of(path).getPathInParent() does work. One thought/observation about this is that it makes it harder to know how to do things because one has to have knowledge of Overlay and its out of band interaction with Path. E.g. There is no way to know how to get this data without separately knowing that Overlay exists and how it connects to a Path. Would it be possible for Path to have a getOverlay() method so that one can traverse all possible values just starting with a Path?

andylowry commented 6 years ago

@dhoffer Thanks for the feedback. You're right that the Overlay class is somewhat less natural and convenient than having the methods directly available in the generated API.

One of the main motivations for the change was to stop polluting the API - which is really intended to reflect the underlying JSON structure - with these methods. Polluting and potentially colliding. This was especially clumsy in the methods to inspect reference information. Certain types had to be designated in the types configuration as "refable", and then objects referencing those types would be equipped with clunky sounding methods like isSchemaReference(name) and getSchemaReference(name). The methods had to be part of the object that contained the schema, not in the schema itself, because the same schema object could be reachable by many paths within a model, one of which would be reference-free. The complications in the internal representation to support this were significant, and the approach still hides references that may appear in the model but do not involve "refable" types. The static Overlay class does a much nicer job of making reference information available.

While I'm hesitant to build something like getOverlay() into the framework itself, there's no reason it couldn't be done in a specific scenario like the OpenApi parser - since we know that there's nothing called an "overlay" in the OpenApi spec, so no concern about name collisions. But the framework is already being used for at least one other project (currently private) that is completely unrelated to OpenApi.

Something similar is already contemplated in https://github.com/RepreZen/JsonOverlay/issues/4. Adding getOverlay() to the OpenApi classes could be handled in the same way. Let me know what you think...

tedepstein commented 6 years ago

Interesting sidebar to this discussion: I recently participated in an OpenAPI TSC call, where the Mulesoft team described a RAML 2.0 feature called an Overlay, proposing to add this feature to OpenAPI. The proposal is under consideration.

RAML overlays allow non-structural information like documentation, examples, etc., to be specified in an external document, and merged in with structural definitions to create the final RAML spec. It's a handy feature for teams using code-first, or even contract-first, where there is a dedicated technical writer or product manager responsible for writing the docs, and it's not convenient for that person to make these contributions directly in the source.

Obviously, this has nothing to do with JSON Overlay. But it illustrates one of the very good reasons for isolating the JSON Overlay API from the primary domain API. Without this isolation, name conflicts can easily arise. Even the getOverlay() method proposed here could very well come in conflict with this proposed new OpenAPI feature!

If we are going to do it, I'd suggest making it getJsonOverlay(), or even _jsonOverlay, which will never conflict with a standard, bean-style accessor.

tedepstein commented 6 years ago

BTW, @andylowry , if you want me to move my comment to RepreZen/JsonOverlay#4, just let me know.